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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Jul 12, 2023 at 4:43 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 11.07.2023 19:49, Jason Andryuk wrote:
> > 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.
>
> Maybe we need to take a step back a revisit what option forms actually
> make sense to express. Part of the problem may be that we permit (but
> not require afaics) the use of colon as a separator after the "main"
> option ("xen", "none", "dom0-kernel", and perhaps now "hwp"). Such a
> colon suggests that what follows are sub-options applicable to that
> specific "main" option, especially since what follows "xen:" can be
> more than just the governor name (and in fact no governor name is
> required - I've been using cpufreq=xen:up_threshold=40 on some of my
> systems, for example). I have to admit that I don't see a clean way
> of (largely) retaining existing behavior while at the same time
> avoiding ambiguity with your additions (and it may well be that there
> is pre-existing ambiguity as well, but the introduction of yet
> another separator [semicolon] clearly makes things worse in this
> regard, as it suggests strong grouping).
>
> Maybe we want to consider an alternative form of expressing the
> fallback. What about e.g. "cpufreq=hwp:hdc(xen:ondemand),verbose"
> (and its possible variations)?

I think I like this less.

I think the strong grouping of ';' is nice.  It clearly delineates the
first and fallback option, and it could be extended to N in the
future.  I like that if you have 2 working cpufreq= lines, you just
concatenate them together with the ';'.  If the suboptions were all a
disjoint set, I think it would be easy to pick this.

The downside is the "verbose" handling.  The strong grouping makes
verbose seemingly only apply to the specified group and not globally.
Not the best, but we can document that away.

I implemented something to only allow specifying "verbose" in only the
last ';' grouping.  It's not that bad, but seems a little weird in
light of the strong grouping since it uses the same comma separator.
Also, failing to parse "verbose" in the first part breaks the
concatenation.

A different or additional separator could be used to separate verbose,
but that seems kinda overkill and would not be backwards compatible.

Another idea could be to allow multiple cpufreq= lines.  Each one
would call setup_cpufreq_option(), which could track the entries in
cpufreq_xen_opts[].  It still doesn't show a single "verbose" applies
globally.  And it breaks "later options override earlier ones."
"cpufreqfallback=" is another option, but I don't particularly care
for that.

I think I just prefer the ';' separator and letting "verbose" be
weird.  It maps well into the existing syntax and I liked it when you
first suggested it.

Regards,
Jason