On 11.07.2023 19:49, Jason Andryuk wrote:
> On Tue, Jul 11, 2023 at 10:41 AM Jan Beulich <jbeul...@suse.com> wrote:
>>
>> On 11.07.2023 16:16, Jason Andryuk wrote:
>>> On Tue, Jul 11, 2023 at 4:18 AM Jan Beulich <jbeul...@suse.com> wrote:
>>>> On 10.07.2023 17:22, Jason Andryuk wrote:
>>>>> On Mon, Jul 10, 2023 at 9:13 AM Jan Beulich <jbeul...@suse.com> 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
