linux-kernel - Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Jul 2021 14:09:39 +0100
From:   Quentin Perret <qperret@...gle.com>
To:     Dietmar Eggemann <dietmar.eggemann@....com>
Cc:     mingo@...hat.com, peterz@...radead.org, vincent.guittot@...aro.org,
        qais.yousef@....com, rickyiu@...gle.com, wvw@...gle.com,
        patrick.bellasi@...bug.net, xuewen.yan94@...il.com,
        linux-kernel@...r.kernel.org, kernel-team@...roid.com
Subject: Re: [PATCH v4 1/2] sched: Fix UCLAMP_FLAG_IDLE setting

Hi Dietmar,

On Wednesday 21 Jul 2021 at 12:07:04 (+0200), Dietmar Eggemann wrote:
> On 19/07/2021 18:16, Quentin Perret wrote:
> > The UCLAMP_FLAG_IDLE flag is set on a runqueue when dequeueing the last
> > active task to maintain the last uclamp.max and prevent blocked util
> 
> s/active/runnable ?

'active' should still be correct here no? We enter uclamp_rq_max_value()
-> uclamp_idle_value() when the last _active_ uclamp_se is decremented,
and when all the buckets are empty, so I think that works?

> > from suddenly becoming visible.
> > 
> 
> [...]
> 
> IMHO, the main argument in v3 to do the clearing outside
> uclamp_rq_inc_id() was a possible order change in `for_each_clamp_id()`.
> So setting/clearing `rq->uclamp_flags` (UCLAMP_FLAG_IDLE) on UCLAMP_MAX
> (currently the highest Uclamp constraint (UCLAMP_CNT-1)) could be
> incorrect when UCLAMP_MIN and UCLAMP_MAX change place because the
> same `rq->uclamp_flags` value is needed for both Uclamp constraint
> values.
> 
> What about decoupling rq->uclamp_flags` handling from UCLAMP_MAX and
> doing this for 'UCLAMP_CNT - 1', i.e. always on the highest Uclamp
> constraint?
> 
> #define for_each_clamp_id(clamp_id) \
>     for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
> 
> In this case the code change can be as easy as in your original v3.
> 
> Setting UCLAMP_FLAG_IDLE in uclamp_idle_value():
> 
>   uclamp_rq_dec_id() -> uclamp_rq_max_value() -> *uclamp_idle_value()*
> 
> Resetting UCLAMP_FLAG_IDLE in uclamp_idle_reset():
> 
>   uclamp_rq_inc_id()                          -> *uclamp_idle_reset()*  
> 
> This would be more symmetrical then uclamp_idle_value() and
> uclamp_rq_inc()/uclamp_rq_reinc_id().

Right, thanks for the suggestion but to be fair I feel like this is a
matter of personal preference at this point. I personally like the way
it is in this patch -- I find it easier to reason about, but maybe
that's because I wrote it ...

Do you feel strongly about it? If not I'd prefer to not re-spin this
another time if possible. Let me know what you think.

Cheers,
Quentin