Re: [PATCH v7 3/3] sched/uclamp: Fix a deadlock when enabling uclamp static key

[Peter Zijlstra <peterz@infradead.org>]

On Thu, Jul 16, 2020 at 12:03:47PM +0100, Qais Yousef wrote:

I've trimmed the Changelog to read like:

---
Subject: sched/uclamp: Fix a deadlock when enabling uclamp static key
From: Qais Yousef <qais.yousef@arm.com>
Date: Thu, 16 Jul 2020 12:03:47 +0100

From: Qais Yousef <qais.yousef@arm.com>

The following splat was caught when setting uclamp value of a task:

  BUG: sleeping function called from invalid context at ./include/linux/percpu-rwsem.h:49

   cpus_read_lock+0x68/0x130
   static_key_enable+0x1c/0x38
   __sched_setscheduler+0x900/0xad8

Fix by ensuring we enable the key outside of the critical section in
__sched_setscheduler()

Fixes: 46609ce22703 ("sched/uclamp: Protect uclamp fast path code with static key")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://lkml.kernel.org/r/20200716110347.19553-4-qais.yousef@arm.com
---

And placed this patch first in the series

That said; don't we have a slight problem with enabling the key late
like this? It means the uclamp will not actually take effect immediately
and we'll have to wait for the next context switch ... whenever that
might be.

Should we not have enabled the key early, instead of late?

something like so perhaps?

---
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index a2a244af9a53..c6499b2696f5 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1282,8 +1282,6 @@ static void __setscheduler_uclamp(struct task_struct *p,
 	if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
 		return;
 
-	static_branch_enable(&sched_uclamp_used);
-
 	if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
 		uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
 			      attr->sched_util_min, true);
@@ -5074,6 +5072,7 @@ static int __sched_setscheduler(struct task_struct *p,
 		retval = uclamp_validate(p, attr);
 		if (retval)
 			return retval;
+		static_branch_enable(&sched_uclamp_used);
 	}
 
 	if (pi)