On 04.07.2025 10:56, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Friday, July 4, 2025 4:33 PM
>> To: Penny, Zheng <penny.zh...@amd.com>; Andryuk, Jason
>> <jason.andr...@amd.com>
>> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD
>> <anthony.per...@vates.tech>; Juergen Gross <jgr...@suse.com>; Andrew
>> Cooper <andrew.coop...@citrix.com>; Orzel, Michal <michal.or...@amd.com>;
>> Julien Grall <jul...@xen.org>; Roger Pau Monné <roger....@citrix.com>; 
>> Stefano
>> Stabellini <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
>> 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 <jbeul...@suse.com>
>>>> Sent: Tuesday, June 17, 2025 6:08 PM
>>>> To: Penny, Zheng <penny.zh...@amd.com>
>>>> Cc: Huang, Ray <ray.hu...@amd.com>; Anthony PERARD
>>>> <anthony.per...@vates.tech>; Juergen Gross <jgr...@suse.com>; Andrew
>>>> Cooper <andrew.coop...@citrix.com>; Orzel, Michal
>>>> <michal.or...@amd.com>; Julien Grall <jul...@xen.org>; Roger Pau
>>>> Monné <roger....@citrix.com>; Stefano Stabellini
>>>> <sstabell...@kernel.org>; xen-devel@lists.xenproject.org
>>>> 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

Reply via email to