[PATCH 1/2] sched/uclamp: Alaways using uclamp_is_used()

[PATCH 1/2] sched/uclamp: Alaways using uclamp_is_used()

[Xuewen Yan]

Now, we have the uclamp_is_used() func to judge the uclamp enabled,
so replace the static_branch_unlikely(&sched_uclamp_used) with it.

Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
---
 kernel/sched/core.c  |  4 ++--
 kernel/sched/sched.h | 28 ++++++++++++++--------------
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 165c90ba64ea..841147759ec7 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1756,7 +1756,7 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
 	 * The condition is constructed such that a NOP is generated when
 	 * sched_uclamp_used is disabled.
 	 */
-	if (!static_branch_unlikely(&sched_uclamp_used))
+	if (!uclamp_is_used())
 		return;
 
 	if (unlikely(!p->sched_class->uclamp_enabled))
@@ -1783,7 +1783,7 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
 	 * The condition is constructed such that a NOP is generated when
 	 * sched_uclamp_used is disabled.
 	 */
-	if (!static_branch_unlikely(&sched_uclamp_used))
+	if (!uclamp_is_used())
 		return;
 
 	if (unlikely(!p->sched_class->uclamp_enabled))
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 38e0e323dda2..f5de05354d80 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -3394,6 +3394,19 @@ static inline bool update_other_load_avgs(struct rq *rq) { return false; }
 
 unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
 
+/*
+ * When uclamp is compiled in, the aggregation at rq level is 'turned off'
+ * by default in the fast path and only gets turned on once userspace performs
+ * an operation that requires it.
+ *
+ * Returns true if userspace opted-in to use uclamp and aggregation at rq level
+ * hence is active.
+ */
+static inline bool uclamp_is_used(void)
+{
+	return static_branch_likely(&sched_uclamp_used);
+}
+
 static inline unsigned long uclamp_rq_get(struct rq *rq,
 					  enum uclamp_id clamp_id)
 {
@@ -3417,7 +3430,7 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
 	unsigned long rq_util;
 	unsigned long max_util;
 
-	if (!static_branch_likely(&sched_uclamp_used))
+	if (!uclamp_is_used())
 		return false;
 
 	rq_util = cpu_util_cfs(cpu_of(rq)) + cpu_util_rt(rq);
@@ -3426,19 +3439,6 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
 	return max_util != SCHED_CAPACITY_SCALE && rq_util >= max_util;
 }
 
-/*
- * When uclamp is compiled in, the aggregation at rq level is 'turned off'
- * by default in the fast path and only gets turned on once userspace performs
- * an operation that requires it.
- *
- * Returns true if userspace opted-in to use uclamp and aggregation at rq level
- * hence is active.
- */
-static inline bool uclamp_is_used(void)
-{
-	return static_branch_likely(&sched_uclamp_used);
-}
-
 #define for_each_clamp_id(clamp_id) \
 	for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
 
-- 
2.25.1

[PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Xuewen Yan]

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.

Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Christian Loehle]

On 2/13/25 09: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.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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;
>  }

Given that we never disable sched_uclamp_used once active this
is fine.
Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Hongyan Xia]

On 13/02/2025 09: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.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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);

NIT: I was also wondering about race conditions, and then realized we 
never disable this static key, so maybe worth adding a comment here.

>   
>   	return 0;
>   }

Reviewed-by: Hongyan Xia <hongyan.xia2@arm.com>

Re: [PATCH 1/2] sched/uclamp: Alaways using uclamp_is_used()

[Hongyan Xia]

Title: s/Alaways/Always/

