Re: [RFC PATCH 1/2] thermal/cpufreq_cooling: remove unused cpu_idx in get_load()

[Lukasz Luba]

Hi Viresh,

On 3/23/26 05:34, Viresh Kumar wrote:
> On 20-03-26, 12:32, Lukasz Luba wrote:
> > Hi Xuewen,
> >
> > On 3/20/26 11:31, Xuewen Yan wrote:
> > > From: Di Shen <di.shen@xxxxxxxxxx>
> > >
> > > The cpu_idx variable in the get_load function is now
> > > unused and can be safely removed.
> > >
> > > No code logic is affected.
> > >
> > > Signed-off-by: Di Shen <di.shen@xxxxxxxxxx>
> > > ---
> > >    drivers/thermal/cpufreq_cooling.c | 13 +++++--------
> > >    1 file changed, 5 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/thermal/cpufreq_cooling.c b/drivers/thermal/cpufreq_cooling.c
> > > index 32bf5ab44f4a..d030dbeb2973 100644
> > > --- a/drivers/thermal/cpufreq_cooling.c
> > > +++ b/drivers/thermal/cpufreq_cooling.c
> > > @@ -151,26 +151,23 @@ static u32 cpu_power_to_freq(struct cpufreq_cooling_device *cpufreq_cdev,
> > >     * get_load() - get load for a cpu
> > >     * @cpufreq_cdev: struct cpufreq_cooling_device for the cpu
> > >     * @cpu: cpu number
> > > - * @cpu_idx: index of the cpu in time_in_idle array
> > >     *
> > >     * Return: The average load of cpu @cpu in percentage since this
> > >     * function was last called.
> > >     */
> > >    #ifdef CONFIG_SMP
> > > -static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
> > > -		    int cpu_idx)
> > > +static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu)
> > >    {
> > >    	unsigned long util = sched_cpu_util(cpu);
> > >    	return (util * 100) / arch_scale_cpu_capacity(cpu);
> > >    }
> > >    #else /* !CONFIG_SMP */
> > > -static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
> > > -		    int cpu_idx)
> > > +static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu)
> > >    {
> > >    	u32 load;
> > >    	u64 now, now_idle, delta_time, delta_idle;
> > > -	struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu_idx];
> > > +	struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu];
> >
> > This is a bug. We allocate 'num_cpus' size of array based on
> > number of CPU in the cpumask for a given cpufreq policy.
> > If there are 4 cpus in the CPU cluster but CPUs have ids:
> > CPU4-7 then accessing it with this change would explode.
>
> I think following commit introduced a bug by removing `i++`.
>
> commit 3f7ced7ac9af ("drivers/thermal/cpufreq_cooling : Refactor thermal_power_cpu_get_power tracing")

Thanks for monitoring the development (it's always good
to have extra engineer opinion)!

I've checked the commit that you referred to and the 'i++' there.
It's safe. That commit removed the heavy operation for only
tracing purpose, namely:
- allocate buffer for N CPUs for 'load_cpu' pointer
- populate CPUs' load from the idle fwk
- put that info into the trace
- free the 'load_cpu' buffer

That has been redesigned since it was just for tracing
and introducing extra time spent for code run in the
throttling phase.

The code in get_load() is OK with the commit that you
mentioned.

Regards,
Lukasz