Re: [PATCH] cpufreq: schedutil: Serialize start against rate limit updates

[Zhongqiu Han]

On 9/29/2026 6:36 PM, Hui Su wrote:
> rate_limit_us_store() is called with the governor attribute set
> update_lock held, but sugov_start() updates freq_update_delay_ns from
> the same tunable without taking that lock.
>
> This allows the two paths to interleave as follows:
>
>          sugov_start()             rate_limit_us_store()
>
>          read old rate_limit_us
>                                    write new rate_limit_us
>                                    publish the new delay
>          publish the old delay
>
> The sysfs tunable then contains the new value while
> freq_update_delay_ns contains the old one. Unlike a transient torn
> read, the stale delay remains in effect until another rate limit update
> or governor restart.
>
> Serialize sugov_start() against sysfs stores by taking the existing
> governor attribute set update_lock around sugov_update_rate_limit_us().
> This keeps the fix off the scheduler hot path.
>
> Fixes: 9bdcb44e391d ("cpufreq: schedutil: New governor based on scheduler utilization data")

The Fixes tag is accurate, but the patch cannot be cleanly backported
because the code has since been refactored.

> Signed-off-by: Hui Su <sh_def@xxxxxxx>

Reviewed-by: Zhongqiu Han <zhongqiu.han@xxxxxxxxxxxxxxxx>

> ---
> kernel/sched/cpufreq_schedutil.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 49ccd6f1c185..e759f09be638 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -861,7 +861,11 @@ static int sugov_start(struct cpufreq_policy *policy)
>   	void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
>   	unsigned int cpu;
>   
> +	/* Serialize against rate_limit_us_store(), which holds this lock. */
> +	mutex_lock(&sg_policy->tunables->attr_set.update_lock);

Nit: Better to wrap this in a helper, e.g.gov_attr_set_lock().

Besides, When I started reviewing this patch, I initially considered
removing the freq_update_delay_ns cache for a lockless design. However,
since it is on a hot path, doing the multiplication every time could
hurt performance.

>   	sugov_update_rate_limit_us(sg_policy);
> +	mutex_unlock(&sg_policy->tunables->attr_set.update_lock);
> +
>   	sg_policy->last_freq_update_time	= 0;
>   	sg_policy->next_freq			= 0;
>   	sg_policy->work_in_progress		= false;
>
> base-commit: 63de2367f6d280904a9c5e5bbc7c75bb91c5b3d8


--
Thx and BRs,
Zhongqiu Han