On 13/02/2025 09:15, Xuewen Yan wrote:
> Now, we have the uclamp_is_used() func to judge the uclamp enabled,
> so replace the static_branch_unlikely(&sched_uclamp_used) with it.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> ---
>   kernel/sched/core.c  |  4 ++--
>   kernel/sched/sched.h | 28 ++++++++++++++--------------
>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 165c90ba64ea..841147759ec7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1756,7 +1756,7 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>   	 * The condition is constructed such that a NOP is generated when
>   	 * sched_uclamp_used is disabled.
>   	 */
> -	if (!static_branch_unlikely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>   		return;
>   
>   	if (unlikely(!p->sched_class->uclamp_enabled))
> @@ -1783,7 +1783,7 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
>   	 * The condition is constructed such that a NOP is generated when
>   	 * sched_uclamp_used is disabled.
>   	 */
> -	if (!static_branch_unlikely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>   		return;
>   
>   	if (unlikely(!p->sched_class->uclamp_enabled))
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 38e0e323dda2..f5de05354d80 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3394,6 +3394,19 @@ static inline bool update_other_load_avgs(struct rq *rq) { return false; }
>   
>   unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
>   
> +/*
> + * When uclamp is compiled in, the aggregation at rq level is 'turned off'
> + * by default in the fast path and only gets turned on once userspace performs
> + * an operation that requires it.
> + *
> + * Returns true if userspace opted-in to use uclamp and aggregation at rq level
> + * hence is active.
> + */
> +static inline bool uclamp_is_used(void)
> +{
> +	return static_branch_likely(&sched_uclamp_used);
> +}
> +
>   static inline unsigned long uclamp_rq_get(struct rq *rq,
>   					  enum uclamp_id clamp_id)
>   {
> @@ -3417,7 +3430,7 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
>   	unsigned long rq_util;
>   	unsigned long max_util;
>   
> -	if (!static_branch_likely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>   		return false;
>   
>   	rq_util = cpu_util_cfs(cpu_of(rq)) + cpu_util_rt(rq);
> @@ -3426,19 +3439,6 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
>   	return max_util != SCHED_CAPACITY_SCALE && rq_util >= max_util;
>   }
>   
> -/*
> - * When uclamp is compiled in, the aggregation at rq level is 'turned off'
> - * by default in the fast path and only gets turned on once userspace performs
> - * an operation that requires it.
> - *
> - * Returns true if userspace opted-in to use uclamp and aggregation at rq level
> - * hence is active.
> - */
> -static inline bool uclamp_is_used(void)
> -{
> -	return static_branch_likely(&sched_uclamp_used);
> -}
> -
>   #define for_each_clamp_id(clamp_id) \
>   	for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
>   

Reviewed-by: Hongyan Xia <hongyan.xia2@arm.com>

Re: [PATCH 1/2] sched/uclamp: Alaways using uclamp_is_used()

[Christian Loehle]

On 2/13/25 09:15, Xuewen Yan wrote:
> Now, we have the uclamp_is_used() func to judge the uclamp enabled,
> so replace the static_branch_unlikely(&sched_uclamp_used) with it.
> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
> ---
>  kernel/sched/core.c  |  4 ++--
>  kernel/sched/sched.h | 28 ++++++++++++++--------------
>  2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 165c90ba64ea..841147759ec7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1756,7 +1756,7 @@ static inline void uclamp_rq_inc(struct rq *rq, struct task_struct *p)
>  	 * The condition is constructed such that a NOP is generated when
>  	 * sched_uclamp_used is disabled.
>  	 */
> -	if (!static_branch_unlikely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>  		return;
>  
>  	if (unlikely(!p->sched_class->uclamp_enabled))
> @@ -1783,7 +1783,7 @@ static inline void uclamp_rq_dec(struct rq *rq, struct task_struct *p)
>  	 * The condition is constructed such that a NOP is generated when
>  	 * sched_uclamp_used is disabled.
>  	 */
> -	if (!static_branch_unlikely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>  		return;
>  
>  	if (unlikely(!p->sched_class->uclamp_enabled))
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index 38e0e323dda2..f5de05354d80 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -3394,6 +3394,19 @@ static inline bool update_other_load_avgs(struct rq *rq) { return false; }
>  
>  unsigned long uclamp_eff_value(struct task_struct *p, enum uclamp_id clamp_id);
>  
> +/*
> + * When uclamp is compiled in, the aggregation at rq level is 'turned off'
> + * by default in the fast path and only gets turned on once userspace performs
> + * an operation that requires it.
> + *
> + * Returns true if userspace opted-in to use uclamp and aggregation at rq level
> + * hence is active.
> + */
> +static inline bool uclamp_is_used(void)
> +{
> +	return static_branch_likely(&sched_uclamp_used);
> +}
> +
>  static inline unsigned long uclamp_rq_get(struct rq *rq,
>  					  enum uclamp_id clamp_id)
>  {
> @@ -3417,7 +3430,7 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
>  	unsigned long rq_util;
>  	unsigned long max_util;
>  
> -	if (!static_branch_likely(&sched_uclamp_used))
> +	if (!uclamp_is_used())
>  		return false;
>  
>  	rq_util = cpu_util_cfs(cpu_of(rq)) + cpu_util_rt(rq);
> @@ -3426,19 +3439,6 @@ static inline bool uclamp_rq_is_capped(struct rq *rq)
>  	return max_util != SCHED_CAPACITY_SCALE && rq_util >= max_util;
>  }
>  
> -/*
> - * When uclamp is compiled in, the aggregation at rq level is 'turned off'
> - * by default in the fast path and only gets turned on once userspace performs
> - * an operation that requires it.
> - *
> - * Returns true if userspace opted-in to use uclamp and aggregation at rq level
> - * hence is active.
> - */
> -static inline bool uclamp_is_used(void)
> -{
> -	return static_branch_likely(&sched_uclamp_used);
> -}
> -
>  #define for_each_clamp_id(clamp_id) \
>  	for ((clamp_id) = 0; (clamp_id) < UCLAMP_CNT; (clamp_id)++)
>  

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Vincent Guittot]

On Thu, 13 Feb 2025 at 10:18, Xuewen Yan <xuewen.yan@unisoc.com> 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.

You should extended this to all calls to
static_branch_enable(&sched_uclamp_used) which are cgroups and system
wide uclamp
Android likes to play with task's uclamp but the same could happen with others

>
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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);

may be use a helper function to encapsulate this and use it everywhere
we enable sched_uclamp_used

>
>         return 0;
>  }
> --
> 2.25.1
>

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Xuewen Yan]

On Wed, Feb 19, 2025 at 1:24 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Thu, 13 Feb 2025 at 10:18, Xuewen Yan <xuewen.yan@unisoc.com> 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.
>
> You should extended this to all calls to
> static_branch_enable(&sched_uclamp_used) which are cgroups and system
> wide uclamp
> Android likes to play with task's uclamp but the same could happen with others
>
> >
> > Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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);
>
> may be use a helper function to encapsulate this and use it everywhere
> we enable sched_uclamp_used
>

