Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.04.2025 10:38, Penny, Zheng wrote:
> [Public]
> 
> Hi,
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, April 16, 2025 2:37 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Stabellini, Stefano <stefano.stabellini@xxxxxxx>; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
>>
>> On 16.04.2025 05:54, Penny, Zheng wrote:
>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Tuesday, April 1, 2025 9:10 PM
>>>>
>>>> On 26.03.2025 06:50, Penny Zheng wrote:
>>>>> --- a/xen/common/Kconfig
>>>>> +++ b/xen/common/Kconfig
>>>>> @@ -557,4 +557,9 @@ config SYSCTL
>>>>>       to reduce Xen footprint.
>>>>>  endmenu
>>>>>
>>>>> +config PM_STATS
>>>>> +   bool "Enable Performance Management Statistics"
>>>>> +   depends on ACPI && HAS_CPUFREQ && SYSCTL
>>>>> +   default y
>>>>
>>>> As per above - either name, prompt and the description that Stefano
>>>> suggested are wrong, or it is too much that is being covered by this new
>> control.
>>>>
>>>
>>> We have two sysctl-op on performance, do_get_pm_info() and do_pm_op().
>>> I think do_get_pm_info() is to collect PM statistic info, which could
>>> be wrapped with CONFIG_PM_STATS, while maybe do_pm_op() is more
>> focusing on performance tuning.
>>> How about we introduce another Kconfig CONFIG_PM_TUNE to wrap
>>> do_pm_op() and related helpers? I suggest to introduce a new file pmtune.c 
>>> to
>> contain.
>>> Or any better suggestion?
>>
>> "tune" is too narrow imo. "ctrl" may be an option, but how about simply 
>> pm-op.c,
>> fitting do_pm_op() pretty nicely? Question is what else is going to end up 
>> in that
>> file.
>>
> 
> The following functions will be included in pm_op.c

To avoid another round trip just for this - I wrote pm-op.c for a reason.

> and wrapped with PM_CTRL or PM_OP
>     - get_cpufreq_para()
>     - set_cpufreq_para()
>     - set_cpufreq_gov()
>     - set_cpufreq_cppc()
>     - cpufreq_driver_getavg()
>     - cpufreq_update_turbo()
>     - cpufreq_get_turbo_status()
> And description for PM_STATS, I'll take stefano's suggestion and for 
> PM_CTRL/PM_OP, maybe it will be
> ```
> Enable userspace performance management control to do power/performance 
> analyzing and tuning
> ```

Reads okay at the first glance.

Jan