Re: [PATCH v5 06/15] cpufreq: Add Hardware P-State (HWP) driver

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Tue, Jul 11, 2023 at 10:41 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 11.07.2023 16:16, Jason Andryuk wrote:
> > On Tue, Jul 11, 2023 at 4:18 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 10.07.2023 17:22, Jason Andryuk wrote:
> >>> On Mon, Jul 10, 2023 at 9:13 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>> On 06.07.2023 20:54, Jason Andryuk wrote:
> >>>>> @@ -510,6 +510,22 @@ choice of `dom0-kernel` is deprecated and not 
> >>>>> supported by all Dom0 kernels.
> >>>>>  * `<maxfreq>` and `<minfreq>` are integers which represent max and min 
> >>>>> processor frequencies
> >>>>>    respectively.
> >>>>>  * `verbose` option can be included as a string or also as 
> >>>>> `verbose=<integer>`
> >>>>> +  for `xen`.  It is a boolean for `hwp`.
> >>>>> +* `hwp` selects Hardware-Controlled Performance States (HWP) on 
> >>>>> supported Intel
> >>>>> +  hardware.  HWP is a Skylake+ feature which provides better CPU power
> >>>>> +  management.  The default is disabled.  If `hwp` is selected, but 
> >>>>> hardware
> >>>>> +  support is not available, Xen will fallback to cpufreq=xen.
> >>>>> +* `<hdc>` is a boolean to enable Hardware Duty Cycling (HDC).  HDC 
> >>>>> enables the
> >>>>> +  processor to autonomously force physical package components into 
> >>>>> idle state.
> >>>>> +  The default is enabled, but the option only applies when `hwp` is 
> >>>>> enabled.
> >>>>> +
> >>>>> +There is also support for `;`-separated fallback options:
> >>>>> +`cpufreq=hwp,verbose;xen`.  This first tries `hwp` and falls back to 
> >>>>> `xen`
> >>>>> +if unavailable.
> >>>>
> >>>> In the given example, does "verbose" also apply to the fallback case? If 
> >>>> so,
> >>>> perhaps better "cpufreq=hwp;xen,verbose", to eliminate that ambiguity?
> >>>
> >>> Yes, "verbose" is applied to both.  I can make the change.  I
> >>> mentioned it in the commit message, but I'll mention it here as well.
> >>
> >> FTAOD my earlier comment implied that the spelling form you use above
> >> should not even be accepted when parsing. I.e. it was not just about
> >> the doc aspect.
> >
> > Oh.  So what exactly do you want then?
> >
> > There is a single cpufreq_verbose variable today that is set by either
> > cpufreq=hwp,verbose or cpufreq=xen,verbose.  Is that okay, or should
> > the "xen" and "hwp" each get a separate variable?
> >
> > Do you only want to allow a single trailing "verbose" to apply to all
> > of cpufreq (cpufreq=$foo,verbose)?  Or do you want "verbose" to be
> > only valid for "xen"?  Both cpufreq_cmdline_parse() and
> > hwp_cmdline_parse() just loop over their options and don't care about
> > order, even though the documentation lists verbose last.  Would you
> > want "cpufreq=hwp,verbose,hdc" to fail to parse?
> >
> > All parsing is done upfront before knowing whether "xen" or "hwp" will
> > be used as the cpufreq driver, so there is a trickiness for
> > implementing "verbose" only for one option.  Similarly,
> > "cpufreq=hwp,invalid;xen" will try "hwp" (but not "xen")  since the
> > live variables are updated.  Even without this patch, cpufreq will be
> > configured up to an invalid parameter.
>
> Right, and I'd like to see "hwp;xen" to be treated as a "unit", with
> ",verbose" applying to whichever succeeds initializing. I don't think
> there is much point to have separate verbosity variables.

When you say "hwp;xen" as a unit, you don't mean to intermix all the
options like:
cpufreq=hwp;xen:ondemand,hdc,maxfreq=42
do you?

Because of the suboptions, I don't treat "hwp;xen" as a unit, but as
strings separated by ';'.
That allows the full selection of parameters like:
cpufreq=hwc,no-hdc;xen:ondemand,maxfreq=42,minfreq=0

This lets each respective parser handle the options it knows about.
This does duplicate "verbose" handling.  cpufreq_cmdline_parse() and
hwp_cmdline_parse() are also usable when only one of "hwp" or "xen" is
specified.

These all work:
cpufreq=xen:ondemand,verbose
cpufreq=hwp,hdc,verbose
cpurfre=hwp,hdc;xen:ondemand,verbose

To disallow "verbose" in "cpufreq=hwp,verbose;xen" would require extra
code, and setup_cpufreq_option() is already rather complicated IMO.
It's a corner case, but doesn't seem harmful to me.   Hmmm, making the
above fail parsing may be worse since it would only try "hwp" without
a fallback to "xen".

I just want to be clear on exactly what I need to implement.

Regards,
Jason