[PATCH 0/3] sched/deadline: fixes and improvements

[PATCH 0/3] sched/deadline: fixes and improvements

[Wander Lairson Costa]

Hello,

This patch series addresses specific issues and improvements within
the scheduler's deadline (DL) class. The patches aim to fix warnings,
remove redundant checks, and consolidate timer cancellation logic for
better consistency and code quality.

Patch 1: Fix warning in migrate_enable for boosted tasks

Fix a warning caused by unnecessary calls to setup_new_dl_entity for
boosted tasks during CPU migate_enable calls.

Patch 2: sched/deadline: avoid redundant check for boosted task

Remove redundant WARN_ON check in enqueue_dl_entity, as boosted tasks
do not require setup_new_dl_entity.

Patch 3: sched/deadline: Consolidate Timer Cancellation

Consolidate timer cancellation logic into dedicated functions,
ensuring consistent behavior and reducing code duplication.

Wander Lairson Costa (3):
  sched/deadline: Fix warning in migrate_enable for boosted tasks
  sched/deadline: avoid redundant check for boosted task
  sched/deadline: Consolidate Timer Cancellation

 kernel/sched/deadline.c | 56 +++++++++++++++++++++++++++--------------
 1 file changed, 37 insertions(+), 19 deletions(-)

-- 
2.45.2

[PATCH 1/3] sched/deadline: Fix warning in migrate_enable for boosted tasks

[Wander Lairson Costa]

When running the following command:

while true; do
    stress-ng --cyclic 30 --timeout 30s --minimize --quiet
done

a warning is eventually triggered:

WARNING: CPU: 43 PID: 2848 at kernel/sched/deadline.c:794
setup_new_dl_entity+0x13e/0x180
...
Call Trace:
 <TASK>
 ? show_trace_log_lvl+0x1c4/0x2df
 ? enqueue_dl_entity+0x631/0x6e0
 ? setup_new_dl_entity+0x13e/0x180
 ? __warn+0x7e/0xd0
 ? report_bug+0x11a/0x1a0
 ? handle_bug+0x3c/0x70
 ? exc_invalid_op+0x14/0x70
 ? asm_exc_invalid_op+0x16/0x20
 enqueue_dl_entity+0x631/0x6e0
 enqueue_task_dl+0x7d/0x120
 __do_set_cpus_allowed+0xe3/0x280
 __set_cpus_allowed_ptr_locked+0x140/0x1d0
 __set_cpus_allowed_ptr+0x54/0xa0
 migrate_enable+0x7e/0x150
 rt_spin_unlock+0x1c/0x90
 group_send_sig_info+0xf7/0x1a0
 ? kill_pid_info+0x1f/0x1d0
 kill_pid_info+0x78/0x1d0
 kill_proc_info+0x5b/0x110
 __x64_sys_kill+0x93/0xc0
 do_syscall_64+0x5c/0xf0
 entry_SYSCALL_64_after_hwframe+0x6e/0x76
 RIP: 0033:0x7f0dab31f92b

This warning occurs because set_cpus_allowed dequeues and enqueues tasks
with the ENQUEUE_RESTORE flag set. If the task is boosted, the warning
is triggered. A boosted task already had its parameters set by
rt_mutex_setprio, and a new call to setup_new_dl_entity is unnecessary,
hence the WARN_ON call.

Check if we are requeueing a boosted task and avoid calling
setup_new_dl_entity if that's the case.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
Fixes: 2279f540ea7d ("sched/deadline: Fix priority inheritance with multiple scheduling classes")

---

The initial idea for this fix was to introduce another ENQUEUE flag,
ENQUEUE_SET_CPUS_ALLOWED. When this flag was set, the deadline scheduler
would bypass the call to setup_new_dl_entity, regardless of whether the
task was boosted. However, this idea was abandoned due to the presence
of other DEQUEUE_SAVE/ENQUEUE_RESTORE pairs in the code. Ultimately, a
simpler approach was chosen, which achieves the same practical effects
without the need to create an additional flag for enqueue_task.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 kernel/sched/deadline.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index f59e5c19d944..312e8fa7ce94 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -1753,6 +1753,7 @@ enqueue_dl_entity(struct sched_dl_entity *dl_se, int flags)
 	} else if (flags & ENQUEUE_REPLENISH) {
 		replenish_dl_entity(dl_se);
 	} else if ((flags & ENQUEUE_RESTORE) &&
+		   !is_dl_boosted(dl_se) &&
 		   dl_time_before(dl_se->deadline, rq_clock(rq_of_dl_se(dl_se)))) {
 		setup_new_dl_entity(dl_se);
 	}
