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

[PATCH] 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. This only
impacts the first wake up 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 we reset rq uclamp_max when waking up from idle.

Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
Signed-off-by: Qais Yousef <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 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] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[Valentin Schneider]

On 25/11/21 16:52, 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. This only
> impacts the first wake up after enabling the static key. And it only

Wouldn't that rather be all wakeups after enabling the static key, until
the rq goes idle and gains UCLAMP_FLAG_IDLE? e.g. if you enqueue N
uclamp_max=512 tasks, the first enqueue flips the static key and the rq
max-aggregate will stay at 1024 after the remaining enqueues.

> 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 we reset rq uclamp_max when waking up from idle.
>
> Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")

Looking at this again, I'm starting to think this actually stems from the
introduction of the flag:

  e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")

Before the commit you point at, we would still initialize ->uclamp_flags to
0. This was probably hidden by all the activity at boot-time (e.g. just
unparking smpboot threads) which yielded an nr_running>0 -> nr_running==0
transition, IOW we'd most likely get UCLAMP_FLAG_IDLE set on a rq before
any userspace task could get on there.

The static key would have only made this problem more visible.

> Signed-off-by: Qais Yousef <qais.yousef@arm.com>

Changelog nitpicking aside:
Reviewed-by: Valentin Schneider <Valentin.Schneider@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 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] sched/uclamp: Fix rq->uclamp_max not set on first enqueue

[Qais Yousef]

Hi Valentin

On 11/26/21 10:51, Valentin Schneider wrote:
> On 25/11/21 16:52, 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. This only
> > impacts the first wake up after enabling the static key. And it only
> 
> Wouldn't that rather be all wakeups after enabling the static key, until
> the rq goes idle and gains UCLAMP_FLAG_IDLE? e.g. if you enqueue N
> uclamp_max=512 tasks, the first enqueue flips the static key and the rq
> max-aggregate will stay at 1024 after the remaining enqueues.

Yep. Bad phrasing from my side. How about:

"This is visible from first wake up(s) until the first dequeue to idle after
enabling the static key"?

> 
> > 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 we reset rq uclamp_max when waking up from idle.
> >
> > Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
> 
> Looking at this again, I'm starting to think this actually stems from the
> introduction of the flag:
> 
>   e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
> 
> Before the commit you point at, we would still initialize ->uclamp_flags to
> 0. This was probably hidden by all the activity at boot-time (e.g. just
> unparking smpboot threads) which yielded an nr_running>0 -> nr_running==0
> transition, IOW we'd most likely get UCLAMP_FLAG_IDLE set on a rq before
> any userspace task could get on there.
> 
> The static key would have only made this problem more visible.

Hmm. I can't see the sequence of events. I guess you could argue in theory that
this commit should have initialized the ->uclamp_flags to UCLAMP_FLAG_IDLE but
I think it used to work because uc_rq->value = 0 by default

	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);
	}

The commit I point to changed makes uc_rq->value = 1024 by default, hence we
miss the first update.

I don't mind updating the FIXES tag here, though AFAICT there's no visible side
effect from it.

> 
> > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
> 
> Changelog nitpicking aside:
> Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>

Thanks!

--
Qais Yousef

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

[Valentin Schneider]

On 30/11/21 11:23, Qais Yousef wrote:
> Hi Valentin
>
> On 11/26/21 10:51, Valentin Schneider wrote:
>> On 25/11/21 16:52, 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. This only
>> > impacts the first wake up after enabling the static key. And it only
>>
>> Wouldn't that rather be all wakeups after enabling the static key, until
>> the rq goes idle and gains UCLAMP_FLAG_IDLE? e.g. if you enqueue N
>> uclamp_max=512 tasks, the first enqueue flips the static key and the rq
>> max-aggregate will stay at 1024 after the remaining enqueues.
>
> Yep. Bad phrasing from my side. How about:
>
> "This is visible from first wake up(s) until the first dequeue to idle after
> enabling the static key"?
>

