diff mbox series

sched: Fix balance_push() vs __sched_setscheduler()

Message ID Yp/Frp7BMp9E5dSp@hirez.programming.kicks-ass.net (mailing list archive)
State New, archived
Headers show
Series sched: Fix balance_push() vs __sched_setscheduler() | expand

Commit Message

Peter Zijlstra June 7, 2022, 9:39 p.m. UTC 
On Tue, Jun 07, 2022 at 10:40:36PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 03, 2022 at 12:15:51AM +0800, Jing-Ting Wu wrote:

> > The patch is helpful to the syndrome, passed stability test over 10
> > days so far. (as-is: < 48 hours failed)
> 
> Excellent, let me go write a Changelog for it, or something.

How's this then?

---
Subject: sched: Fix balance_push() vs __sched_setscheduler()
From: Peter Zijlstra <peterz@infradead.org>
Date: Tue Jun 7 22:41:55 CEST 2022

The purpose of balance_push() is to act as a filter on task selection
in the case of CPU hotplug, specifically when taking the CPU out.

It does this by (ab)using the balance callback infrastructure, with
the express purpose of keeping all the unlikely/odd cases in a single
place.

In order to serve it's purpose, the balance_push_callback needs to be
(exclusively) on the callback list at all times (noting that the
callback always places itself back on the list the moment it runs,
also noting that when the CPU goes down, regular balancing concerns
are moot, so ignoring them is fine).

And here-in lies the problem, __sched_setscheduler()'s use of
splice_balance_callbacks() takes the callbacks off the list across a
lock-break, making it possible for, an interleaving, __schedule() to
see an empty list and not get filtered.

Reported-by: Jing-Ting Wu <jing-ting.wu@mediatek.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Jing-Ting Wu <jing-ting.wu@mediatek.com>
Link: https://lkml.kernel.org/r/20220519134706.GH2578@worktop.programming.kicks-ass.net
---
 kernel/sched/core.c |   36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

Comments

Jing-Ting Wu June 8, 2022, 2:16 p.m. UTC | #1 
Hi Peter


On Tue, 2022-06-07 at 23:39 +0200, Peter Zijlstra wrote:
> On Tue, Jun 07, 2022 at 10:40:36PM +0200, Peter Zijlstra wrote:
> > On Fri, Jun 03, 2022 at 12:15:51AM +0800, Jing-Ting Wu wrote:
> > > The patch is helpful to the syndrome, passed stability test over
> > > 10
> > > days so far. (as-is: < 48 hours failed)
> > 
> > Excellent, let me go write a Changelog for it, or something.
> 
> How's this then?

I think the description is fine.
Thanks for your help.

[...]
>  
> -static inline struct callback_head *splice_balance_callbacks(struct
> rq *rq)
> +static inline struct callback_head *
> +__splice_balance_callbacks(struct rq *rq, bool split)
>  {
>  	struct callback_head *head = rq->balance_callback;
>  
> +	if (likely(!head))
> +		return NULL;
> +
>  	lockdep_assert_rq_held(rq);
> -	if (head)
> +	/*
> +	 * Must not take balance_push_callback off the list when
> +	 * splace_balance_callbac() and balance_callbacks() are not


Should we change splace_balance_callbac() to splice_balance_callbacks()
at here?


> +	 * in the same rq->lock section.
> +	 *
> +	 * In that case it would be possible for __schedule() to
> interleave
> +	 * and observe the list empty.
> +	 */
> +	if (split && head == &balance_push_callback)
> +		head = NULL;
> +	else
>  		rq->balance_callback = NULL;
>  
>  	return head;
>  }
>  
[...]


Best Regards,
Jing-Ting Wu
Peter Zijlstra June 8, 2022, 2:48 p.m. UTC | #2 
On Wed, Jun 08, 2022 at 10:16:43PM +0800, Jing-Ting Wu wrote:
> > +	/*
> > +	 * Must not take balance_push_callback off the list when
> > +	 * splace_balance_callbac() and balance_callbacks() are not
> 
> 
> Should we change splace_balance_callbac() to splice_balance_callbacks()
> at here?

Pff, typing is so hard.. :-)

I'll also go find me a Fixes tag I suppose.
diff mbox series

Patch

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4798,25 +4798,55 @@  static void do_balance_callbacks(struct
 
 static void balance_push(struct rq *rq);
 
+/*
+ * balance_push_callback is a right abuse of the callback interface and plays
+ * by significantly different rules.
+ *
+ * Where the normal balance_callback's purpose is to be ran in the same context
+ * that queued it (only later, when it's safe to drop rq->lock again),
+ * balance_push_callback is specifically targeted at __schedule().
+ *
+ * This abuse is tolerated because it places all the unlikely/odd cases behind
+ * a single test, namely: rq->balance_callback == NULL.
+ */
 struct callback_head balance_push_callback = {
 	.next = NULL,
 	.func = (void (*)(struct callback_head *))balance_push,
 };
 
-static inline struct callback_head *splice_balance_callbacks(struct rq *rq)
+static inline struct callback_head *
+__splice_balance_callbacks(struct rq *rq, bool split)
 {
 	struct callback_head *head = rq->balance_callback;
 
+	if (likely(!head))
+		return NULL;
+
 	lockdep_assert_rq_held(rq);
-	if (head)
+	/*
+	 * Must not take balance_push_callback off the list when
+	 * splace_balance_callbac() and balance_callbacks() are not
+	 * in the same rq->lock section.
+	 *
+	 * In that case it would be possible for __schedule() to interleave
+	 * and observe the list empty.
+	 */
+	if (split && head == &balance_push_callback)
+		head = NULL;
+	else
 		rq->balance_callback = NULL;
 
 	return head;
 }
 
+static inline struct callback_head *splice_balance_callbacks(struct rq *rq)
+{
+	return __splice_balance_callbacks(rq, true);
+}
+
 static void __balance_callbacks(struct rq *rq)
 {
-	do_balance_callbacks(rq, splice_balance_callbacks(rq));
+	do_balance_callbacks(rq, __splice_balance_callbacks(rq, false));
 }
 
 static inline void balance_callbacks(struct rq *rq, struct callback_head *head)