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 Wednesday 18 Sep 2024 at 10:05:13 (+0800), Jie Zhan wrote:
> 
> 
> On 17/09/2024 18:36, Ionela Voinescu wrote:
> 
> ...
> 
> >>> @@ -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. 
> 
> Yeah understood. I did try minimizing duplicate code, but ended up with either
> duplicate 'get desired perf' stuff or duplicate cppc_perf_to_khz().
> 
> ...
> >>
> >>           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.
> > 
> 
> Sure, thanks for reminding this.
> 
> ...
> 
> How does this look? I think this should have the least duplicate code except for
> two cppc_perf_to_khz() calls, while keeping the logic easy to follow.
> 
> diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
> index bafa32dd375d..6070444ed098 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,11 +729,27 @@ 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;
>  }
>  
> +static int cppc_get_perf_ctrs_sample(int cpu,
> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t0,
> +                                    struct cppc_perf_fb_ctrs *fb_ctrs_t1)
> +{
> +       int ret;
> +
> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t0);
> +       if (ret)
> +               return ret;
> +
> +       udelay(2); /* 2usec delay between sampling */
> +
> +       ret = cppc_get_perf_ctrs(cpu, fb_ctrs_t1);

nit: white line before return.

> +       return ret;
> +}
> +
>  static unsigned int cppc_cpufreq_get_rate(unsigned int cpu)
>  {
>         struct cppc_perf_fb_ctrs fb_ctrs_t0 = {0}, fb_ctrs_t1 = {0};
> @@ -746,20 +765,30 @@ 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;
> +       ret = cppc_get_perf_ctrs_sample(cpu, &fb_ctrs_t0, &fb_ctrs_t1);
> +       if (ret) {
> +               if (ret == -EFAULT)
> +                       goto out_invalid_counters;
> +               else
> +                       return 0;
> +       }
>  
>         delivered_perf = cppc_perf_from_fbctrs(cpu_data, &fb_ctrs_t0,
>                                                &fb_ctrs_t1);
> +       if (!delivered_perf)
> +               goto out_invalid_counters;
>  
>         return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
> +
> +out_invalid_counters:
> +       /*
> +        * Feedback counters could be unchanged or 0 when a cpu enters a
> +        * low-power idle state, e.g. clock-gated or power-gated.
> +        * Get the lastest or cached desired perf for reflecting frequency.
> +        */
> +       if (cppc_get_desired_perf(cpu, &delivered_perf))
> +               delivered_perf = cpu_data->perf_ctrls.desired_perf;

nit: same white line before return here :).

Looks good, thanks for the changes.

Reviewed-by: Ionela Voinescu <ionela.voinescu@arm.com>

Ionela.

> +       return cppc_perf_to_khz(&cpu_data->perf_caps, delivered_perf);
>  }
>  
>  static int cppc_cpufreq_set_boost(struct cpufreq_policy *policy, int state)
> 
> 
> Thanks!
> Jie