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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>>> +            {
>>> +                switch ( cpufreq_xen_opts[i] )
>>> +                {
>>> +                case CPUFREQ_xen:
>>> +                    ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>>> +                    break;
>>> +                case CPUFREQ_hwp:
>>> +                    ret = hwp_register_driver();
>>> +                    break;
>>> +                }
>>> +
>>> +                if ( ret == 0 )
>>> +                    break;
>>> +            }
>>>              break;
>>
>> In this model any kind of failure results in the fallback to be tried
>> (and the fallback's error to be returned to the caller rather than
>> the primary one). This may or may not be what we actually want;
>> personally I would have expected
>>
>>                 if ( ret != -ENODEV )
>>                     break;
>>
>> or some such instead.
> 
> I guess this comes back to our fruit preferences. :)

Does it? It's not just a style question here, but one of when / whether
to use the fallback.

> I can switch it around like that, and make hwp_register_driver()
> return -ENODEV for hwp_available() returning false.

Thanks.

>>> +int __init hwp_cmdline_parse(const char *s, const char *e)
>>> +{
>>> +    do
>>> +    {
>>> +        const char *end = strpbrk(s, ",;");
>>> +
>>> +        if ( s && !hwp_handle_option(s, end) )
>>
>> This check of s not being NULL comes too late, as strpbrk() would have
>> de-referenced it already. Considering ...
>>
>>> +        {
>>> +            printk(XENLOG_WARNING "cpufreq/hwp: option '%s' not 
>>> recognized\n",
>>> +                   s);
>>> +
>>> +            return -1;
>>> +        }
>>> +
>>> +        s = end ? ++end : end;
>>> +    } while ( s && s < e );
>>
>> ... this it probably wants to move even ahead of the loop.
> 
> I'll switch from do/while to just while and then the NULL check will
> be covered.  In practice, this function is never called with s ==
> NULL.

In which case - why not leave things largely as they are, simply dropping
the odd check of s?

>>> +    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.

Jan