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

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

Hi,

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Tuesday, April 1, 2025 9:10 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>;
> Anthony PERARD <anthony.perard@xxxxxxxxxx>; Orzel, Michal
> <Michal.Orzel@xxxxxxx>; Julien Grall <julien@xxxxxxx>; Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 10/19] xen/sysctl: introduce CONFIG_PM_STATS
>
> 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.
>

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?

>
> Jan