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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.07.2025 10:56, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Friday, July 4, 2025 4:33 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>; Andryuk, Jason
>> <Jason.Andryuk@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
>> <anthony.perard@xxxxxxxxxx>; Juergen Gross <jgross@xxxxxxxx>; Andrew
>> Cooper <andrew.cooper3@xxxxxxxxxx>; Orzel, Michal <Michal.Orzel@xxxxxxx>;
>> Julien Grall <julien@xxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>; 
>> Stefano
>> Stabellini <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC sub-
>> cmd
>>
>> On 04.07.2025 10:13, Penny, Zheng wrote:
>>> [Public]
>>>
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>>>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Anthony PERARD
>>>> <anthony.perard@xxxxxxxxxx>; Juergen Gross <jgross@xxxxxxxx>; Andrew
>>>> Cooper <andrew.cooper3@xxxxxxxxxx>; Orzel, Michal
>>>> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Roger Pau
>>>> Monné <roger.pau@xxxxxxxxxx>; Stefano Stabellini
>>>> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>>>> Subject: Re: [PATCH v5 14/18] xen/cpufreq: introduce GET_CPUFREQ_CPPC
>>>> sub- cmd
>>>>
>>>> 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.
>>
> 
> Because we are limiting each sysctl-subcmd-struct, such as " struct 
> xen_sysctl_pm_op ", 128 bytes in "struct xen_sysctl"，They are all combined as 
> a union.
> Also, in "struct xen_sysctl_pm_op", its descending sub-op structs, including 
> "struct xen_get_cpufreq_para", are all combined as union too
> ```
> struct xen_sysctl_pm_op {
>     ... ...
>     union {
>         struct xen_get_cpufreq_para get_para;
>         struct xen_set_cpufreq_gov  set_gov;
>         struct xen_set_cpufreq_para set_para;
>         struct xen_set_cppc_para    set_cppc;
>         uint64_aligned_t get_avgfreq;
>         uint32_t                    set_sched_opt_smt;
> #define XEN_SYSCTL_CX_UNLIMITED 0xffffffffU
>         uint32_t                    get_max_cstate;
>         uint32_t                    set_max_cstate;
>     } u;
> }
> ```
> It could deduce that "struct xen_get_cpufreq_para" is limited to 128 bytes (I 
> think actual limit is smaller than 128)....

And that implies what? In my earlier reply I already said that you may then
simply need to invoke more than one sysctl to get all the data you need. (As
one of the options, that is.)

Jan