Sounds good.

>>
>> > 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 we reset rq uclamp_max when waking up from idle.
>> >
>> > Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
>>
>> Looking at this again, I'm starting to think this actually stems from the
>> introduction of the flag:
>>
>>   e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
>>
>> Before the commit you point at, we would still initialize ->uclamp_flags to
>> 0. This was probably hidden by all the activity at boot-time (e.g. just
>> unparking smpboot threads) which yielded an nr_running>0 -> nr_running==0
>> transition, IOW we'd most likely get UCLAMP_FLAG_IDLE set on a rq before
>> any userspace task could get on there.
>>
>> The static key would have only made this problem more visible.
>
> Hmm. I can't see the sequence of events. I guess you could argue in theory that
> this commit should have initialized the ->uclamp_flags to UCLAMP_FLAG_IDLE but
> I think it used to work because uc_rq->value = 0 by default
>
>       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);
>       }
>
> The commit I point to changed makes uc_rq->value = 1024 by default, hence we
> miss the first update.
>
> I don't mind updating the FIXES tag here, though AFAICT there's no visible side
> effect from it.
>

Oh, you're right, that initial uc_rq->value ends up being equivalent to
having the flag. Sorry for the confusion!

Patching up that original commit would only really be a "code correctness"
thing, it wouldn't fix any visible problem, so I think it's better to keep
your current Fixes:.

>>
>> > Signed-off-by: Qais Yousef <qais.yousef@arm.com>
>>
>> Changelog nitpicking aside:
>> Reviewed-by: Valentin Schneider <Valentin.Schneider@arm.com>
>
> Thanks!
>
> --
> Qais Yousef

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

[Qais Yousef]

On 11/30/21 12:29, Valentin Schneider wrote:
> On 30/11/21 11:23, Qais Yousef wrote:
> > Hi Valentin
> >
> > On 11/26/21 10:51, Valentin Schneider wrote:
> >> On 25/11/21 16:52, 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. This only
> >> > impacts the first wake up after enabling the static key. And it only
> >>
> >> Wouldn't that rather be all wakeups after enabling the static key, until
> >> the rq goes idle and gains UCLAMP_FLAG_IDLE? e.g. if you enqueue N
> >> uclamp_max=512 tasks, the first enqueue flips the static key and the rq
> >> max-aggregate will stay at 1024 after the remaining enqueues.
> >
> > Yep. Bad phrasing from my side. How about:
> >
> > "This is visible from first wake up(s) until the first dequeue to idle after
> > enabling the static key"?
> >
> 
> Sounds good.

+1

> 
> >>
> >> > 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 we reset rq uclamp_max when waking up from idle.
> >> >
> >> > Fixes: d81ae8aac85c ("sched/uclamp: Fix initialization of struct uclamp_rq")
> >>
> >> Looking at this again, I'm starting to think this actually stems from the
> >> introduction of the flag:
> >>
> >>   e496187da710 ("sched/uclamp: Enforce last task's UCLAMP_MAX")
> >>
> >> Before the commit you point at, we would still initialize ->uclamp_flags to
> >> 0. This was probably hidden by all the activity at boot-time (e.g. just
> >> unparking smpboot threads) which yielded an nr_running>0 -> nr_running==0
> >> transition, IOW we'd most likely get UCLAMP_FLAG_IDLE set on a rq before
> >> any userspace task could get on there.
> >>
> >> The static key would have only made this problem more visible.
> >
> > Hmm. I can't see the sequence of events. I guess you could argue in theory that
> > this commit should have initialized the ->uclamp_flags to UCLAMP_FLAG_IDLE but
> > I think it used to work because uc_rq->value = 0 by default
> >
> >       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);
> >       }
> >
> > The commit I point to changed makes uc_rq->value = 1024 by default, hence we
> > miss the first update.
> >
> > I don't mind updating the FIXES tag here, though AFAICT there's no visible side
> > effect from it.
> >
> 
> Oh, you're right, that initial uc_rq->value ends up being equivalent to
> having the flag. Sorry for the confusion!

