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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)?

Jan