-- 
2.45.2

Re: [PATCH 1/3] sched/deadline: Fix warning in migrate_enable for boosted tasks

[Juri Lelli]

Hi Wander,

On 22/07/24 10:29, Wander Lairson Costa wrote:
> When running the following command:
> 
> while true; do
>     stress-ng --cyclic 30 --timeout 30s --minimize --quiet
> done
> 
> a warning is eventually triggered:
> 
> WARNING: CPU: 43 PID: 2848 at kernel/sched/deadline.c:794
> setup_new_dl_entity+0x13e/0x180
> ...
> Call Trace:
>  <TASK>
>  ? show_trace_log_lvl+0x1c4/0x2df
>  ? enqueue_dl_entity+0x631/0x6e0
>  ? setup_new_dl_entity+0x13e/0x180
>  ? __warn+0x7e/0xd0
>  ? report_bug+0x11a/0x1a0
>  ? handle_bug+0x3c/0x70
>  ? exc_invalid_op+0x14/0x70
>  ? asm_exc_invalid_op+0x16/0x20
>  enqueue_dl_entity+0x631/0x6e0
>  enqueue_task_dl+0x7d/0x120
>  __do_set_cpus_allowed+0xe3/0x280
>  __set_cpus_allowed_ptr_locked+0x140/0x1d0
>  __set_cpus_allowed_ptr+0x54/0xa0
>  migrate_enable+0x7e/0x150
>  rt_spin_unlock+0x1c/0x90
>  group_send_sig_info+0xf7/0x1a0
>  ? kill_pid_info+0x1f/0x1d0
>  kill_pid_info+0x78/0x1d0
>  kill_proc_info+0x5b/0x110
>  __x64_sys_kill+0x93/0xc0
>  do_syscall_64+0x5c/0xf0
>  entry_SYSCALL_64_after_hwframe+0x6e/0x76
>  RIP: 0033:0x7f0dab31f92b
> 
> This warning occurs because set_cpus_allowed dequeues and enqueues tasks
> with the ENQUEUE_RESTORE flag set. If the task is boosted, the warning
> is triggered. A boosted task already had its parameters set by
> rt_mutex_setprio, and a new call to setup_new_dl_entity is unnecessary,
> hence the WARN_ON call.
> 
> Check if we are requeueing a boosted task and avoid calling
> setup_new_dl_entity if that's the case.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> Fixes: 2279f540ea7d ("sched/deadline: Fix priority inheritance with multiple scheduling classes")

