Re: [PATCH v3 04/14 RESEND] cpufreq: Add Hardware P-State (HWP) driver

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Mon, May 8, 2023 at 2:33 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 05.05.2023 17:35, Jason Andryuk wrote:
> > On Fri, May 5, 2023 at 3:01 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>
> >> On 04.05.2023 18:56, Jason Andryuk wrote:
> >>> On Thu, May 4, 2023 at 9:11 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 01.05.2023 21:30, Jason Andryuk wrote:
> >>>>> --- a/docs/misc/xen-command-line.pandoc
> >>>>> +++ b/docs/misc/xen-command-line.pandoc
> >>>>> @@ -499,7 +499,7 @@ If set, force use of the performance counters for 
> >>>>> oprofile, rather than detectin
> >>>>>  available support.
> >>>>>
> >>>>>  ### cpufreq
> >>>>> -> `= none | {{ <boolean> | xen } 
> >>>>> [:[powersave|performance|ondemand|userspace][,<maxfreq>][,[<minfreq>][,[verbose]]]]}
> >>>>>  | dom0-kernel`
> >>>>> +> `= none | {{ <boolean> | xen } 
> >>>>> [:[powersave|performance|ondemand|userspace][,<hdc>][,[<hwp>]][,[<maxfreq>]][,[<minfreq>]][,[verbose]]]}
> >>>>>  | dom0-kernel`
> >>>>
> >>>> Considering you use a special internal governor, the 4 governor 
> >>>> alternatives are
> >>>> meaningless for hwp. Hence at the command line level recognizing "hwp" 
> >>>> as if it
> >>>> was another governor name would seem better to me. This would then also 
> >>>> get rid
> >>>> of one of the two special "no-" prefix parsing cases (which I'm not 
> >>>> overly
> >>>> happy about).
> >>>>
> >>>> Even if not done that way I'm puzzled by the way you spell out the 
> >>>> interaction
> >>>> of "hwp" and "hdc": As you say in the description, "hdc" is meaningful 
> >>>> only when
> >>>> "hwp" was specified, so even if not merged with the governors group 
> >>>> "hwp" should
> >>>> come first, and "hdc" ought to be rejected if "hwp" wasn't first 
> >>>> specified. (The
> >>>> way you've spelled it out it actually looks to be kind of the other way 
> >>>> around.)
> >>>
> >>> I placed them in alphabetical order, but, yes, it doesn't make sense.
> >>>
> >>>> Strictly speaking "maxfreq" and "minfreq" also should be objected to 
> >>>> when "hwp"
> >>>> was specified.
> >>>>
> >>>> Overall I'm getting the impression that beyond your "verbose" related 
> >>>> adjustment
> >>>> more is needed, if you're meaning to get things closer to how we parse 
> >>>> the
> >>>> option (splitting across multiple lines to help see what I mean):
> >>>>
> >>>> `= none
> >>>>  | {{ <boolean> | xen } [:{powersave|performance|ondemand|userspace}
> >>>>                           
> >>>> [{,hwp[,hdc]|[,maxfreq=<maxfreq>[,minfreq=<minfreq>]}]
> >>>>                           [,verbose]]}
> >>>>  | dom0-kernel`
> >>>>
> >>>> (We're still parsing in a more relaxed way, e.g. minfreq may come ahead 
> >>>> of
> >>>> maxfreq, but better be more tight in the doc than too relaxed.)
> >>>>
> >>>> Furthermore while max/min freq don't apply directly, there are still two 
> >>>> MSRs
> >>>> controlling bounds at the package and logical processor levels.
> >>>
> >>> Well, we only program the logical processor level MSRs because we
> >>> don't have a good idea of the packages to know when we can skip
> >>> writing an MSR.
> >>>
> >>> How about this:
> >>> `= none
> >>>  | {{ <boolean> | xen } {
> >>> [:{powersave|performance|ondemand|userspace}[,maxfreq=<maxfreq>[,minfreq=<minfreq>]]
> >>>                         | [:hwp[,hdc]] }
> >>>                           [,verbose]]}
> >>>  | dom0-kernel`
> >>
> >> Looks right, yes.
> >
> > There is a wrinkle to using the hwp governor.  The hwp governor was
> > named "hwp-internal", so it needs to be renamed to "hwp" for use with
> > command line parsing.  That means the checking for "-internal" needs
> > to change to just "hwp" which removes the generality of the original
> > implementation.
>
> I'm afraid I don't see why this would strictly be necessary or a
> consequence.

Maybe I took your comment too far when you mentioned using hwp as a
fake governor.  I used the actual HWP struct cpufreq_governor to hook
into cpufreq_cmdline_parse().  cpufreq_cmdline_parse() uses the that
name for comparison.  But the naming stops being an issue if struct
cpufreq_governor gains a bool .internal flag.  That flag also makes
the registration checks clearer.

> > The other issue is that if you select "hwp" as the governor, but HWP
> > hardware support is not available, then hwp_available() needs to reset
> > the governor back to the default.  This feels like a layering
> > violation.
>
> Layering violation - yes. But why would the governor need resetting in
> this case? If HWP was asked for but isn't available, I don't think any
> other cpufreq handling (and hence governor) should be put in place.
> And turning off cpufreq altogether (if necessary in the first place)
> wouldn't, to me, feel as much like a layering violation.

My goal was for Xen to use HWP if available and fallback to the acpi
cpufreq driver if not.  That to me seems more user-friendly than
disabling cpufreq.

            if ( hwp_available() )
                ret = hwp_register_driver();
            else
                ret = cpufreq_register_driver(&acpi_cpufreq_driver);

If we are setting cpufreq_opt_governor to enter hwp_available(), but
then HWP isn't available, it seems to me that we need to reset
cpufreq_opt_governor when exiting hwp_available() false.

Regards,
Jason