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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> FYI, cpufreq=xen;hwp will be accepted.  "xen" shouldn't fail, so it
> doesn't make sense to specify that.  But it didn't seem necessary to
> prevent it.

Sure, that's fine.

>>>>> +    if ( data->curr_req.raw == -1 )
>>>>> +    {
>>>>> +        hwp_err(cpu, "Could not initialize HWP properly\n");
>>>>> +        per_cpu(hwp_drv_data, cpu) = NULL;
>>>>> +        xfree(data);
>>>>> +        return -ENODEV;
>>>>> +    }
>>>>> +
>>>>> +    data->minimum = data->curr_req.min_perf;
>>>>> +    data->maximum = data->curr_req.max_perf;
>>>>> +    data->desired = data->curr_req.desired;
>>>>> +    data->energy_perf = data->curr_req.energy_perf;
>>>>> +    data->activity_window = data->curr_req.activity_window;
>>>>> +
>>>>> +    if ( cpu == 0 )
>>>>> +        hwp_verbose("CPU%u: HWP_CAPABILITIES: %016lx\n", cpu, 
>>>>> data->hwp_caps);
>>>>
>>>> While I'm fine with this (perhaps apart from you using "cpu == 0",
>>>> which is an idiom we're trying to get rid of), ...
>>>
>>> Oh, I didn't know that.  What is the preferred way to identify the
>>> BSP?
>>
>> Sometimes we pass a separate boolean to functions, in other cases we
>> check whether a struct cpuinfo_x86 * equals &boot_cpu_info. The
>> latter clearly can't be used here, and the former doesn't look to be
>> a good fit either. However, ...
>>
>>>  This doesn't necessarily run on the BSP, so "cpu"/"policy->cpu"
>>> is all we have to make a determination.
>>
>> ... isn't it, conversely, the case that the function only ever runs
>> on "cpu" when it is the BSP? In which case "cpu == smp_processor_id()"
>> ought to do the trick.
> 
> The calls do not necessarily run from the BSP.  The cpufreq init
> callbacks run later when dom0 uploads the ACPI processor data.

Oh, of course. How did I manage to forget?

>  If you
> don't want "cpu == 0", maybe just print for the first CPU regardless
> of number, and then print differences from that?

Perhaps best then.

Jan