linux-kernel - Re: [PATCH 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: <CAB8ipk_qXYrMC54ddxA=wJ7YabxMwKmp1MzJRSq=NcfMJDOfjQ@mail.gmail.com>
Date: Mon, 24 Feb 2025 09:55:29 +0800
From: Xuewen Yan <xuewen.yan94@...il.com>
To: Qais Yousef <qyousef@...alina.io>
Cc: Xuewen Yan <xuewen.yan@...soc.com>, mingo@...hat.com, peterz@...radead.org, 
	juri.lelli@...hat.com, vincent.guittot@...aro.org, dietmar.eggemann@....com, 
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com, 
	ke.wang@...soc.com, di.shen@...soc.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before
 enable it

Hi Qais,

On Sun, Feb 23, 2025 at 7:36 AM Qais Yousef <qyousef@...alina.io> wrote:
>
> On 02/13/25 17:15, 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 the uclamp_validate() would call the static_branch_enable()
> > frequently, so add the uclamp_is_used() check to prevent calling
> > the cpus_read_lock() frequently.
>
> FWIW original patch was doing such check but it was taken out after review
> comments.
>
> Is something like below completely broken instead? I think uclamp usage isn't
> unique but haven't really audited the code to see if there are similar users.
>
> I think it is a valid pattern to allow and the expectation was there shouldn't
> be side effect of calling this repeatedly.
>
> Good catch by the way.
>
> --->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;
> +       }
> +
>         cpus_read_lock();
>         static_key_disable_cpuslocked(key);
>         cpus_read_unlock();
>
> --->8---

I don't think we should do it this way.
Uclamp can do this because it has never been disabled after being enabled.
However, for others, they might frequently enable and disable it.
If we don't add a lock here, there could be concurrency issues due to
potential race conditions.

---
By the way, I sincerely apologize for forgetting to add you when I
sent the patch-v2 and patch-v3 emails.

V2: https://lore.kernel.org/all/20250219093747.2612-2-xuewen.yan@unisoc.com/
V3: https://lore.kernel.org/all/20250220055950.4405-2-xuewen.yan@unisoc.com/

BR

---
xuewen

>
> >
> > Signed-off-by: Xuewen Yan <xuewen.yan@...soc.com>
> > ---
> >  kernel/sched/syscalls.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> > index 456d339be98f..d718fddadb03 100644
> > --- a/kernel/sched/syscalls.c
> > +++ b/kernel/sched/syscalls.c
> > @@ -368,7 +368,8 @@ static int uclamp_validate(struct task_struct *p,
> >        * blocking operation which obviously cannot be done while holding
> >        * scheduler locks.
> >        */
> > -     static_branch_enable(&sched_uclamp_used);
> > +     if (!uclamp_is_used())
> > +             static_branch_enable(&sched_uclamp_used);
> >
> >       return 0;
> >  }
> > --
> > 2.25.1
> >