public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/2] sched/deadline: fixes and improvements
@ 2024-07-24 14:22 Wander Lairson Costa
  2024-07-24 14:22 ` [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks Wander Lairson Costa
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Wander Lairson Costa @ 2024-07-24 14:22 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER
  Cc: 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: Consolidate Timer Cancellation

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

Changelog
---------

v2:
* Drop patch to remove the redundant WARN_ON call.
* Change the "Fixes" tag in the patch 1.
* Change function names in the patch 2.

Wander Lairson Costa (2):
  sched/deadline: Fix warning in migrate_enable for boosted tasks
  sched/deadline: Consolidate Timer Cancellation

 kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 16 deletions(-)

-- 
2.45.2


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks
  2024-07-24 14:22 [PATCH v2 0/2] sched/deadline: fixes and improvements Wander Lairson Costa
@ 2024-07-24 14:22 ` Wander Lairson Costa
  2024-12-02 11:14   ` [tip: sched/core] " tip-bot2 for Wander Lairson Costa
  2024-07-24 14:22 ` [PATCH v2 2/2] sched/deadline: Consolidate Timer Cancellation Wander Lairson Costa
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Wander Lairson Costa @ 2024-07-24 14:22 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER
  Cc: 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: 295d6d5e3736 ("sched/deadline: Fix switching to -deadline")

---

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] sched/deadline: Consolidate Timer Cancellation
  2024-07-24 14:22 [PATCH v2 0/2] sched/deadline: fixes and improvements Wander Lairson Costa
  2024-07-24 14:22 ` [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks Wander Lairson Costa
@ 2024-07-24 14:22 ` Wander Lairson Costa
  2024-12-02 11:14   ` [tip: sched/core] " tip-bot2 for Wander Lairson Costa
  2024-07-25  7:29 ` [PATCH v2 0/2] sched/deadline: fixes and improvements Juri Lelli
  2024-09-23 16:45 ` Wander Lairson Costa
  3 siblings, 1 reply; 10+ messages in thread
From: Wander Lairson Costa @ 2024-07-24 14:22 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER
  Cc: 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 312e8fa7ce94..b509ac4f3f6d 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_dl_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_replenish_timer(struct sched_dl_entity *dl_se)
+{
+	cancel_dl_timer(dl_se, &dl_se->dl_timer);
+}
+
+static __always_inline
+void cancel_inactive_timer(struct sched_dl_entity *dl_se)
+{
+	cancel_dl_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
@@ -1805,13 +1824,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_replenish_timer(&p->dl);
 			p->dl.dl_throttled = 0;
 		}
 	} else if (!dl_prio(p->normal_prio)) {
@@ -1976,8 +1990,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);
@@ -2732,8 +2745,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] sched/deadline: fixes and improvements
  2024-07-24 14:22 [PATCH v2 0/2] sched/deadline: fixes and improvements Wander Lairson Costa
  2024-07-24 14:22 ` [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks Wander Lairson Costa
  2024-07-24 14:22 ` [PATCH v2 2/2] sched/deadline: Consolidate Timer Cancellation Wander Lairson Costa
@ 2024-07-25  7:29 ` Juri Lelli
  2024-08-07 10:59   ` Peter Zijlstra
  2024-09-23 16:45 ` Wander Lairson Costa
  3 siblings, 1 reply; 10+ messages in thread
From: Juri Lelli @ 2024-07-25  7:29 UTC (permalink / raw)
  To: Wander Lairson Costa
  Cc: Ingo Molnar, Peter Zijlstra, Vincent Guittot, Dietmar Eggemann,
	Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider,
	open list:SCHEDULER

Hi Wander,

On 24/07/24 11:22, Wander Lairson Costa wrote:
> 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: Consolidate Timer Cancellation
> 
> Consolidate timer cancellation logic into dedicated functions,
> ensuring consistent behavior and reducing code duplication.
> 
> Changelog
> ---------
> 
> v2:
> * Drop patch to remove the redundant WARN_ON call.
> * Change the "Fixes" tag in the patch 1.
> * Change function names in the patch 2.
> 
> Wander Lairson Costa (2):
>   sched/deadline: Fix warning in migrate_enable for boosted tasks
>   sched/deadline: Consolidate Timer Cancellation
> 
>  kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 16 deletions(-)

Looks good to me now.

Acked-by: Juri Lelli <juri.lelli@redhat.com>

Thanks!
Juri


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] sched/deadline: fixes and improvements
  2024-07-25  7:29 ` [PATCH v2 0/2] sched/deadline: fixes and improvements Juri Lelli
@ 2024-08-07 10:59   ` Peter Zijlstra
  2024-08-07 11:58     ` Wander Lairson Costa
  0 siblings, 1 reply; 10+ messages in thread
From: Peter Zijlstra @ 2024-08-07 10:59 UTC (permalink / raw)
  To: Juri Lelli
  Cc: Wander Lairson Costa, Ingo Molnar, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER

On Thu, Jul 25, 2024 at 09:29:26AM +0200, Juri Lelli wrote:
> Hi Wander,
> 
> On 24/07/24 11:22, Wander Lairson Costa wrote:
> > 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: Consolidate Timer Cancellation
> > 
> > Consolidate timer cancellation logic into dedicated functions,
> > ensuring consistent behavior and reducing code duplication.
> > 
> > Changelog
> > ---------
> > 
> > v2:
> > * Drop patch to remove the redundant WARN_ON call.
> > * Change the "Fixes" tag in the patch 1.
> > * Change function names in the patch 2.
> > 
> > Wander Lairson Costa (2):
> >   sched/deadline: Fix warning in migrate_enable for boosted tasks
> >   sched/deadline: Consolidate Timer Cancellation
> > 
> >  kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 16 deletions(-)
> 
> Looks good to me now.
> 
> Acked-by: Juri Lelli <juri.lelli@redhat.com>

I stuck this in queue/sched/core, but there was some trivial rejects on
the second patch, which I stomped on. It builds, but please double check
I didn't mess it up.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] sched/deadline: fixes and improvements
  2024-08-07 10:59   ` Peter Zijlstra
