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

[Juri Lelli <juri.lelli@redhat.com>]

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@redhat.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