Messages in this thread | | | Date | Fri, 2 Jul 2021 11:54:07 +0000 | From | Quentin Perret <> | Subject | Re: [PATCH v3 1/3] sched: Fix UCLAMP_FLAG_IDLE setting |
| |
On Thursday 01 Jul 2021 at 18:59:32 (+0100), Qais Yousef wrote: > On 07/01/21 15:20, Quentin Perret wrote: > > > > Right or maybe we can just check that uclamp_id == UCLAMP_MAX here and > > > > we should be good to go? That is, what about the below? > > > > > > Wouldn't it be better to do this from uclamp_idle_reset() then? > > > > That should work too, but clearing the flag outside of > > uclamp_rq_inc_id() feels a little bit more robust to ordering > > issues. > > > > Specifically, uclamp_rq_inc() has the following pattern: > > > > for_each_clamp_id(clamp_id) > > uclamp_rq_inc_id(rq, p , clamp_id); > > > > if (rq->uclamp_flags & UCLAMP_FLAG_IDLE) > > rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE; > > > > So, if we change this to clear the flag from > > uclamp_rq_inc_id()->uclamp_idle_reset() then we'll have issues if > > (for example) for_each_clamp_id()'s order changes in the future. > > IOW, it feels cleaner to not create side effects in uclamp_rq_inc_id() > > that impact the idle flag given that its very own behaviour depends on > > the flag. > > > > WDYT? > > Do the clearing from outside the loop then to keep the pattern consistent?
Right, but I actually preferred doing it from here as we're under task_rq_lock(), which means well behaved readers won't observe the flag being transiently set. I could also refactor the locking, but oh well ...
> Anyway, I think there's no clear objective advantage. So I'll trust your > judgement and promise not to complain with your final choice ;-)
:) Alrighty, I'll cook something.
Thanks! Quentin
| |