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: <Zp90uZSUPkLmluEo@jlelli-thinkpadt14gen4.remote.csb>
Date: Tue, 23 Jul 2024 11:15:37 +0200
From: Juri Lelli <juri.lelli@...hat.com>
To: Wander Lairson Costa <wander@...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

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?

Best,
Juri