Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/uclamp: Fix rq->uclamp_max not set on first enqueue | Date | Tue, 30 Nov 2021 12:29:48 +0000 |
| |
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
| |