I believe your fix makes sense to me. I only wonder if however it
actually fixes 295d6d5e37360 ("sched/deadline: Fix switching to
-deadline") instead of the change you reference above?

Thanks,
Juri

Re: [PATCH 1/3] sched/deadline: Fix warning in migrate_enable for boosted tasks

[Wander Lairson Costa]

On Tue, Jul 23, 2024 at 5:50 AM Juri Lelli <juri.lelli@redhat.com> wrote:
>
> Hi Wander,
>
> On 22/07/24 10:29, Wander Lairson Costa wrote:
> > When running the following command:
> >
> > while true; do
> >     stress-ng --cyclic 30 --timeout 30s --minimize --quiet
> > done
> >
> > a warning is eventually triggered:
> >
> > WARNING: CPU: 43 PID: 2848 at kernel/sched/deadline.c:794
> > setup_new_dl_entity+0x13e/0x180
> > ...
> > Call Trace:
> >  <TASK>
> >  ? show_trace_log_lvl+0x1c4/0x2df
> >  ? enqueue_dl_entity+0x631/0x6e0
> >  ? setup_new_dl_entity+0x13e/0x180
> >  ? __warn+0x7e/0xd0
> >  ? report_bug+0x11a/0x1a0
> >  ? handle_bug+0x3c/0x70
> >  ? exc_invalid_op+0x14/0x70
> >  ? asm_exc_invalid_op+0x16/0x20
> >  enqueue_dl_entity+0x631/0x6e0
> >  enqueue_task_dl+0x7d/0x120
> >  __do_set_cpus_allowed+0xe3/0x280
> >  __set_cpus_allowed_ptr_locked+0x140/0x1d0
> >  __set_cpus_allowed_ptr+0x54/0xa0
> >  migrate_enable+0x7e/0x150
> >  rt_spin_unlock+0x1c/0x90
> >  group_send_sig_info+0xf7/0x1a0
> >  ? kill_pid_info+0x1f/0x1d0
> >  kill_pid_info+0x78/0x1d0
> >  kill_proc_info+0x5b/0x110
> >  __x64_sys_kill+0x93/0xc0
> >  do_syscall_64+0x5c/0xf0
> >  entry_SYSCALL_64_after_hwframe+0x6e/0x76
> >  RIP: 0033:0x7f0dab31f92b
> >
> > This warning occurs because set_cpus_allowed dequeues and enqueues tasks
> > with the ENQUEUE_RESTORE flag set. If the task is boosted, the warning
> > is triggered. A boosted task already had its parameters set by
> > rt_mutex_setprio, and a new call to setup_new_dl_entity is unnecessary,
> > hence the WARN_ON call.
> >
> > Check if we are requeueing a boosted task and avoid calling
> > setup_new_dl_entity if that's the case.
> >
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > Fixes: 2279f540ea7d ("sched/deadline: Fix priority inheritance with multiple scheduling classes")
>
> I believe your fix makes sense to me. I only wonder if however it
> actually fixes 295d6d5e37360 ("sched/deadline: Fix switching to
> -deadline") instead of the change you reference above?
>

That makes more sense, thanks.

> Thanks,
> Juri
>

[PATCH 2/3] sched/deadline: avoid redundant check for boosted task

[Wander Lairson Costa]

enqueue_dl_entity only calls setup_new_dl_entity if the task is not
boosted, so the WARN_ON check is unnecessary.

Signed-off-by: Wander Lairson Costa <wander@redhat.com>
---
 kernel/sched/deadline.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 312e8fa7ce94..908d5ce79425 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -785,12 +785,11 @@ static inline void replenish_dl_new_period(struct sched_dl_entity *dl_se,
  * one, and to (try to!) reconcile itself with its own scheduling
  * parameters.
  */
-static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
+static inline void __setup_new_dl_entity(struct sched_dl_entity *dl_se)
 {
 	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
 	struct rq *rq = rq_of_dl_rq(dl_rq);
 
-	WARN_ON(is_dl_boosted(dl_se));
 	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 
 	/*
@@ -809,6 +808,12 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
 	replenish_dl_new_period(dl_se, rq);
 }
 
+static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
+{
+	WARN_ON(is_dl_boosted(dl_se));
+	__setup_new_dl_entity(dl_se);
+}
+
 /*
  * Pure Earliest Deadline First (EDF) scheduling does not deal with the
  * possibility of a entity lasting more than what it declared, and thus
@@ -1755,7 +1760,7 @@ enqueue_dl_entity(struct sched_dl_entity *dl_se, int flags)
 	} else if ((flags & ENQUEUE_RESTORE) &&
 		   !is_dl_boosted(dl_se) &&
 		   dl_time_before(dl_se->deadline, rq_clock(rq_of_dl_se(dl_se)))) {
-		setup_new_dl_entity(dl_se);
+		__setup_new_dl_entity(dl_se);
 	}
 
 	__enqueue_dl_entity(dl_se);
-- 
2.45.2

Re: [PATCH 2/3] sched/deadline: avoid redundant check for boosted task

[Juri Lelli]

Hi Wander,

On 22/07/24 10:29, Wander Lairson Costa wrote:
> enqueue_dl_entity only calls setup_new_dl_entity if the task is not
> boosted, so the WARN_ON check is unnecessary.
> 
> Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> ---
>  kernel/sched/deadline.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 312e8fa7ce94..908d5ce79425 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -785,12 +785,11 @@ static inline void replenish_dl_new_period(struct sched_dl_entity *dl_se,
>   * one, and to (try to!) reconcile itself with its own scheduling
>   * parameters.
>   */
> -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> +static inline void __setup_new_dl_entity(struct sched_dl_entity *dl_se)
>  {
>  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
>  	struct rq *rq = rq_of_dl_rq(dl_rq);
>  
> -	WARN_ON(is_dl_boosted(dl_se));
>  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
>  
>  	/*
> @@ -809,6 +808,12 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
>  	replenish_dl_new_period(dl_se, rq);
>  }
>  
> +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> +{
> +	WARN_ON(is_dl_boosted(dl_se));
> +	__setup_new_dl_entity(dl_se);
> +}
> +

So, the other call path is from dl_server_start() and for this we know
the entity is not boosted either. We could probably just remove the
WARN_ON w/o the additional wrapper function. That said, considering it's
not fast path, I wonder if we actually want to leave the WARN_ON where
it is, so that we can catch potential future erroneous usages?

Thanks,
Juri

Re: [PATCH 2/3] sched/deadline: avoid redundant check for boosted task

[Wander Lairson Costa]

On Tue, Jul 23, 2024 at 5:55 AM Juri Lelli <juri.lelli@redhat.com> wrote:
>
> Hi Wander,
>
> On 22/07/24 10:29, Wander Lairson Costa wrote:
> > enqueue_dl_entity only calls setup_new_dl_entity if the task is not
> > boosted, so the WARN_ON check is unnecessary.
> >
> > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > ---
> >  kernel/sched/deadline.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > index 312e8fa7ce94..908d5ce79425 100644
> > --- a/kernel/sched/deadline.c
> > +++ b/kernel/sched/deadline.c
> > @@ -785,12 +785,11 @@ static inline void replenish_dl_new_period(struct sched_dl_entity *dl_se,
> >   * one, and to (try to!) reconcile itself with its own scheduling
> >   * parameters.
> >   */
> > -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > +static inline void __setup_new_dl_entity(struct sched_dl_entity *dl_se)
> >  {
> >       struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> >       struct rq *rq = rq_of_dl_rq(dl_rq);
> >
> > -     WARN_ON(is_dl_boosted(dl_se));
> >       WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
> >
> >       /*
> > @@ -809,6 +808,12 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> >       replenish_dl_new_period(dl_se, rq);
> >  }
> >
> > +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > +{
> > +     WARN_ON(is_dl_boosted(dl_se));
> > +     __setup_new_dl_entity(dl_se);
> > +}
> > +
>
> So, the other call path is from dl_server_start() and for this we know
> the entity is not boosted either. We could probably just remove the
> WARN_ON w/o the additional wrapper function. That said, considering it's
> not fast path, I wonder if we actually want to leave the WARN_ON where
> it is, so that we can catch potential future erroneous usages?
>

Yeah, if you feel the patch is not worth it, I am more in favor of
dropping the patch than removing the WARN_ON.

> Thanks,
> Juri
>

Re: [PATCH 2/3] sched/deadline: avoid redundant check for boosted task

[Juri Lelli]

On 23/07/24 09:27, Wander Lairson Costa wrote:
> On Tue, Jul 23, 2024 at 5:55 AM Juri Lelli <juri.lelli@redhat.com> wrote:
> >
> > Hi Wander,
> >
> > On 22/07/24 10:29, Wander Lairson Costa wrote:
> > > enqueue_dl_entity only calls setup_new_dl_entity if the task is not
> > > boosted, so the WARN_ON check is unnecessary.
> > >
> > > Signed-off-by: Wander Lairson Costa <wander@redhat.com>
> > > ---
> > >  kernel/sched/deadline.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> > > index 312e8fa7ce94..908d5ce79425 100644
> > > --- a/kernel/sched/deadline.c
> > > +++ b/kernel/sched/deadline.c
> > > @@ -785,12 +785,11 @@ static inline void replenish_dl_new_period(struct sched_dl_entity *dl_se,
> > >   * one, and to (try to!) reconcile itself with its own scheduling
> > >   * parameters.
> > >   */
> > > -static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > > +static inline void __setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > >  {
> > >       struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> > >       struct rq *rq = rq_of_dl_rq(dl_rq);
> > >
> > > -     WARN_ON(is_dl_boosted(dl_se));
> > >       WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
> > >
> > >       /*
> > > @@ -809,6 +808,12 @@ static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > >       replenish_dl_new_period(dl_se, rq);
> > >  }
> > >
> > > +static inline void setup_new_dl_entity(struct sched_dl_entity *dl_se)
> > > +{
> > > +     WARN_ON(is_dl_boosted(dl_se));
> > > +     __setup_new_dl_entity(dl_se);
> > > +}
> > > +
> >
> > So, the other call path is from dl_server_start() and for this we know
> > the entity is not boosted either. We could probably just remove the
> > WARN_ON w/o the additional wrapper function. That said, considering it's
> > not fast path, I wonder if we actually want to leave the WARN_ON where
> > it is, so that we can catch potential future erroneous usages?
> >
> 
> Yeah, if you feel the patch is not worth it, I am more in favor of
> dropping the patch than removing the WARN_ON.

Think we can drop it yes.

Thanks,
Juri

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

[Wander Lairson Costa]

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)
+{
+	cancel_timer(dl_se, &dl_se->dl_timer);
+}
+
+static __always_inline
+void cancel_inactive_timer(struct sched_dl_entity *dl_se)
+{
+	cancel_timer(dl_se, &dl_se->inactive_timer);
+}
+
 static void dl_change_utilization(struct task_struct *p, u64 new_bw)
 {
 	struct rq *rq;
@@ -340,8 +363,7 @@ static void dl_change_utilization(struct task_struct *p, u64 new_bw)
 		 * will not touch the rq's active utilization,
 		 * so we are still safe.
 		 */
-		if (hrtimer_try_to_cancel(&p->dl.inactive_timer) == 1)
-			put_task_struct(p);
+		cancel_inactive_timer(&p->dl);
 	}
 	__sub_rq_bw(p->dl.dl_bw, &rq->dl);
 	__add_rq_bw(new_bw, &rq->dl);
@@ -490,10 +512,7 @@ static void task_contending(struct sched_dl_entity *dl_se, int flags)
 		 * will not touch the rq's active utilization,
 		 * so we are still safe.
 		 */
-		if (hrtimer_try_to_cancel(&dl_se->inactive_timer) == 1) {
-			if (!dl_server(dl_se))
-				put_task_struct(dl_task_of(dl_se));
-		}
+		cancel_inactive_timer(dl_se);
 	} else {
 		/*
 		 * Since "dl_non_contending" is not set, the
@@ -1810,13 +1829,8 @@ static void enqueue_task_dl(struct rq *rq, struct task_struct *p, int flags)
 			 * The replenish timer needs to be canceled. No
 			 * problem if it fires concurrently: boosted threads
 			 * are ignored in dl_task_timer().
-			 *
-			 * If the timer callback was running (hrtimer_try_to_cancel == -1),
-			 * it will eventually call put_task_struct().
 			 */
-			if (hrtimer_try_to_cancel(&p->dl.dl_timer) == 1 &&
-			    !dl_server(&p->dl))
-				put_task_struct(p);
+			cancel_dl_timer(&p->dl);
 			p->dl.dl_throttled = 0;
 		}
 	} else if (!dl_prio(p->normal_prio)) {
@@ -1981,8 +1995,7 @@ static void migrate_task_rq_dl(struct task_struct *p, int new_cpu __maybe_unused
 		 * will not touch the rq's active utilization,
 		 * so we are still safe.
 		 */
-		if (hrtimer_try_to_cancel(&p->dl.inactive_timer) == 1)
-			put_task_struct(p);
+		cancel_inactive_timer(&p->dl);
 	}
 	sub_rq_bw(&p->dl, &rq->dl);
 	rq_unlock(rq, &rf);
@@ -2737,8 +2750,7 @@ static void switched_from_dl(struct rq *rq, struct task_struct *p)
  */
 static void switched_to_dl(struct rq *rq, struct task_struct *p)
 {
-	if (hrtimer_try_to_cancel(&p->dl.inactive_timer) == 1)
-		put_task_struct(p);
+	cancel_inactive_timer(&p->dl);
 
 	/*
 	 * In case a task is setscheduled to SCHED_DEADLINE we need to keep
-- 
2.45.2

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

[Juri Lelli]

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

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

That makes sense, I will change it in v2.

> Best,
> Juri
>