[PATCH v2] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[PATCH v2] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[Qais Yousef]

Commit d81ae8aac85c ("sched/uclamp: Fix initialization of struct
uclamp_rq") introduced a bug where uclamp_max of the rq is not reset to
match the woken up task's uclamp_max when the rq is idle.

The code was relying on rq->uclamp_max initialized to zero, so on first
enqueue

	static inline void uclamp_rq_inc_id(struct rq *rq, struct task_struct *p,
					    enum uclamp_id clamp_id)
	{
		...

		if (uc_se->value > READ_ONCE(uc_rq->value))
			WRITE_ONCE(uc_rq->value, uc_se->value);
	}

was actually resetting it. But since commit d81ae8aac85c changed the
default to 1024, this no longer works. And since rq->uclamp_flags is
also initialized to 0, neither above code path nor uclamp_idle_reset()
update the rq->uclamp_max on first wake up from idle.

This is only visible from first wake up(s) until the first dequeue to
idle after enabling the static key. And it only matters if the
uclamp_max of this task is < 1024 since only then its uclamp_max will be
effectively ignored.

Fix it by properly initializing rq->uclamp_flags = UCLAMP_FLAG_IDLE to
ensure uclamp_idle_reset() is called which then will update the rq
uclamp_max value as expected.

Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>
Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
---

Changes in v2:
  - Update commit message to be more descriptive (Valentin).
  - Added Reviewed-by and Tested-by from Valentin and Dietmar (Thanks!)

 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index beaa8be6241e..52b0c7513a32 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1929,7 +1929,7 @@ static void __init init_uclamp_rq(struct rq *rq)
 		};
 	}
 
-	rq->uclamp_flags = 0;
+	rq->uclamp_flags = UCLAMP_FLAG_IDLE;
 }
 
 static void __init init_uclamp(void)
-- 
2.25.1

Re: [PATCH v2] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[Peter Zijlstra]

On Thu, Dec 02, 2021 at 11:20:33AM +0000, Qais Yousef wrote:
> Commit d81ae8aac85c ("sched/uclamp: Fix initialization of struct
> uclamp_rq") introduced a bug where uclamp_max of the rq is not reset to
> match the woken up task's uclamp_max when the rq is idle.
> 
> The code was relying on rq->uclamp_max initialized to zero, so on first
> enqueue
> 
> 	static inline void uclamp_rq_inc_id(struct rq *rq, struct task_struct *p,
> 					    enum uclamp_id clamp_id)
> 	{
> 		...
> 
> 		if (uc_se->value > READ_ONCE(uc_rq->value))
> 			WRITE_ONCE(uc_rq->value, uc_se->value);
> 	}
> 
> was actually resetting it. But since commit d81ae8aac85c changed the
> default to 1024, this no longer works. And since rq->uclamp_flags is
> also initialized to 0, neither above code path nor uclamp_idle_reset()
> update the rq->uclamp_max on first wake up from idle.
> 
> This is only visible from first wake up(s) until the first dequeue to
> idle after enabling the static key. And it only matters if the
> uclamp_max of this task is < 1024 since only then its uclamp_max will be
> effectively ignored.
> 
> Fix it by properly initializing rq->uclamp_flags = UCLAMP_FLAG_IDLE to
> ensure uclamp_idle_reset() is called which then will update the rq
> uclamp_max value as expected.
> 
> Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
> Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>
> Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Thanks!

[tip: sched/urgent] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[tip-bot2 for Qais Yousef]

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

Commit-ID:     315c4f884800c45cb6bd8c90422fad554a8b9588
Gitweb:        https://git.kernel.org/tip/315c4f884800c45cb6bd8c90422fad554a8b9588
Author:        Qais Yousef <qais.yousef@arm.com>
AuthorDate:    Thu, 02 Dec 2021 11:20:33 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Sat, 04 Dec 2021 10:56:18 +01:00

sched/uclamp: Fix rq->uclamp_max not set on first enqueue

Commit d81ae8aac85c ("sched/uclamp: Fix initialization of struct
uclamp_rq") introduced a bug where uclamp_max of the rq is not reset to
match the woken up task's uclamp_max when the rq is idle.

The code was relying on rq->uclamp_max initialized to zero, so on first
enqueue

	static inline void uclamp_rq_inc_id(struct rq *rq, struct task_struct *p,
					    enum uclamp_id clamp_id)
	{
		...

		if (uc_se->value > READ_ONCE(uc_rq->value))
			WRITE_ONCE(uc_rq->value, uc_se->value);
	}

was actually resetting it. But since commit d81ae8aac85c changed the
default to 1024, this no longer works. And since rq->uclamp_flags is
also initialized to 0, neither above code path nor uclamp_idle_reset()
update the rq->uclamp_max on first wake up from idle.

This is only visible from first wake up(s) until the first dequeue to
idle after enabling the static key. And it only matters if the
uclamp_max of this task is < 1024 since only then its uclamp_max will be
effectively ignored.

Fix it by properly initializing rq->uclamp_flags = UCLAMP_FLAG_IDLE to
ensure uclamp_idle_reset() is called which then will update the rq
uclamp_max value as expected.

Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
Signed-off-by: Qais Yousef <qais.yousef@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>
Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Link: https://lkml.kernel.org/r/20211202112033.1705279-1-qais.yousef@arm.com
---
 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 814c52d..7756310 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1918,7 +1918,7 @@ static void __init init_uclamp_rq(struct rq *rq)
 		};
 	}
 
-	rq->uclamp_flags = 0;
+	rq->uclamp_flags = UCLAMP_FLAG_IDLE;
 }
 
 static void __init init_uclamp(void)