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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAq0SUmQV6-NJnFGNuCFp-dTpFAft0tV0WL-Cht9dG_Sa1fCDA@mail.gmail.com>
Date: Tue, 23 Jul 2024 09:33:09 -0300
From: Wander Lairson Costa <wander@...hat.com>
To: Juri Lelli <juri.lelli@...hat.com>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 
	Vincent Guittot <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>, 
	Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, "open list:SCHEDULER" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] sched/deadline: Consolidate Timer Cancellation

On Tue, Jul 23, 2024 at 6:15 AM Juri Lelli <juri.lelli@...hat.com> 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@...hat.com>
> > ---
> >  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
>