@ 2024-08-07 11:58     ` Wander Lairson Costa
  0 siblings, 0 replies; 10+ messages in thread
From: Wander Lairson Costa @ 2024-08-07 11:58 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Juri Lelli, Ingo Molnar, Vincent Guittot, Dietmar Eggemann,
	Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider,
	open list:SCHEDULER

On Wed, Aug 7, 2024 at 7:59 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Thu, Jul 25, 2024 at 09:29:26AM +0200, Juri Lelli wrote:
> > Hi Wander,
> >
> > On 24/07/24 11:22, Wander Lairson Costa wrote:
> > > 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: Consolidate Timer Cancellation
> > >
> > > Consolidate timer cancellation logic into dedicated functions,
> > > ensuring consistent behavior and reducing code duplication.
> > >
> > > Changelog
> > > ---------
> > >
> > > v2:
> > > * Drop patch to remove the redundant WARN_ON call.
> > > * Change the "Fixes" tag in the patch 1.
> > > * Change function names in the patch 2.
> > >
> > > Wander Lairson Costa (2):
> > >   sched/deadline: Fix warning in migrate_enable for boosted tasks
> > >   sched/deadline: Consolidate Timer Cancellation
> > >
> > >  kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
> > >  1 file changed, 29 insertions(+), 16 deletions(-)
> >
> > Looks good to me now.
> >
> > Acked-by: Juri Lelli <juri.lelli@redhat.com>
>
> I stuck this in queue/sched/core, but there was some trivial rejects on
> the second patch, which I stomped on. It builds, but please double check
> I didn't mess it up.
>

I gave it a look and everything seems fine.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] sched/deadline: fixes and improvements
  2024-07-24 14:22 [PATCH v2 0/2] sched/deadline: fixes and improvements Wander Lairson Costa
                   ` (2 preceding siblings ...)
  2024-07-25  7:29 ` [PATCH v2 0/2] sched/deadline: fixes and improvements Juri Lelli
@ 2024-09-23 16:45 ` Wander Lairson Costa
  2024-09-25 15:29   ` Phil Auld
  3 siblings, 1 reply; 10+ messages in thread
From: Wander Lairson Costa @ 2024-09-23 16:45 UTC (permalink / raw)
  To: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER

On Wed, Jul 24, 2024 at 11:23 AM Wander Lairson Costa <wander@redhat.com> wrote:
>
> 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: Consolidate Timer Cancellation
>
> Consolidate timer cancellation logic into dedicated functions,
> ensuring consistent behavior and reducing code duplication.
>
> Changelog
> ---------
>
> v2:
> * Drop patch to remove the redundant WARN_ON call.
> * Change the "Fixes" tag in the patch 1.
> * Change function names in the patch 2.
>
> Wander Lairson Costa (2):
>   sched/deadline: Fix warning in migrate_enable for boosted tasks
>   sched/deadline: Consolidate Timer Cancellation
>
>  kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 16 deletions(-)
>
> --
> 2.45.2
>

Notice there was a PR from sched a few days ago, but this patchset was
not part of it. Do you know if this will be added later in this
window?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 0/2] sched/deadline: fixes and improvements
  2024-09-23 16:45 ` Wander Lairson Costa
