sched: Cleanup the change-pattern and related locking

As proposed a long while ago -- and half done by scx -- wrap the scheduler's 'change' pattern in a guard helper.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

---

kernel/sched/sched.h +10 -5

+struct sched_change_ctx {
 	struct task_struct	*p;
-	int			queue_flags;
+	int			flags;
 	bool			queued;
 	bool			running;
 };
 
-void sched_deq_and_put_task(struct task_struct *p, int queue_flags,
-			    struct sched_enq_and_set_ctx *ctx);
-void sched_enq_and_set_task(struct sched_enq_and_set_ctx *ctx);
+struct sched_change_ctx *sched_change_begin(struct task_struct *p, unsigned int flags);
+void sched_change_end(struct sched_change_ctx *ctx);
 
-#endif /* CONFIG_SCHED_CLASS_EXT */
+DEFINE_CLASS(sched_change, struct sched_change_ctx *,
+	     sched_change_end(_T),
+	     sched_change_begin(p, flags),
+	     struct task_struct *p, unsigned int flags)
+
+DEFINE_CLASS_IS_UNCONDITIONAL(sched_change)
 
 #include "ext.h"
 

kernel/sched/syscalls.c +19 -40

@@ -64,7 +64,6 @@ static int effective_prio(struct task_st
 
 void set_user_nice(struct task_struct *p, long nice)
 {
-	bool queued, running;
 	struct rq *rq;
 	int old_prio;
 

@@ -90,22 +89,12 @@ void set_user_nice(struct task_struct *p
 		return;
 	}
 
-	queued = task_on_rq_queued(p);
-	running = task_current_donor(rq, p);
-	if (queued)
-		dequeue_task(rq, p, DEQUEUE_SAVE | DEQUEUE_NOCLOCK);
-	if (running)
-		put_prev_task(rq, p);
-
-	p->static_prio = NICE_TO_PRIO(nice);
-	set_load_weight(p, true);
-	old_prio = p->prio;
-	p->prio = effective_prio(p);
-
-	if (queued)
-		enqueue_task(rq, p, ENQUEUE_RESTORE | ENQUEUE_NOCLOCK);
-	if (running)
-		set_next_task(rq, p);
+	scoped_guard (sched_change, p, DEQUEUE_SAVE | DEQUEUE_NOCLOCK) {
+		p->static_prio = NICE_TO_PRIO(nice);
+		set_load_weight(p, true);
+		old_prio = p->prio;
+		p->prio = effective_prio(p);
+	}
 
 	/*
 	 * If the task increased its priority or is running and

@@ -515,7 +504,7 @@ int __sched_setscheduler(struct task_str
 			 bool user, bool pi)
 {
 	int oldpolicy = -1, policy = attr->sched_policy;
-	int retval, oldprio, newprio, queued, running;
+	int retval, oldprio, newprio;
 	const struct sched_class *prev_class, *next_class;
 	struct balance_callback *head;
 	struct rq_flags rf;

@@ -698,33 +687,25 @@ int __sched_setscheduler(struct task_str
 	if (prev_class != next_class && p->se.sched_delayed)
 		dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
 
-	queued = task_on_rq_queued(p);
-	running = task_current_donor(rq, p);
-	if (queued)
-		dequeue_task(rq, p, queue_flags);
-	if (running)
-		put_prev_task(rq, p);
-
-	if (!(attr->sched_flags & SCHED_FLAG_KEEP_PARAMS)) {
-		__setscheduler_params(p, attr);
-		p->sched_class = next_class;
-		p->prio = newprio;
-	}
-	__setscheduler_uclamp(p, attr);
-	check_class_changing(rq, p, prev_class);
+	scoped_guard (sched_change, p, queue_flags) {
 
-	if (queued) {
-		/*
-		 * We enqueue to tail when the priority of a task is
-		 * increased (user space view).
-		 */
-		if (oldprio < p->prio)
-			queue_flags |= ENQUEUE_HEAD;
+		if (!(attr->sched_flags & SCHED_FLAG_KEEP_PARAMS)) {
+			__setscheduler_params(p, attr);
+			p->sched_class = next_class;
+			p->prio = newprio;
+		}
+		__setscheduler_uclamp(p, attr);
+		check_class_changing(rq, p, prev_class);
 
-		enqueue_task(rq, p, queue_flags);
+		if (scope->queued) {
+			/*
+			 * We enqueue to tail when the priority of a task is
+			 * increased (user space view).

Hi Peter,

Quoted 6 lines · Peter Zijlstra

As proposed a long while ago -- and half done by scx -- wrap the scheduler's 'change' pattern in a guard helper.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

---

...

Quoted 6 lines

kernel/sched/sched.h +0 -0

@@ -3885,23 +3885,22 @@ extern void check_class_changed(struct r
 extern struct balance_callback *splice_balance_callbacks(struct rq *rq);
 extern void balance_callbacks(struct rq *rq, struct balance_callback *head);
 

kernel/sched/sched.h extern void check_class_changed(struct r · line 3893

Quoted 6 lines

kernel/sched/sched.h +0 -6

-#ifdef CONFIG_SCHED_CLASS_EXT
-/*
- * Used by SCX in the enable/disable paths to move tasks between sched_classes
- * and establish invariants.
- */
-struct sched_enq_and_set_ctx {

Not necessarily for this patch, we can add it later, but I kinda liked the comment that briefly explained how the context is used. Maybe having something along these lines could be helpful?

Quoted 16 lines · Andrea Righi

Hi Peter,

Quoted 6 lines · Peter Zijlstra

As proposed a long while ago -- and half done by scx -- wrap the scheduler's 'change' pattern in a guard helper.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

---

...

Quoted 6 lines

kernel/sched/sched.h +0 -0

@@ -3885,23 +3885,22 @@ extern void check_class_changed(struct r
 extern struct balance_callback *splice_balance_callbacks(struct rq *rq);
 extern void balance_callbacks(struct rq *rq, struct balance_callback *head);
 

Quoted 10 lines

kernel/sched/sched.h extern void check_class_changed(struct r · line 3893

Quoted 6 lines

kernel/sched/sched.h +0 -6

-#ifdef CONFIG_SCHED_CLASS_EXT
-/*
- * Used by SCX in the enable/disable paths to move tasks between sched_classes
- * and establish invariants.
- */
-struct sched_enq_and_set_ctx {

Not necessarily for this patch, we can add it later, but I kinda liked the comment that briefly explained how the context is used. Maybe having something along these lines could be helpful?

I have changed it thus:

kernel/sched/sched.h +6 -0

@@ -3885,6 +3885,22 @@ extern void check_class_changed(struct r
 extern struct balance_callback *splice_balance_callbacks(struct rq *rq);
 extern void balance_callbacks(struct rq *rq, struct balance_callback *head);
 
+/*
+ * The 'sched_change' pattern is the safe, easy and slow way of changing a
+ * task's scheduling properties. It dequeues a task, such that the scheduler
+ * is fully unaware of it; at which point its properties can be modified;
+ * after which it is enqueued again.
+ *

Quoted 4 lines · Peter Zijlstra

kernel/sched/core.c +0 -0

@@ -7391,52 +7391,42 @@ void rt_mutex_setprio(struct task_struct
      if (prev_class != next_class && p->se.sched_delayed)
              dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
 

Review void rt_mutex_setprio(struct task_struct · line 7395

Quoted 2 lines

(quoted diff) +0 -2

-	queued = task_on_rq_queued(p);
-	running = task_current_donor(rq, p);

I'm not sure how that plays with sched_ext, but for the "standard" change pattern such as this one & the others in core.c, that becomes a task_current() per sched_change_begin(). I'm guessing we want to make sched_change_begin() use task_current_donor() instead?

Add {DE,EN}QUEUE_CLASS and fold the sched_class::switch* methods into the change pattern. This completes and makes the pattern more symmetric.

This changes the order of callbacks slightly:

				|
				|  switching_from()
  dequeue_task();		|  dequeue_task()
  put_prev_task();		|  put_prev_task()
				|  switched_from()
				|
  ... change task ...		|  ... change task ...
				|
  switching_to();		|  switching_to()
  enqueue_task();		|  enqueue_task()
  set_next_task();		|  set_next_task()
  prev_class->switched_from()	|
  switched_to()			|  switched_to()
				|

Notably, it moves the switched_from() callback right after the dequeue/put. Existing implementations don't appear to be affected by this change in location -- specifically the task isn't enqueued on the class in question in either location.

Make (CLASS)^(SAVE|MOVE), because there is nothing to save-restore when changing scheduling classes.

Quoted 26 lines · Peter Zijlstra

Add {DE,EN}QUEUE_CLASS and fold the sched_class::switch* methods into the change pattern. This completes and makes the pattern more symmetric.

This changes the order of callbacks slightly:

				|
				|  switching_from()
  dequeue_task();		|  dequeue_task()
  put_prev_task();		|  put_prev_task()
				|  switched_from()
				|
  ... change task ...		|  ... change task ...
				|
  switching_to();		|  switching_to()
  enqueue_task();		|  enqueue_task()
  set_next_task();		|  set_next_task()
  prev_class->switched_from()	|
  switched_to()			|  switched_to()
				|

Notably, it moves the switched_from() callback right after the dequeue/put. Existing implementations don't appear to be affected by this change in location -- specifically the task isn't enqueued on the class in question in either location.

Quoted 2 lines

Make (CLASS)^(SAVE|MOVE), because there is nothing to save-restore when changing scheduling classes.

This one causes a DL bw related warning when I run a simple 1 DL task rt-app workload:

Not sure yet how this is related to switched_from_dl() being now called earlier?

Quoted 32 lines · Dietmar Eggemann

Quoted 21 lines · Peter Zijlstra

Add {DE,EN}QUEUE_CLASS and fold the sched_class::switch* methods into the change pattern. This completes and makes the pattern more symmetric.

This changes the order of callbacks slightly:

				|
				|  switching_from()
  dequeue_task();		|  dequeue_task()
  put_prev_task();		|  put_prev_task()
				|  switched_from()
				|
  ... change task ...		|  ... change task ...
				|
  switching_to();		|  switching_to()
  enqueue_task();		|  enqueue_task()
  set_next_task();		|  set_next_task()
  prev_class->switched_from()	|
  switched_to()			|  switched_to()
				|

Quoted 7 lines

Notably, it moves the switched_from() callback right after the dequeue/put. Existing implementations don't appear to be affected by this change in location -- specifically the task isn't enqueued on the class in question in either location.

Make (CLASS)^(SAVE|MOVE), because there is nothing to save-restore when changing scheduling classes.

This one causes a DL bw related warning when I run a simple 1 DL task rt-app workload:

Quoted 1 line

Not sure yet how this is related to switched_from_dl() being now called earlier?

Ooh, I might see a problem. task_non_contending() uses dl_task(), which uses p->prio. The move above means it is now called using the 'old' prio, whereas it used to run with the 'new' prio.

Let me see if I can figure out something for this.

Quoted 36 lines · Peter Zijlstra

Quoted 34 lines · Dietmar Eggemann

Quoted 21 lines · Peter Zijlstra

Add {DE,EN}QUEUE_CLASS and fold the sched_class::switch* methods into the change pattern. This completes and makes the pattern more symmetric.

This changes the order of callbacks slightly:

				|
				|  switching_from()
  dequeue_task();		|  dequeue_task()
  put_prev_task();		|  put_prev_task()
				|  switched_from()
				|
  ... change task ...		|  ... change task ...
				|
  switching_to();		|  switching_to()
  enqueue_task();		|  enqueue_task()
  set_next_task();		|  set_next_task()
  prev_class->switched_from()	|
  switched_to()			|  switched_to()
				|

Quoted 7 lines

Notably, it moves the switched_from() callback right after the dequeue/put. Existing implementations don't appear to be affected by this change in location -- specifically the task isn't enqueued on the class in question in either location.

Make (CLASS)^(SAVE|MOVE), because there is nothing to save-restore when changing scheduling classes.

This one causes a DL bw related warning when I run a simple 1 DL task rt-app workload:

Not sure yet how this is related to switched_from_dl() being now called earlier?

Quoted 5 lines

Ooh, I might see a problem. task_non_contending() uses dl_task(), which uses p->prio. The move above means it is now called using the 'old' prio, whereas it used to run with the 'new' prio.

Let me see if I can figure out something for this.

Does this help? /me goes find rt-app.

Yes, but there seems to be more ... missing DEQUEUE_SAVE (a.k.a. ENQUEUE_RESTORE) in

enqueue_dl_entity()

    if (flags & (ENQUEUE_RESTORE|ENQUEUE_MIGRATING))
                 ^^^^^^^^^^^^^^^
      ...
      add_running_bw(dl_se, dl_rq)

and

__sched_setscheduler()

    ...
    if (prev_class != next_class)
      queue_flags |= DEQUEUE_CLASS;
      queue_flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
                       ^^^^^^^^^^^^

as well as

sched_change_begin()

    ...
    if (flags & DEQUEUE_CLASS) {
      if (WARN_ON_ONCE(flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)))
        flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
                   ^^^^^^^^^^^^

Use the new sched_class::switching_from() method to dequeue delayed tasks before switching to another class.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

---
 kernel/sched/core.c     |   12 ++++++++----
 kernel/sched/ext.c      |    6 ------
 kernel/sched/fair.c     |    7 +++++++
 kernel/sched/syscalls.c |    3 ---
 4 files changed, 15 insertions(+), 13 deletions(-)

kernel/sched/core.c +8 -4

@@ -7368,9 +7368,6 @@ void rt_mutex_setprio(struct task_struct
                queue_flag &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
        }
 
-       if (prev_class != next_class && p->se.sched_delayed)
-               dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
-
        scoped_guard (sched_change, p, queue_flag) {
                /*
                 * Boosting condition are:

@@ -10845,8 +10842,15 @@ struct sched_change_ctx *sched_change_be
                if (WARN_ON_ONCE(flags & (DEQUEUE_SAVE | DEQUEUE_MOVE)))
                        flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
 
-               if (p->sched_class->switching_from)
+               if (p->sched_class->switching_from) {
+                       /*
+                        * switching_from_fair() assumes CLASS implies NOCLOCK;
+                        * fixing this assumption would mean switching_from()
+                        * would need to be able to change flags.
+                        */
+                       WARN_ON(!(flags & DEQUEUE_NOCLOCK));
                        p->sched_class->switching_from(rq, p);
+               }
        }
 
        *ctx = (struct sched_change_ctx){

kernel/sched/ext.c +0 -6

@@ -3924,9 +3924,6 @@ static void scx_disable_workfn(struct kt
                        queue_flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
                }
 
-               if (old_class != new_class && p->se.sched_delayed)
-                       dequeue_task(task_rq(p), p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
-
                scoped_guard (sched_change, p, queue_flags) {
                        p->sched_class = new_class;
                }

@@ -4677,9 +4674,6 @@ static int scx_enable(struct sched_ext_o
                        queue_flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
                }
 
-               if (old_class != new_class && p->se.sched_delayed)
-                       dequeue_task(task_rq(p), p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
-
                scoped_guard (sched_change, p, queue_flags) {
                        p->scx.slice = SCX_SLICE_DFL;
                        p->sched_class = new_class;

kernel/sched/fair.c +7 -0

@@ -13237,6 +13237,12 @@ static void attach_task_cfs_rq(struct ta
        attach_entity_cfs_rq(se);
 }
 
+static void switching_from_fair(struct rq *rq, struct task_struct *p)
+{
+       if (p->se.sched_delayed)
+               dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
+}
+
 static void switched_from_fair(struct rq *rq, struct task_struct *p)
 {
        detach_task_cfs_rq(p);

@@ -13638,6 +13644,7 @@ DEFINE_SCHED_CLASS(fair) = {
 
        .reweight_task          = reweight_task_fair,
        .prio_changed           = prio_changed_fair,
+       .switching_from         = switching_from_fair,
        .switched_from          = switched_from_fair,
        .switched_to            = switched_to_fair,
 

kernel/sched/syscalls.c +0 -3

@@ -689,9 +689,6 @@ int __sched_setscheduler(struct task_str
                queue_flags &= ~(DEQUEUE_SAVE | DEQUEUE_MOVE);
        }
 
-       if (prev_class != next_class && p->se.sched_delayed)
-               dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED | DEQUEUE_NOCLOCK);
-
        scoped_guard (sched_change, p, queue_flags) {
 
                if (!(attr->sched_flags & SCHED_FLAG_KEEP_PARAMS)) {

Move sched_class::prio_changed() into the change pattern.

And while there, extend it with sched_class::get_prio() in order to fix the deadline sitation.

Suggested-by Tejun Heo <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Tejun Heo <[email protected]>

---
  kernel/sched/core.c      |   24 +++++++++++++-----------
  kernel/sched/deadline.c  |   20 +++++++++++---------
  kernel/sched/ext.c       |    8 +-------
  kernel/sched/fair.c      |    8 ++++++--
  kernel/sched/idle.c      |    5 ++++-
  kernel/sched/rt.c        |    5 ++++-
  kernel/sched/sched.h     |    7 ++++---
  kernel/sched/stop_task.c |    5 ++++-
  kernel/sched/syscalls.c  |    9 ---------
  9 files changed, 47 insertions(+), 44 deletions(-)

kernel/sched/core.c +0 -6

@@ -2169,12 +2169,6 @@ inline int task_curr(const struct task_s
  	return cpu_curr(task_cpu(p)) == p;
  }
 
-void check_prio_changed(struct rq *rq, struct task_struct *p, int oldprio)
-{
-	if (oldprio != p->prio || dl_task(p))
-		p->sched_class->prio_changed(rq, p, oldprio);
-}
-
  void wakeup_preempt(struct rq *rq, struct task_struct *p, int flags)
  {
  	struct task_struct *donor = rq->donor;

kernel/sched/core.c +7 -0

@@ -10860,6 +10851,13 @@ struct sched_change_ctx *sched_change_be
  		.running = task_current(rq, p),
  	};
 
+	if (!(flags & DEQUEUE_CLASS)) {
+		if (p->sched_class->get_prio)
+			ctx->prio = p->sched_class->get_prio(rq, p);
+		else
+			ctx->prio = p->prio;
+	}
+
  	if (ctx->queued)
  		dequeue_task(rq, p, flags);
  	if (ctx->running)

@@ -10886,6 +10884,10 @@ void sched_change_end(struct sched_chang
  	if (ctx->running)

Hello Peter,

It seems this patch:
6455ad5346c9 ("sched: Move sched_class::prio_changed() into the change pattern") is triggering the following warning:
rq_pin_lock()
\-WARN_ON_ONCE(rq->balance_callback && rq->balance_callback != &balance_push_callback);

Hello Pierre,

Quoted 2 lines · Pierre Gondois

Hello Peter,

Quoted 5 lines

It seems this patch:
6455ad5346c9 ("sched: Move sched_class::prio_changed() into the change pattern") is triggering the following warning:
rq_pin_lock()
\-WARN_ON_ONCE(rq->balance_callback && rq->balance_callback != &balance_push_callback);

Can you check if the following solution helps your case too:
https://lore.kernel.org/all/[email protected]/

Hello Prateek,

Quoted 5 lines · K Prateek Nayak

Hello Pierre,

Quoted 2 lines · Pierre Gondois

Hello Peter,

Quoted 8 lines

Quoted 5 lines

It seems this patch:
6455ad5346c9 ("sched: Move sched_class::prio_changed() into the change pattern") is triggering the following warning:
rq_pin_lock()
\-WARN_ON_ONCE(rq->balance_callback && rq->balance_callback != &balance_push_callback);

Can you check if the following solution helps your case too:
https://lore.kernel.org/all/[email protected]/

I can still see the issue.
It seems the task deadline is also updated in:
sched_change_end()
\-enqueue_task_dl()
  \-enqueue_dl_entity()
    \-setup_new_dl_entity()
      \-replenish_dl_new_period()
if the task's period finished.

So in sched_change_end(), the task priority (i.e. p->dl.deadline) is updated. This results in having an old_deadline earlier than the new p->dl.deadline. Thus the rq->balance_callback:

prio_changed_dl() {
...
if (dl_time_before(old_deadline, p->dl.deadline))
  deadline_queue_pull_task(rq);
...
}

Does enabling WARN_DOUBLE_CLOCK warn of a double clock update before hitting this warning?

Quoted 2 lines · K Prateek Nayak

Does enabling WARN_DOUBLE_CLOCK warn of a double clock update before hitting this warning?

setup_new_dl_entity() -> update_rq_clock() seems like it will trip that in this case.

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · K Prateek Nayak

Does enabling WARN_DOUBLE_CLOCK warn of a double clock update before hitting this warning?

setup_new_dl_entity() -> update_rq_clock() seems like it will trip that in this case.

Something like so to fix: 9f239df55546 ("sched/deadline: Initialize dl_servers after SMP")

kernel/sched/deadline.c +0 -2

@@ -752,8 +752,6 @@ static inline void setup_new_dl_entity(s
  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
  	struct rq *rq = rq_of_dl_rq(dl_rq);
 
-	update_rq_clock(rq);
-
  	WARN_ON(is_dl_boosted(dl_se));
  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 

Quoted 6 lines · Peter Zijlstra

Quoted 2 lines · K Prateek Nayak

Does enabling WARN_DOUBLE_CLOCK warn of a double clock update before hitting this warning?

setup_new_dl_entity() -> update_rq_clock() seems like it will trip that in this case.

Something like so to fix: 9f239df55546 ("sched/deadline: Initialize dl_servers after SMP")

kernel/sched/deadline.c +0 -2

@@ -752,8 +752,6 @@ static inline void setup_new_dl_entity(s
  	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
  	struct rq *rq = rq_of_dl_rq(dl_rq);
 
-	update_rq_clock(rq);
-
  	WARN_ON(is_dl_boosted(dl_se));
  	WARN_ON(dl_time_before(rq_clock(rq), dl_se->deadline));
 

Hello Prateek,

Quoted 5 lines · K Prateek Nayak

Hello Pierre,

Quoted 2 lines · Pierre Gondois

Hello Peter,

Quoted 8 lines

Quoted 5 lines

It seems this patch:
6455ad5346c9 ("sched: Move sched_class::prio_changed() into the change pattern") is triggering the following warning:
rq_pin_lock()
\-WARN_ON_ONCE(rq->balance_callback && rq->balance_callback != &balance_push_callback);

Can you check if the following solution helps your case too:
https://lore.kernel.org/all/[email protected]/

I can still see the issue.
It seems the task deadline is also updated in:
sched_change_end()
\-enqueue_task_dl()
  \-enqueue_dl_entity()
    \-setup_new_dl_entity()
      \-replenish_dl_new_period()
if the task's period finished.

So in sched_change_end(), the task priority (i.e. p->dl.deadline) is updated.
This results in having an old_deadline earlier than the new p->dl.deadline. Thus the rq->balance_callback:

prio_changed_dl() {
...
if (dl_time_before(old_deadline, p->dl.deadline))
  deadline_queue_pull_task(rq);
...
}

Hum... so this one is a little more tricky.

So the normal rules are that DEQUEUE_SAVE + ENQUEUE_RESTORE should be as invariant as possible.

But what I think happens here is that at the point of dequeue we are effectively ready to throttle/replenish, but we don't.

Then at enqueue, we do. The replenish changes the deadline and we're up a creek.

Let me think about this for a bit...

Hello Peter,

Quoted 10 lines · Peter Zijlstra

Hum... so this one is a little more tricky.

So the normal rules are that DEQUEUE_SAVE + ENQUEUE_RESTORE should be as invariant as possible.

But what I think happens here is that at the point of dequeue we are effectively ready to throttle/replenish, but we don't.

Then at enqueue, we do. The replenish changes the deadline and we're up a creek.

I've the following data from the scenario in which I observe the same splat as Pierre splat wit the two fixes on top of tip:

    yes-4108    [194] d..2.    53.396872: get_prio_dl: get_prio_dl: clock(53060728757)
    yes-4108    [194] d..2.    53.396873: update_curr_dl_se: update_curr_dl_se: past throttle label
    yes-4108    [194] d..2.    53.396873: update_curr_dl_se: dl_throttled(0) dl_overrun(0) timer_queued(0) server?(0)
    yes-4108    [194] d..2.    53.396873: update_curr_dl_se: dl_se->runtime(190623) rq->dl.overloaded(0)
    yes-4108    [194] d..2.    53.396874: get_prio_dl: get_prio_dl: deadline(53060017809)

    yes-4108    [194] d..2.    53.396878: enqueue_dl_entity: ENQUEUE_RESTORE update_dl_entity
    yes-4108    [194] d..2.    53.396878: enqueue_dl_entity: setup_new_dl_entity
    yes-4108    [194] d..2.    53.396878: enqueue_dl_entity: Replenish: Old: 53060017809 dl_deadline(1000000)
    yes-4108    [194] d..2.    53.396879: enqueue_dl_entity: Replenish: New: 53061728757
    yes-4108    [194] d..2.    53.396882: prio_changed_dl.part.0: Woops! prio_changed_dl: CPU(194) clock(53060728757) overloaded(0): Task: yes(4108), Curr: yes(4108) deadline: 53060017809 -> 53061728757

get_prio_dl() sees "deadline < rq->clock" but dl_se->runtime is still positive so update_curr_dl_se() doesn't fiddle with the deadline.

ENQUEUE_RESTORE sees "deadline" before "rq->clock" and calls setup_new_dl_entity() which calls replenish.

Quoted 1 line

Let me think about this for a bit...

Should prio_changed_dl() care about "dl_se->dl_deadline" having changed within the sched_change guard since that is the attribute that can be changed using sched_setattr() right?

Juri, Luca, I'm tempted to suggest to simply remove the replenish on RESTORE entirely -- that would allow the task to continue as it had been, irrespective of it being 'late'.

Something like so -- what would this break?

Quoted 19 lines · Peter Zijlstra

Juri, Luca, I'm tempted to suggest to simply remove the replenish on RESTORE entirely -- that would allow the task to continue as it had been, irrespective of it being 'late'.

Something like so -- what would this break?

kernel/sched/deadline.c +0 -4

@@ -2214,10 +2214,6 @@ enqueue_dl_entity(struct sched_dl_entity
 		update_dl_entity(dl_se);
 	} 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);
 	}
 

Quoted 1 line

/*

Ah, this is de-boost, right? Boosting allows one to break the CBS rules and then we have to rein in the excesses.

But we have {DE,EN}QUEUE_MOVE for this, that explicitly allows priority to change and is set for rt_mutex_setprio() (among others).

So doing s/RESTORE/MOVE/ above.

The corollary to all this is that everybody that sets MOVE must be able to deal with balance callbacks, so audit that too.

This then gives something like so.. which builds and boots for me, but clearly I haven't been able to trigger these funny cases.

---

kernel/sched/core.c +5 -1

@@ -4969,9 +4969,13 @@ struct balance_callback *splice_balance_
 	return __splice_balance_callbacks(rq, true);
 }
 
-static void __balance_callbacks(struct rq *rq)
+void __balance_callbacks(struct rq *rq, struct rq_flags *rf)
 {
+	if (rf)
+		rq_unpin_lock(rq, rf);
 	do_balance_callbacks(rq, __splice_balance_callbacks(rq,
 false));
+	if (rf)
+		rq_repin_lock(rq, rf);
 }
 

void balance_callbacks(struct rq *rq, struct balance_callback *head)

kernel/sched/core.c +3 -3

@@ -5018,7 +5022,7 @@ static inline void finish_lock_switch(st
 	 * prev into current:
 	 */
 	spin_acquire(&__rq_lockp(rq)->dep_map, 0, 0, _THIS_IP_);
-	__balance_callbacks(rq);
+	__balance_callbacks(rq, NULL);
 	raw_spin_rq_unlock_irq(rq);
 }
 

@@ -6901,7 +6905,7 @@ static void __sched notrace __schedule(i
 			proxy_tag_curr(rq, next);
 
 		rq_unpin_lock(rq, &rf);
-		__balance_callbacks(rq);
+		__balance_callbacks(rq, NULL);
 		raw_spin_rq_unlock_irq(rq);
 	}
 	trace_sched_exit_tp(is_switch);

@@ -7350,7 +7354,7 @@ void rt_mutex_setprio(struct task_struct
 	trace_sched_pi_setprio(p, pi_task);
 	oldprio = p->prio;
 
-	if (oldprio == prio)
+	if (oldprio == prio && !dl_prio(prio))
 		queue_flag &= ~DEQUEUE_MOVE;
 
 	prev_class = p->sched_class;

@@ -7396,9 +7400,7 @@ void rt_mutex_setprio(struct task_struct
 out_unlock:
 	/* Caller holds task_struct::pi_lock, IRQs are still

disabled */

kernel/sched/core.c +5 -3

-	rq_unpin_lock(rq, &rf);
-	__balance_callbacks(rq);
-	rq_repin_lock(rq, &rf);
+	__balance_callbacks(rq, &rf);
 	__task_rq_unlock(rq, p, &rf);
 }
 #endif /* CONFIG_RT_MUTEXES */

@@ -9167,6 +9169,8 @@ void sched_move_task(struct task_struct
 
 	if (resched)
 		resched_curr(rq);
+
+	__balance_callbacks(rq, &rq_guard.rf);
 }
 
 static struct cgroup_subsys_state *

@@ -10891,6 +10895,9 @@ void sched_change_end(struct sched_chang
 				resched_curr(rq);
 		}
 	} else {
+		/*
+		 * XXX validate prio only really changed when

ENQUEUE_MOVE is set.

kernel/sched/core.c +1 -0

+		 */
 		p->sched_class->prio_changed(rq, p, ctx->prio);
 	}
 }

kernel/sched/deadline.c +3 -1

@@ -2214,9 +2214,14 @@ enqueue_dl_entity(struct sched_dl_entity
 		update_dl_entity(dl_se);
 	} else if (flags & ENQUEUE_REPLENISH) {
 		replenish_dl_entity(dl_se);
-	} else if ((flags & ENQUEUE_RESTORE) &&
+	} else if ((flags & ENQUEUE_MOVE) &&
 		   !is_dl_boosted(dl_se) &&
 		   dl_time_before(dl_se->deadline,
 rq_clock(rq_of_dl_se(dl_se)))) {
+		/*
+		 * Deals with the de-boost case, and ENQUEUE_MOVE

explicitly

kernel/sched/deadline.c +1 -0

+		 * allows us to change priority. Callers are

expected to deal

kernel/sched/deadline.c +2 -0

+		 * with balance_callbacks.
+		 */
 		setup_new_dl_entity(dl_se);
 	}

kernel/sched/ext.c +1 -0

@@ -545,6 +545,7 @@ static void scx_task_iter_start(struct s
 static void __scx_task_iter_rq_unlock(struct scx_task_iter *iter)
 {
 	if (iter->locked_task) {
+		__balance_callbacks(iter->rq, &iter->rf);
 		task_rq_unlock(iter->rq, iter->locked_task,
 &iter->rf); iter->locked_task = NULL;
 	}

kernel/sched/sched.h +0 -0

@@ -2430,7 +2430,8 @@ extern const u32

sched_prio_to_wmult[40
* should preserve as much state as possible.
*
* MOVE - paired with SAVE/RESTORE, explicitly does not preserve the
location
- * in the runqueue.
+ * in the runqueue. IOW the priority is allowed to change.
Callers
+ * must expect to deal with balance callbacks.
*
* NOCLOCK - skip the update_rq_clock() (avoids double updates) *

kernel/sched/sched.h +1 -0

@@ -4019,6 +4020,8 @@ extern void enqueue_task(struct rq *rq,
 extern bool dequeue_task(struct rq *rq, struct task_struct *p, int
 flags); 
 extern struct balance_callback *splice_balance_callbacks(struct rq
 *rq); +
+extern void __balance_callbacks(struct rq *rq, struct rq_flags *rf);
 extern void balance_callbacks(struct rq *rq, struct balance_callback
 *head); 
 /*

kernel/sched/syscalls.c +1 -1

@@ -639,7 +639,7 @@ int __sched_setscheduler(struct task_str
 		 * itself.
 		 */
 		newprio = rt_effective_prio(p, newprio);
-		if (newprio == oldprio)
+		if (newprio == oldprio && !dl_prio(newprio))
 			queue_flags &= ~DEQUEUE_MOVE;
 	}