Re: [PATCH 01/13] cpufreq: Allow restricting to internal governors only

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, May 26, 2021 at 9:18 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 03.05.2021 21:27, Jason Andryuk wrote:
> > For hwp, the standard governors are not usable, and only the internal
> > one is applicable.
>
> So you say "one" here but use plural in the subject. Which one is
> it (going to be)?

hwp only uses a single governor, but this is common code.  AMD or ARM
could require their own internal governor which is why the subject say
plural.

> >  Add the cpufreq_governor_internal boolean to
> > indicate when an internal governor, like hwp-internal, will be used.
> > This is set during presmp_initcall, so that it can suppress governor
>
> DYM s/is/will be/? Afaict this is going to happen later in the series.
> Which is a good indication that such "hanging in the air" changes
> aren't necessarily the best way of splitting a set of changes, ...

In terms of the patch series, yes, "will be".  The use of "is" is
directing how to use the feature.  Yes, it is "hanging in the air",
but I was trying to explain the "why" and "how" of using it.

I was trying to split this preparatory change from the actual hwp
introduction.  I suppose it could be ordered after hwp, and the extra,
unusable governors would be advertised until then.

> > --- a/xen/drivers/cpufreq/cpufreq.c
> > +++ b/xen/drivers/cpufreq/cpufreq.c
> > @@ -57,6 +57,7 @@ struct cpufreq_dom {
> >  };
> >  static LIST_HEAD_READ_MOSTLY(cpufreq_dom_list_head);
> >
> > +bool __read_mostly cpufreq_governor_internal;
>
> ... also supported by you introducing a non-static variable without
> any consumer outside of this file (and without any producer at all).
>
> > @@ -122,6 +123,9 @@ int __init cpufreq_register_governor(struct 
> > cpufreq_governor *governor)
> >      if (!governor)
> >          return -EINVAL;
> >
> > +    if (cpufreq_governor_internal && strstr(governor->name, "internal") == 
> > NULL)
>
> I wonder whether designating any governors ending in "-internal"
> wouldn't be less prone for possible future ambiguities.

Yes, that would be good.

> > --- a/xen/include/acpi/cpufreq/cpufreq.h
> > +++ b/xen/include/acpi/cpufreq/cpufreq.h
> > @@ -115,6 +115,7 @@ extern struct cpufreq_governor cpufreq_gov_dbs;
> >  extern struct cpufreq_governor cpufreq_gov_userspace;
> >  extern struct cpufreq_governor cpufreq_gov_performance;
> >  extern struct cpufreq_governor cpufreq_gov_powersave;
> > +extern bool cpufreq_governor_internal;
>
> Please separate from the governor declarations by a blank line.

Sure.

> Sorry, all quite nit-like remarks, but still ...

It's fine.  Would a design session be useful to discuss hwp?

Regards,
Jason