@ 2024-09-25 15:29   ` Phil Auld
  0 siblings, 0 replies; 10+ messages in thread
From: Phil Auld @ 2024-09-25 15:29 UTC (permalink / raw)
  To: Wander Lairson Costa
  Cc: Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot,
	Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman,
	Valentin Schneider, open list:SCHEDULER

On Mon, Sep 23, 2024 at 01:45:13PM -0300 Wander Lairson Costa wrote:
> On Wed, Jul 24, 2024 at 11:23 AM Wander Lairson Costa <wander@redhat.com> wrote:
> >
> > 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: Consolidate Timer Cancellation
> >
> > Consolidate timer cancellation logic into dedicated functions,
> > ensuring consistent behavior and reducing code duplication.
> >
> > Changelog
> > ---------
> >
> > v2:
> > * Drop patch to remove the redundant WARN_ON call.
> > * Change the "Fixes" tag in the patch 1.
> > * Change function names in the patch 2.
> >
> > Wander Lairson Costa (2):
> >   sched/deadline: Fix warning in migrate_enable for boosted tasks
> >   sched/deadline: Consolidate Timer Cancellation
> >
> >  kernel/sched/deadline.c | 45 ++++++++++++++++++++++++++---------------
> >  1 file changed, 29 insertions(+), 16 deletions(-)
> >
> > --
> > 2.45.2
> >
> 
> Notice there was a PR from sched a few days ago, but this patchset was
> not part of it. Do you know if this will be added later in this
> window?
>

Unless Peter takes this in sched/urgent then it will likely be in v6.13
assuming it gets picked up.  Ping Peter?


Cheers,
Phil



-- 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [tip: sched/core] sched/deadline: Consolidate Timer Cancellation
  2024-07-24 14:22 ` [PATCH v2 2/2] sched/deadline: Consolidate Timer Cancellation Wander Lairson Costa
