Re: [PATCH v2 1/3] cppc_cpufreq: Return desired perf in ->get() if feedback counters are 0

[Ionela Voinescu <ionela.voinescu@arm.com>]

Hi,

On Friday 13 Sep 2024 at 20:05:50 (+0800), Jie Zhan wrote:
> 
> Hi Ionela,
> 
> On 12/09/2024 17:43, Ionela Voinescu wrote:
> 
> ...
> 
> > 
> > A possible (slimmer) alternative implementation for you to consider
> > (this merges patches 1 & 2):
> > 
> > diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> > index bafa32dd375d..c16be9651a6f 100644
> > --- a/drivers/cpufreq/cppc_cpufreq.c
> > +++ b/drivers/cpufreq/cppc_cpufreq.c
> > @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
> > 
> >         perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
> >                                      &fb_ctrs);
> > +       if (!perf)
> > +               perf = cpu_data->perf_ctrls.desired_perf;
> > +
> 
> I think it's better to just return here.
> If feedback counters are successfully read but unchanged, the following
> calculation and update in cppc_scale_freq_workfn() is meaningless because it
> won't change anything.

Agreed!

> 
> >         cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
> > 
> >         perf <<= SCHED_CAPACITY_SHIFT;
> > @@ -726,7 +729,7 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
> > 
> >         /* Check to avoid divide-by zero and invalid delivered_perf */
> >         if (!delta_reference || !delta_delivered)
> > -               return cpu_data->perf_ctrls.desired_perf;
> > +               return 0;
> 
> This makes sense to me.
> Here is probably why Patch 2 looks bulky.
> 
> > 
> >         return (reference_perf * delta_delivered) / delta_reference;
> >  }
> > @@ -736,7 +739,7 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> >         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> >         struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> >         struct cppc_cpudata *cpu_data;
> > -       u64 delivered_perf;
> > +       u64 delivered_perf = 0;
> >         int ret;
> > 
> >         if (!policy)
> > @@ -747,19 +750,22 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> >         cpufreq_cpu_put(policy);
> > 
> >         ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
> > -       if (ret)
> > -               return 0;
> > -
> > -       udelay(2); /* 2usec delay between sampling */
> > -
> > -       ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> > -       if (ret)
> > -               return 0;
> > +       if (!ret) {
> > +               udelay(2); /* 2usec delay between sampling */
> > +               ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
> > +       }
> > +       if (!ret)
> > +               delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> > +                                                      &fb_ctrs_t1);
> 
> TBH, 'if (!ret)' style looks very strange to me.
> We haven't done so anywhere in cppc_cpufreq, so let's keep consistency and make
> it easier for people to read and maintain?

I agree it's a bit of a difficult read, that's why I only sent my code
as a suggestion. I did like the benefit of not having to have two
different calls to cppc_perf_to_khz() and making the code below common
for the error and non-error paths. But it's up to you. 
> 
> > +       if ((ret == -EFAULT) || !delivered_perf) {
> > +               if (cppc_get_desired_perf(cpu, &delivered_perf))
> > +                       delivered_perf = cpu_data->perf_ctrls.desired_perf;
> 
> will take this.
> 
> > +       }
> > 
> > -       delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
> > -                                              &fb_ctrs_t1);
> > +       if (delivered_perf)
> > +               return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> > 
> > -       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> > +       return 0;
> >  }
> > 
> > disclaimer: not fully checked so likely not "production ready" code :)
> > 
> > Hope it helps,
> > Ionela.
> > 
> >>  
> >>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> >> -- 
> >> 2.33.0
> >>
> > 
> 
> How about this? merged patch 1 & 2 as well.
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..411303f2e8cb 100644
> --- a/drivers/cpufreq/cppc_cpufreq.c
> +++ b/drivers/cpufreq/cppc_cpufreq.c
> @@ -118,6 +118,9 @@ static void cppc_scale_freq_workfn(struct kthread_work *work)
> 
>           perf = cppc_perf_from_fbctrs(cpu_data, &cppc_fi->prev_perf_fb_ctrs,
>                                        &fb_ctrs);
> +       if (!perf)
> +               return;
> +
>           cppc_fi->prev_perf_fb_ctrs = fb_ctrs;
> 
>           perf <<= SCHED_CAPACITY_SHIFT;
> @@ -726,7 +729,7 @@ static int cppc_perf_from_fbctrs(struct cppc_cpudata *cpu_data,
> 
>           /* Check to avoid divide-by zero and invalid delivered_perf */
>           if (!delta_reference || !delta_delivered)
> -               return cpu_data->perf_ctrls.desired_perf;
> +               return 0;
> 
>           return (reference_perf * delta_delivered) / delta_reference;
>    }
> @@ -748,18 +751,32 @@ static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
> 
>           ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t0);
>           if (ret)
> -               return 0;
> +               goto out_err;
> 
>           udelay(2); /* 2usec delay between sampling */
> 
>           ret = cppc_get_perf_ctrs(cpu, &fb_ctrs_t1);
>           if (ret)
> -               return 0;
> +               goto out_err;
> 
>           delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>                                                  &fb_ctrs_t1);

You need a check here for !delivered_perf (when at least one of the
deltas is 0) in which case it would be good to take the same error path
below. Something like:

            if(delivered_perf)
	            return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
	    else
		ret = -EFAULT;

That's why I did the tricky if/else dance above as we need to take the
error path below for multiple cases.

Thanks,
Ionela.

> 
>           return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +
> +out_err:
> +       /*
> +        * Feedback counters could be 0 when cores are powered down.
> +        * Take desired perf for reflecting frequency in this case.
> +        */
> +       if (ret == -EFAULT) {
> +               if (cppc_get_desired_perf(cpu, &delivered_perf))
> +                       delivered_perf = cpu_data->perf_ctrls.desired_perf;
> +
> +               return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +       }
> +
> +       return 0;
>    }
> 
>    static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> ---
> 
> Thanks indeed!
> Jie