Re: [PATCH v3] cpufreq: schedutil: Fix rate limit overflow
"Rafael J. Wysocki (Intel)" <[email protected]>
| Newsgroups | org.kernel.vger.linux-pm,org.kernel.vger.linux-kernel,org.kernel.vger.stable |
|---|---|
| Message-ID | <CAJZ5v0h=9nMsQU9nnYi-tVHPd7erHi-a+EEMAmTEq-ZueebZhA@mail.gmail.com> |
On Thu, Aug 6, 2026 at 4:23 PM Hui Su <[email protected]> wrote: > > rate_limit_us is an unsigned int, while NSEC_PER_USEC is defined as > 1000L. On 32-bit systems, the multiplication is therefore performed > using 32-bit unsigned arithmetic before 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 rate_limit_us to s64 before > converting it to nanoseconds. Use the helper when updating the tunable > through sysfs and when starting the governor, so both paths perform the > conversion without overflow. > > Fixes: 9bdcb44e391d ("cpufreq: schedutil: New governor based on scheduler utilization data") > Cc: [email protected] > Signed-off-by: Hui Su <[email protected]> > Reviewed-by: Zhongqiu Han <[email protected]> Applied as 7.3 material, thanks! > --- > Changes in v3: > - Add a comment explaining why rate_limit_us must be cast before > multiplication. > > v2: https://lore.kernel.org/r/[email protected] > > Changes in v2: > - Clarify why the multiplication uses 32-bit unsigned arithmetic on > 32-bit systems. > - Cast rate_limit_us to s64 to match freq_update_delay_ns. > - Add Zhongqiu's Reviewed-by tag. > > v1: https://lore.kernel.org/r/[email protected] > > kernel/sched/cpufreq_schedutil.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c > index dff4ee04694c..614ff0d33c01 100644 > --- a/kernel/sched/cpufreq_schedutil.c > +++ b/kernel/sched/cpufreq_schedutil.c > @@ -61,6 +61,17 @@ static DEFINE_PER_CPU(struct sugov_cpu, sugov_cpu); > > /************************ Governor internals ***********************/ > > +static void sugov_update_rate_limit_us(struct sugov_policy *sg_policy) > +{ > + /* > + * Cast rate_limit_us before multiplication to force 64-bit arithmetic. > + * Otherwise, on 32-bit platforms, both operands are converted to > + * 32-bit unsigned long and the multiplication may overflow. > + */ > + sg_policy->freq_update_delay_ns = > + (s64)sg_policy->tunables->rate_limit_us * NSEC_PER_USEC; > +} > + > static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time) > { > s64 delta_ns; > @@ -606,7 +617,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 +859,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; > -- > 2.43.0 >