@ 2024-12-02 11:14   ` tip-bot2 for Wander Lairson Costa
  0 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Wander Lairson Costa @ 2024-12-02 11:14 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Wander Lairson Costa, Peter Zijlstra (Intel), Juri Lelli, x86,
	linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     3a181f20fb4e9ad3c93ea6c71520c23826042629
Gitweb:        https://git.kernel.org/tip/3a181f20fb4e9ad3c93ea6c71520c23826042629
Author:        Wander Lairson Costa <wander@redhat.com>
AuthorDate:    Wed, 24 Jul 2024 11:22:48 -03:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 02 Dec 2024 12:01:31 +01:00

sched/deadline: Consolidate Timer Cancellation

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>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Juri Lelli <juri.lelli@redhat.com>
Link: https://lore.kernel.org/r/20240724142253.27145-3-wander@redhat.com
---
 kernel/sched/deadline.c | 41 ++++++++++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index 1c8b838..33b4646 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -342,6 +342,29 @@ static void dl_rq_change_utilization(struct rq *rq, struct sched_dl_entity *dl_s
 	__add_rq_bw(new_bw, &rq->dl);
 }
 
+static __always_inline
+void cancel_dl_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_replenish_timer(struct sched_dl_entity *dl_se)
+{
+	cancel_dl_timer(dl_se, &dl_se->dl_timer);
+}
+
+static __always_inline
+void cancel_inactive_timer(struct sched_dl_entity *dl_se)
+{
+	cancel_dl_timer(dl_se, &dl_se->inactive_timer);
+}
+
 static void dl_change_utilization(struct task_struct *p, u64 new_bw)
 {
 	WARN_ON_ONCE(p->dl.flags & SCHED_FLAG_SUGOV);
@@ -495,10 +518,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
@@ -2113,13 +2133,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_replenish_timer(&p->dl);
 			p->dl.dl_throttled = 0;
 		}
 	} else if (!dl_prio(p->normal_prio)) {
@@ -2287,8 +2302,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);
@@ -3036,8 +3050,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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [tip: sched/core] sched/deadline: Fix warning in migrate_enable for boosted tasks
  2024-07-24 14:22 ` [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks Wander Lairson Costa
@ 2024-12-02 11:14   ` tip-bot2 for Wander Lairson Costa
  0 siblings, 0 replies; 10+ messages in thread
From: tip-bot2 for Wander Lairson Costa @ 2024-12-02 11:14 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Wander Lairson Costa, Peter Zijlstra (Intel), Juri Lelli, x86,
	linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     0664e2c311b9fa43b33e3e81429cd0c2d7f9c638
Gitweb:        https://git.kernel.org/tip/0664e2c311b9fa43b33e3e81429cd0c2d7f9c638
Author:        Wander Lairson Costa <wander@redhat.com>
AuthorDate:    Wed, 24 Jul 2024 11:22:47 -03:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 02 Dec 2024 12:01:29 +01:00

sched/deadline: Fix warning in migrate_enable for boosted tasks

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.

Fixes: 295d6d5e3736 ("sched/deadline: Fix switching to -deadline")
Signed-off-by: Wander Lairson Costa <wander@redhat.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Juri Lelli <juri.lelli@redhat.com>
Link: https://lore.kernel.org/r/20240724142253.27145-2-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 206691d..db47f33 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2042,6 +2042,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);
 	}

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-12-02 11:14 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-24 14:22 [PATCH v2 0/2] sched/deadline: fixes and improvements Wander Lairson Costa
2024-07-24 14:22 ` [PATCH v2 1/2] sched/deadline: Fix warning in migrate_enable for boosted tasks Wander Lairson Costa
2024-12-02 11:14   ` [tip: sched/core] " tip-bot2 for Wander Lairson Costa
2024-07-24 14:22 ` [PATCH v2 2/2] sched/deadline: Consolidate Timer Cancellation Wander Lairson Costa
2024-12-02 11:14   ` [tip: sched/core] " tip-bot2 for Wander Lairson Costa
2024-07-25  7:29 ` [PATCH v2 0/2] sched/deadline: fixes and improvements Juri Lelli
2024-08-07 10:59   ` Peter Zijlstra
2024-08-07 11:58     ` Wander Lairson Costa
2024-09-23 16:45 ` Wander Lairson Costa
2024-09-25 15:29   ` Phil Auld

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox