Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-cmd

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.07.2025 10:32, Jan Beulich wrote:
> On 04.07.2025 10:13, Penny, Zheng wrote:
>>> -----Original Message-----
>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>
>>> On 27.05.2025 10:48, Penny Zheng wrote:
>>>> --- a/tools/misc/xenpm.c
>>>> +++ b/tools/misc/xenpm.c
>>>> @@ -69,6 +69,7 @@ void show_help(void)
>>>>              " set-max-cstate        <num>|'unlimited' 
>>>> [<num2>|'unlimited']\n"
>>>>              "                                     set the C-State 
>>>> limitation (<num> >= 0) and\n"
>>>>              "                                     optionally the 
>>>> C-sub-state limitation
>>> (<num2> >= 0)\n"
>>>> +            " get-cpufreq-cppc      [cpuid]       list cpu cppc parameter 
>>>> of CPU
>>> <cpuid> or all\n"
>>>>              " set-cpufreq-cppc      [cpuid] 
>>>> [balance|performance|powersave]
>>> <param:val>*\n"
>>>>              "                                     set Hardware P-State 
>>>> (HWP) parameters\n"
>>>>              "                                     on CPU <cpuid> or all 
>>>> if omitted.\n"
>>>> @@ -812,33 +813,7 @@ static void print_cpufreq_para(int cpuid, struct
>>>> xc_get_cpufreq_para *p_cpufreq)
>>>>
>>>>      printf("scaling_driver       : %s\n", p_cpufreq->scaling_driver);
>>>>
>>>> -    if ( hwp )
>>>> -    {
>>>> -        const xc_cppc_para_t *cppc = &p_cpufreq->u.cppc_para;
>>>> -
>>>> -        printf("cppc variables       :\n");
>>>> -        printf("  hardware limits    : lowest [%"PRIu32"] lowest nonlinear
>>> [%"PRIu32"]\n",
>>>> -               cppc->lowest, cppc->lowest_nonlinear);
>>>> -        printf("                     : nominal [%"PRIu32"] highest 
>>>> [%"PRIu32"]\n",
>>>> -               cppc->nominal, cppc->highest);
>>>> -        printf("  configured limits  : min [%"PRIu32"] max [%"PRIu32"] 
>>>> energy perf
>>> [%"PRIu32"]\n",
>>>> -               cppc->minimum, cppc->maximum, cppc->energy_perf);
>>>> -
>>>> -        if ( cppc->features & XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW )
>>>> -        {
>>>> -            unsigned int activity_window;
>>>> -            const char *units;
>>>> -
>>>> -            activity_window = calculate_activity_window(cppc, &units);
>>>> -            printf("                     : activity_window [%"PRIu32" 
>>>> %s]\n",
>>>> -                   activity_window, units);
>>>> -        }
>>>> -
>>>> -        printf("                     : desired [%"PRIu32"%s]\n",
>>>> -               cppc->desired,
>>>> -               cppc->desired ? "" : " hw autonomous");
>>>> -    }
>>>> -    else
>>>> +    if ( !hwp )
>>>>      {
>>>>          if ( p_cpufreq->gov_num )
>>>>              printf("scaling_avail_gov    : %s\n",
>>>
>>> I'm not sure it is a good idea to alter what is being output for 
>>> get-cpufreq-para.
>>> People may simply miss that output then, without having any indication 
>>> where it
>>> went.
>>
>> Hwp is more like amd-cppc in active mode. It also does not rely on Xen 
>> governor to do performance tuning, so in previous design, we could borrow 
>> governor filed to output cppc info
>> However after introducing amd-cppc passive mode, we have request to output 
>> both governor and CPPC info. And if continuing expanding get-cpufreq-para to 
>> include CPPC info, it will make the parent stuct xen_sysctl.u exceed exceed 
>> 128 bytes.
> 
> How is the xenpm command "get-cpufreq-para" related to the sysctl interface 
> struct
> size? If you need to invoke two sysctl sub-ops to produce all relevant
> "get-cpufreq-para" output, so be it I would say.
> 
>> So I'm left to create a new subcmd to specifically deal with CPPC info
>> I could leave above output for get-cpufreq-para unchanged. Then we will have 
>> duplicate CPPC info in two commands. Or is it fine to do that?
> 
> Duplicate information (in distinct commands) isn't a problem either, imo.

But, ftaod, duplicate code producing the same information is. Such code would
want breaking out into a helper function then. (And yet further ftaod, if the
code only produces identical information, but from different input structures,
such breaking out of course wouldn't normally be an option.)

Jan