linux-kernel - Re: [PATCH v3 2/2] sched/uclamp: Add uclamp_is

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] [day] [month] [year] [list]
Message-ID: <20250225131743.id4q2hotdfvzhkfh@airbuntu>
Date: Tue, 25 Feb 2025 13:17:43 +0000
From: Qais Yousef <qyousef@...alina.io>
To: Xuewen Yan <xuewen.yan94@...il.com>
Cc: Xuewen Yan <xuewen.yan@...soc.com>, vincent.guittot@...aro.org,
	mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
	christian.loehle@....com, dietmar.eggemann@....com,
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
	vschneid@...hat.com, hongyan.xia2@....com, ke.wang@...soc.com,
	di.shen@...soc.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] sched/uclamp: Add uclamp_is_used() check before
 enable it

On 02/25/25 14:23, Xuewen Yan wrote:
> On Tue, Feb 25, 2025 at 7:45 AM Qais Yousef <qyousef@...alina.io> wrote:
> >
> > On 02/20/25 13:59, Xuewen Yan wrote:
> > > Because the static_branch_enable() would get the cpus_read_lock(),
> > > and sometimes users may frequently set the uclamp value of tasks,
> > > and this operation would call the static_branch_enable()
> > > frequently, so add the uclamp_is_used() check to prevent calling
> > > the cpus_read_lock() frequently.
> > > And to make the code more concise, add a helper function to encapsulate
> > > this and use it everywhere we enable sched_uclamp_used.
> > >
> > > Signed-off-by: Xuewen Yan <xuewen.yan@...soc.com>
> > > Reviewed-by: Vincent Guittot <vincent.guittot@...aro.org>
> > > Reviewed-by: Christian Loehle <christian.loehle@....com>
> > > ---
> >
> > [...]
> >
> > > +/*
> > > + * Enabling static branches would get the cpus_read_lock(),
> > > + * check uclamp_is_used before enabling it. There is no race
> > > + * issue because we never disable this static key once enabled.
> > > + */
> > > +static inline void sched_uclamp_enable(void)
> > > +{
> > > +     if (!uclamp_is_used())
> > > +             static_branch_enable(&sched_uclamp_used);
> > > +}
> > > +
> >
> > As I indicated in [1] I think the pattern of repeatedly enable is actually sane
> > and what we probably should be doing is modify the static_key_enable() logic to
> > do the early bail out logic outside of the lock. I had this code this way FWIW
> > initially and Peter asked for it to be called unconditionally instead.
> >
> > I think repeated calls to static_key_enable/disable() should be made cheap and
> > I don't see a side effect of _replicating_ the early bail out logic outside of
> > the lock so that if we have already enabled/disabled we just return immediately
> > without any side effect (holding the lock in this case). I agree the hotplug
> 
> Because of the jump_lable_lock(), early bailout could indeed be a good idea.

Yeah AFAICS the current code already handles the double enable, but it's
embedded within the cpus_read_lock(). I don't see why they must be inside.
Though I think we can't just move the code, we must replicate it outside the
lock in case the two enables race. I don't think the atomic_t will protect and
force one of them only to proceed. The lock will be stronger synchronization to
serialize, but one of them will end up doing the actual enable and the other
one will bail out after holding the lock. All future redundant enables will
bail out before holding the lock safely AFAICT.

> 
> > lock is ugly and if we can avoid touching it when we don't really need to that
> > would be better.
> >
> > --->8---
> >
> > diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> > index d9c822bbffb8..17583c98c447 100644
> > --- a/kernel/jump_label.c
> > +++ b/kernel/jump_label.c
> > @@ -214,6 +214,13 @@ EXPORT_SYMBOL_GPL(static_key_enable_cpuslocked);
> >
> >  void static_key_enable(struct static_key *key)
> >  {
> > +       STATIC_KEY_CHECK_USE(key);
> > +
> > +       if (atomic_read(&key->enabled) > 0) {
> > +               WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
> > +               return;
> > +       }
> > +
> >         cpus_read_lock();
> >         static_key_enable_cpuslocked(key);
> >         cpus_read_unlock();
> > @@ -239,6 +246,13 @@ EXPORT_SYMBOL_GPL(static_key_disable_cpuslocked);
> >
> >  void static_key_disable(struct static_key *key)
> >  {
> > +       STATIC_KEY_CHECK_USE(key);
> > +
> > +       if (atomic_read(&key->enabled) > 0) {
> > +               WARN_ON_ONCE(atomic_read(&key->enabled) != 1);
> > +               return;
> > +       }
> 
> Maybe here should be just check whether == 0, because when enabling
> the static key, the enable may occur to be -1.

If it is -1 then the code is already being patched to enable the key and this
call to enable is redundant and can be ignored, no? ie: if we check for ==
0 then we'll hold the lock to bail out anyway.

There's no need for the warning though as this can trigger incorrectly, yes.

> 
> +       if (atomic_read(&key->enabled) == 0)
> +               return;
> 
> > +
> >         cpus_read_lock();
> >         static_key_disable_cpuslocked(key);
> >         cpus_read_unlock();
> >
> > --->8---
> >
> > [1] https://lore.kernel.org/all/20250222233627.3yx55ks5lnq3elby@airbuntu/
> 
> BR