* Re: [PATCH] cpufreq: schedutil: Fix rate limit overflow
2026-08-05 14:39 [PATCH] cpufreq: schedutil: Fix rate limit overflow Hui Su
@ 2026-08-06 6:17 ` Zhongqiu Han
0 siblings, 0 replies; 2+ messages in thread
From: Zhongqiu Han @ 2026-08-06 6:17 UTC (permalink / raw)
To: Hui Su, rafael, viresh.kumar
Cc: linux-pm, mingo, peterz, linux-kernel, stable, zhongqiu.han
On 8/5/2026 10:39 PM, Hui Su wrote:
> rate_limit_us is unsigned int. On 32-bit systems, multiplying it by
> NSEC_PER_USEC is therefore carried out in 32-bit arithmetic before
The real issue is that NSEC_PER_USEC is a long, which is only 32 bits on
32-bit systems. By contrast, unsigned int is 32 bits on both 32-bit and
64-bit platforms.
> the result is assigned to freq_update_delay_ns.
>
> For example, writing 4294968 to rate_limit_us wraps the delay from
> 4294968000 ns to 704 ns. This makes schedutil update far more often
> than configured.
>
> Add sugov_update_rate_limit_us() to widen the value to u64 before
> converting units. Use it when updating the tunable through sysfs and
> when starting the governor, so both paths set the same valid delay.
>
> Fixes: 9bdcb44e391d ("cpufreq: schedutil: New governor based on scheduler utilization data")
> Signed-off-by: Hui Su <sh_def@163.com>
>
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index dff4ee04694c2b90301ed0fa158a509a3423cee1..8bf7a2c34b9f200d8f5776205ea2a7f495e04e69 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -61,6 +61,12 @@ static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu);
>
> /************************ Governor internals ***********************/
>
> +static void sugov_update_rate_limit_us(struct sugov_policy *sg_policy)
> +{
> + sg_policy->freq_update_delay_ns =
> + (u64)sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
Small nit: better to use (s64) to match the type of
freq_update_delay_ns.
Besides, sashiko report one torn read issue,
https://sashiko.dev/#/patchset/20260805143942.805176-1-sh_def%40163.com
On 32-bit systems, the torn-read mechanism itself is pre-existing and
not introduced by this patch. However, this patch does change
its impact: the main difference is that, prior to this patch, the upper
32 bits were effectively always zero. After this change, a previously
written large value(In practice, configuring rate_limit_us to a
very large value, especially one exceeding 4,294,968 us (about 4.29
seconds), is neither reasonable nor common.) may leave non zero upper 32
bits, allowing sugov_should_update_freq() to theoretically observe a
mixed value (old upper 32 bits + new lower 32 bits) and temporarily
treat freq_update_delay_ns as much larger than intended. This could
cause one frequency update to be skipped, but since
cpufreq_update_util() runs in a hot path, the next invocation would be
expected to observe the correct value. Using atomic64 or a lock on this
hot path would be more costly than it's worth. Based on that, I think we
can keep this torn read as is. Please correct me if anyone disagrees.
With the commit log updated,
Reviewed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> +}
> +
> static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
> {
> s64 delta_ns;
> @@ -606,7 +612,7 @@ rate_limit_us_store(struct gov_attr_set *attr_set, const char *buf, size_t count
> tunables->rate_limit_us = rate_limit_us;
>
> list_for_each_entry(sg_policy, &attr_set->policy_list, tunables_hook)
> - sg_policy->freq_update_delay_ns = rate_limit_us * NSEC_PER_USEC;
> + sugov_update_rate_limit_us(sg_policy);
>
> return count;
> }
> @@ -848,7 +854,7 @@ static int sugov_start(struct cpufreq_policy *policy)
> void (*uu)(struct update_util_data *data, u64 time, unsigned int flags);
> unsigned int cpu;
>
> - sg_policy->freq_update_delay_ns = sg_policy->tunables->rate_limit_us * NSEC_PER_USEC;
> + sugov_update_rate_limit_us(sg_policy);
> sg_policy->last_freq_update_time = 0;
> sg_policy->next_freq = 0;
> sg_policy->work_in_progress = false;
--
Thx and BRs,
Zhongqiu Han
^ permalink raw reply [flat|nested] 2+ messages in thread