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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.03.2025 06:50, Penny Zheng wrote:
> We intend to introduce CONFIG_PM_STATS for wrapping all operations
> regarding performance management statistics.
> The major codes reside in xen/drivers/acpi/pmstat.c, including two main
> pm-related sysctl op: do_get_pm_info() and do_pm_op().
> So This commit also makes CONFIG_PM_STATS depend on CONFIG_SYSCTL
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v1 -> v2:
> - rename to CONFIG_PM_STATS
> - fix indention and stray semicolon
> - make code movements into a new commit
> - No need to wrap inline functions and declarations
> ---
>  xen/arch/x86/acpi/cpu_idle.c                 |  2 ++
>  xen/arch/x86/acpi/cpufreq/hwp.c              |  6 ++++++
>  xen/arch/x86/acpi/cpufreq/powernow.c         |  4 ++++
>  xen/common/Kconfig                           |  5 +++++
>  xen/common/sysctl.c                          |  4 ++--
>  xen/drivers/acpi/Makefile                    |  2 +-
>  xen/drivers/cpufreq/cpufreq_misc_governors.c |  2 ++
>  xen/drivers/cpufreq/cpufreq_ondemand.c       |  2 ++
>  xen/include/acpi/cpufreq/processor_perf.h    | 14 ++++++++++++++
>  9 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/cpu_idle.c b/xen/arch/x86/acpi/cpu_idle.c
> index 420198406d..b537ac4cd6 100644
> --- a/xen/arch/x86/acpi/cpu_idle.c
> +++ b/xen/arch/x86/acpi/cpu_idle.c
> @@ -1487,6 +1487,7 @@ static void amd_cpuidle_init(struct 
> acpi_processor_power *power)
>          vendor_override = -1;
>  }
>  
> +#ifdef CONFIG_PM_STATS
>  uint32_t pmstat_get_cx_nr(unsigned int cpu)
>  {
>      return processor_powers[cpu] ? processor_powers[cpu]->count : 0;
> @@ -1606,6 +1607,7 @@ int pmstat_reset_cx_stat(unsigned int cpu)
>  {
>      return 0;
>  }
> +#endif /* CONFIG_PM_STATS */
>  
>  void cpuidle_disable_deep_cstate(void)
>  {
> diff --git a/xen/arch/x86/acpi/cpufreq/hwp.c b/xen/arch/x86/acpi/cpufreq/hwp.c
> index d5fa3d47ca..98e9d46890 100644
> --- a/xen/arch/x86/acpi/cpufreq/hwp.c
> +++ b/xen/arch/x86/acpi/cpufreq/hwp.c
> @@ -466,6 +466,7 @@ static int cf_check hwp_cpufreq_cpu_exit(struct 
> cpufreq_policy *policy)
>      return 0;
>  }
>  
> +#ifdef CONFIG_PM_STATS
>  /*
>   * The SDM reads like turbo should be disabled with MSR_IA32_PERF_CTL and
>   * PERF_CTL_TURBO_DISENGAGE, but that does not seem to actually work, at 
> least
> @@ -508,6 +509,7 @@ static int cf_check hwp_cpufreq_update(unsigned int cpu, 
> struct cpufreq_policy *
>  
>      return per_cpu(hwp_drv_data, cpu)->ret;
>  }
> +#endif /* CONFIG_PM_STATS */
>  
>  static const struct cpufreq_driver __initconst_cf_clobber
>  hwp_cpufreq_driver = {
> @@ -516,9 +518,12 @@ hwp_cpufreq_driver = {
>      .target = hwp_cpufreq_target,
>      .init   = hwp_cpufreq_cpu_init,
>      .exit   = hwp_cpufreq_cpu_exit,
> +#ifdef CONFIG_PM_STATS
>      .update = hwp_cpufreq_update,
> +#endif
>  };

Something's wrong here: The .update hook is actually making changes, so is
definitely not (only) about statistics. Same for the powernow driver.

> +#ifdef CONFIG_PM_STATS
>  int get_hwp_para(unsigned int cpu,
>                   struct xen_cppc_para *cppc_para)
>  {
> @@ -639,6 +644,7 @@ int set_hwp_para(struct cpufreq_policy *policy,
>  
>      return hwp_cpufreq_target(policy, 0, 0);
>  }
> +#endif /* CONFIG_PM_STATS */

This also isn't about statistics, but about getting / setting parameters.

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

> --- a/xen/drivers/cpufreq/cpufreq_misc_governors.c
> +++ b/xen/drivers/cpufreq/cpufreq_misc_governors.c
> @@ -64,6 +64,7 @@ static int cf_check cpufreq_governor_userspace(
>      return ret;
>  }
>  
> +#ifdef CONFIG_PM_STATS
>  int write_userspace_scaling_setspeed(unsigned int cpu, unsigned int freq)
>  {
>      struct cpufreq_policy *policy;
> @@ -80,6 +81,7 @@ int write_userspace_scaling_setspeed(unsigned int cpu, 
> unsigned int freq)
>  
>      return __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L);
>  }
> +#endif /* CONFIG_PM_STATS */

Here the name of the function also makes pretty clear that it isn't about
statistics.

> --- a/xen/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/xen/drivers/cpufreq/cpufreq_ondemand.c
> @@ -57,6 +57,7 @@ static struct dbs_tuners {
>  
>  static DEFINE_PER_CPU(struct timer, dbs_timer);
>  
> +#ifdef CONFIG_PM_STATS
>  int write_ondemand_sampling_rate(unsigned int sampling_rate)
>  {
>      if ( (sampling_rate > MAX_SAMPLING_RATE / MICROSECS(1)) ||
> @@ -93,6 +94,7 @@ int get_cpufreq_ondemand_para(uint32_t *sampling_rate_max,
>  
>      return 0;
>  }
> +#endif /* CONFIG_PM_STATS */

Same for the ones here.

> --- a/xen/include/acpi/cpufreq/processor_perf.h
> +++ b/xen/include/acpi/cpufreq/processor_perf.h
> @@ -9,9 +9,23 @@
>  
>  unsigned int powernow_register_driver(void);
>  unsigned int get_measured_perf(unsigned int cpu, unsigned int flag);
> +#ifdef CONFIG_PM_STATS
>  void cpufreq_statistic_update(unsigned int cpu, uint8_t from, uint8_t to);
>  int  cpufreq_statistic_init(unsigned int cpu);
>  void cpufreq_statistic_exit(unsigned int cpu);
> +#else
> +static inline void cpufreq_statistic_update(unsigned int cpu, uint8_t from,
> +                                            uint8_t to)
> +{
> +}

This could do with both braces moved to the line with the closing parenthesis.

> +static inline int cpufreq_statistic_init(unsigned int cpu)
> +{
> +    return 0;
> +}
> +static inline void cpufreq_statistic_exit(unsigned int cpu)
> +{
> +}

Same here.

Jan