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