Re: [PATCH 2/2] thermal: cpufreq_cooling: Reuse effective_cpu_util()

["Rafael J. Wysocki" <rafael@kernel.org>]

On Wed, Jul 15, 2020 at 9:32 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 14-07-20, 15:05, Rafael J. Wysocki wrote:
> > On Tue, Jul 14, 2020 at 8:37 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >  static u32 get_load(struct cpufreq_cooling_device *cpufreq_cdev, int cpu,
> > >                     int cpu_idx)
> > >  {
> > > -       u32 load;
> > > -       u64 now, now_idle, delta_time, delta_idle;
> > > -       struct time_in_idle *idle_time = &cpufreq_cdev->idle_time[cpu_idx];
> > > -
> > > -       now_idle = get_cpu_idle_time(cpu, &now, 0);
> > > -       delta_idle = now_idle - idle_time->time;
> > > -       delta_time = now - idle_time->timestamp;
> > > +       unsigned long util = cpu_util_cfs(cpu_rq(cpu));
> > > +       unsigned long max = arch_scale_cpu_capacity(cpu);
> > >
> > > -       if (delta_time <= delta_idle)
> > > -               load = 0;
> > > -       else
> > > -               load = div64_u64(100 * (delta_time - delta_idle), delta_time);
> > > -
> > > -       idle_time->time = now_idle;
> > > -       idle_time->timestamp = now;
> > > -
> > > -       return load;
> > > +       util = effective_cpu_util(cpu, util, max, ENERGY_UTIL, NULL);
> >
> > Hmm.
> >
> > It doesn't look like cpufreq_cdev and cpu_idx are needed any more in
> > this function, so maybe drop them from the arg list?
>
> Right.
>
> > And then there
> > won't be anything specific to CPU cooling in this function, so maybe
> > move it to sched and export it from there properly?
>
> There isn't a lot happening in this routine right now TBH and am not
> sure if it is really worth it to have a separate routine for this
> (unless we can get rid of something for all the callers, like avoiding
> a call to arch_scale_cpu_capacity() and then naming it
> effective_cpu_load().

Maybe yes.  Or sched_cpu_load() to stand for "the effective CPU load
as seen by the scheduler".

But I'm not sure if percent is the best unit to return from it.  Maybe
make it return something like (util << SCHED_CAPACITY_SHFT) /
arch_scale_cpu_capacity(cpu).

> > Also it looks like max could be passed to it along with the CPU number
> > instead of being always taken as arch_scale_cpu_capacity(cpu).
>
> I am not sure what you are suggesting here. What will be the value of
> max if not arch_scale_cpu_capacity() ?

I was thinking about a value supplied by the caller, eg.
sched_cpu_load(cpu, max), but if all callers would pass
arch_scale_cpu_capacity(cpu) as max anyway, then it's better to simply
call it from there.