Re: [PATCH 3/3] sched/deadline: Consolidate Timer Cancellation

[Wander Lairson Costa]

On Tue, Jul 23, 2024 at 6:15 AM Juri Lelli <juri.lelli@xxxxxxxxxx> wrote:
>
> Hi Wander,
>
> Guess this might help yes, only a minor naming suggestion below.
>
> On 22/07/24 10:29, Wander Lairson Costa wrote:
> > After commit b58652db66c9 ("sched/deadline: Fix task_struct reference
> > leak"), I identified additional calls to hrtimer_try_to_cancel that
> > might also require a dl_server check. It remains unclear whether this
> > omission was intentional or accidental in those contexts.
> >
> > This patch consolidates the timer cancellation logic into dedicated
> > functions, ensuring consistent behavior across all calls.
> > Additionally, it reduces code duplication and improves overall code
> > cleanliness.
> >
> > Note the use of the __always_inline keyword. In some instances, we
> > have a task_struct pointer, dereference the dl member, and then use
> > the container_of macro to retrieve the task_struct pointer again. By
> > inlining the code, the compiler can potentially optimize out this
> > redundant round trip.
> >
> > Signed-off-by: Wander Lairson Costa <wander@xxxxxxxxxx>
> > ---
> >  kernel/sched/deadline.c | 44 ++++++++++++++++++++++++++---------------
> >  1 file changed, 28 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index 908d5ce79425..8b0bbade2dcb 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -320,6 +320,29 @@ void sub_running_bw(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
> >               __sub_running_bw(dl_se->dl_bw, dl_rq);
> >  }
> >
> > +static __always_inline
> > +void cancel_timer(struct sched_dl_entity *dl_se, struct hrtimer *timer)
> > +{
> > +     /*
> > +      * If the timer callback was running (hrtimer_try_to_cancel == -1),
> > +      * it will eventually call put_task_struct().
> > +      */
> > +     if (hrtimer_try_to_cancel(timer) == 1 && !dl_server(dl_se))
> > +             put_task_struct(dl_task_of(dl_se));
> > +}
> > +
> > +static __always_inline
> > +void cancel_dl_timer(struct sched_dl_entity *dl_se)
>
> Maybe we could call the above cancel_replenish_timer
>
> > +{
> > +     cancel_timer(dl_se, &dl_se->dl_timer);
>
> and this one cancel_dl_timer?
>

That makes sense, I will change it in v2.

> Best,
> Juri
>