Re: [PATCH 06/13] cpufreq: Export HWP parameters to userspace

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, May 27, 2021 at 4:03 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 03.05.2021 21:28, Jason Andryuk wrote:
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -290,6 +290,12 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op 
> > *op)
> >              &op->u.get_para.u.ondemand.sampling_rate,
> >              &op->u.get_para.u.ondemand.up_threshold);
> >      }
> > +
> > +    if ( !strncasecmp(op->u.get_para.scaling_governor,
> > +                      "hwp-internal", CPUFREQ_NAME_LEN) )
> > +    {
> > +        ret = get_hwp_para(policy, &op->u.get_para.u.hwp_para);
> > +    }
> >      op->u.get_para.turbo_enabled = cpufreq_get_turbo_status(op->cpuid);
>
> Nit: Unnecessary parentheses again, and with the leading blank line
> you also want a trailing one. (As an aside I'm also not overly happy
> to see the call keyed to the governor name. Is there really no other
> indication that hwp is in use?)

This is preceded by similar checks for "userspace" and "ondemand", so
it is following existing code.  Unlike other governors, hwp-internal
is static.  It could be exported if you want to switch to comparing
with cpufreq_driver.

> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -246,4 +246,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, 
> > unsigned int freq);
> >  void cpufreq_dbs_timer_suspend(void);
> >  void cpufreq_dbs_timer_resume(void);
> >
> > +/********************** hwp hypercall helper *************************/
> > +int get_hwp_para(struct cpufreq_policy *policy, struct xen_hwp_para 
> > *hwp_para);
>
> While I can see that the excessive number of stars matches what
> we have elsewhere in the header, I still wonder if you need to go
> this far for a single declaration. If you want to stick to this,
> then to match the rest of the file you want to follow the comment
> by a blank line.

Will remove.

> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -301,6 +301,23 @@ struct xen_ondemand {
> >      uint32_t up_threshold;
> >  };
> >
> > +struct xen_hwp_para {
> > +    uint16_t activity_window; /* 7bit mantissa and 3bit exponent */
>
> If you go this far with commenting, you should also make the further
> aspects clear: Which bits these are, and that the exponent is taking
> 10 as the base (in most other cases one would expect 2).

Yes, this is much more useful.

> > +#define XEN_SYSCTL_HWP_FEAT_ENERGY_PERF (1 << 0) /* energy_perf range 
> > 0-255 if
> > +                                                    1. Otherwise 0-15 */
> > +#define XEN_SYSCTL_HWP_FEAT_ACT_WINDOW  (1 << 1) /* activity_window 
> > supported
> > +                                                    if 1 */
>
> Style: Comment formatting. You may want to move the comment on separate
> lines ahead of what they comment.
>
> > +    uint8_t hw_feature; /* bit flags for features */
> > +    uint8_t hw_lowest;
> > +    uint8_t hw_most_efficient;
> > +    uint8_t hw_guaranteed;
> > +    uint8_t hw_highest;
>
> Any particular reason for the recurring hw_ prefixes?

The idea was to differentiate values provided by CPU hardware from
user-configured values.

Regards,
Jason