Okay, thank you very much for your suggestion, I would modify it in patch-v2.

BR

> >
> >         return 0;
> >  }
> > --
> > 2.25.1
> >

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Qais Yousef]

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---

> 
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.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
> 

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Xuewen Yan]

Hi Qais,

On Sun, Feb 23, 2025 at 7:36 AM Qais Yousef <qyousef@layalina.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@unisoc.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
> >

Re: [PATCH 2/2] sched/uclamp: Add uclamp_is_used() check before enable it

[Qais Yousef]

On 02/24/25 09:55, Xuewen Yan wrote:
> Hi Qais,
> 
> On Sun, Feb 23, 2025 at 7:36 AM Qais Yousef <qyousef@layalina.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.

I can't see why this is special to uclamp because it is not disabled. The
problem is that we enable unconditionally because the logic in the jump_label
should know that this call is redundant. And that was the feedback from Peter
then as I had it exactly as you're trying to do now initially. It just it seems
we do the bail out check after holding the lock, we can replicate this
condition before holding the lock so if there are double, triple or N'ble calls
to static_key_enable() they'll just end up doing nothing without any side
effect of holding the lock. The fact that it gets disabled doesn't relate to
the problem that the static_key_enable() is being called repeatedly is what is
causing the problem here. And the solution is to not hold the lock. I think the
atomic_t variable is enough to do early bail out without holding any locks.

What are the concurrency issues you have in mind? And why the frequent disable
is a problem to _replicate_ the early bailout logic outside of the lock?

If you have in mind a concurrent enable/disable, then I'd say the calling code
has a bigger problem for doing concurrent enable/disable. This doesn't make
sense and the logic will be accidentally enabled one time and disabled other
times depending on the planet order, and moving the guard for early bail out
out of the lock wouldn't make this any worse, no?

> 
> ---
> 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/

np, I'll put a comment there too.

Thanks!

--
Qais Yousef