On 05.05.2023 17:35, Jason Andryuk wrote:
> On Fri, May 5, 2023 at 3:01 AM Jan Beulich <jbeul...@suse.com> wrote:
>>
>> On 04.05.2023 18:56, Jason Andryuk wrote:
>>> On Thu, May 4, 2023 at 9:11 AM Jan Beulich <jbeul...@suse.com> 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.
> 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.
> I'm still investigating, but promoting hwp to a top level option -
> cpufreq=hwp - might be a better arrangement.
Might be an alternative, yes.
Jan