No worries! I probably need to mention this in the commit message too..

> 
> Patching up that original commit would only really be a "code correctness"
> thing, it wouldn't fix any visible problem, so I think it's better to keep
> your current Fixes:.

Cool. I'll let this brew a bit more and send v2 with the updated commit
message.

Thanks!

--
Qais Yousef

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

[Dietmar Eggemann]

On 30.11.21 16:41, Qais Yousef wrote:
> On 11/30/21 12:29, Valentin Schneider wrote:
>> On 30/11/21 11:23, Qais Yousef wrote:
>>> Hi Valentin
>>>
>>> On 11/26/21 10:51, Valentin Schneider wrote:
>>>> On 25/11/21 16:52, 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. This only
>>>>> impacts the first wake up after enabling the static key. And it only

LGTM.

Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Tested with rt-app:

   "tasks": {
        "task_n1": {
            "util_min": 0,
            "util_max": 369,
            "loop": 1,
            "phases": {
                "p000001": {                     
                    "loop": 5,
                    "run": 800,
                    "timer": {
                        "period": 16000,
                        "ref": "task_n1"
                    }
                }
            },
            "policy": "SCHED_OTHER"
        }
    }

w/o patch:

/*  missing (1) since rq->uclamp_flags = UCLAMP_FLAG_IDLE is not set initially */
[75.002086] (3) uclamp_rq_inc_id() CPU5 p=[task_n1-0 1693] uc_se->value=369 uc_rq->value=1024
/* first dequeue to _uclamp_ idle set UCLAMP_FLAG_IDLE */
[75.013851] (2) uclamp_idle_value() CPU5 p=[task_n1-0 1693] clamp_id=1 value=369
[75.017972] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1693] clamp_id=0 value=0
/* UCLAMP_FLAG_IDLE is set -> set rq->uclamp[UCLAMP_MAX].value to *369* */
[75.017984] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1693] clamp_id=1 value=369
[75.017995] (3) uclamp_rq_inc_id() CPU5 p=[task_n1-0 1693] uc_se->value=369 uc_rq->value=*369*

w/ patch:

[63.393974] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1700] clamp_id=0 value=0
/* UCLAMP_FLAG_IDLE is set -> set rq->uclamp[UCLAMP_MAX].value to *369* */
[63.401269] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1700] clamp_id=1 value=369
[63.415513] (3) uclamp_rq_inc_id() CPU5 p=[task_n1-0 1700] uc_se->value=369 uc_rq->value=*369*
/* first dequeue to _uclamp_ idle set UCLAMP_FLAG_IDLE (again) */
[63.434781] (2) uclamp_idle_value() CPU5 p=[task_n1-0 1700] clamp_id=1 value=369
[63.449681] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1700] clamp_id=0 value=0
[63.449691] (1) uclamp_idle_reset() CPU5 p=[task_n1-0 1700] clamp_id=1 value=369
[63.449699] (3) uclamp_rq_inc_id() CPU5 p=[task_n1-0 1700] uc_se->value=369 uc_rq->value=369

[...]

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

[Qais Yousef]

On 12/01/21 10:20, Dietmar Eggemann wrote:
> On 30.11.21 16:41, Qais Yousef wrote:
> > On 11/30/21 12:29, Valentin Schneider wrote:
> >> On 30/11/21 11:23, Qais Yousef wrote:
> >>> Hi Valentin
> >>>
> >>> On 11/26/21 10:51, Valentin Schneider wrote:
> >>>> On 25/11/21 16:52, 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. This only
> >>>>> impacts the first wake up after enabling the static key. And it only
> 
> LGTM.
> 
> Tested-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Thanks!

--
Qais Yousef