Re: [PATCH v3 10/20] xen/pmstat: introduce CONFIG_PM_OP

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.04.2025 09:37, Penny Zheng wrote:
> We move the following functions into a new file drivers/acpi/pm_op.c, as
> they are all more fitting in performance controling and only called by
> do_pm_op():
>  - get_cpufreq_para()
>  - set_cpufreq_para()
>  - set_cpufreq_gov()
>  - set_cpufreq_cppc()
>  - cpufreq_driver_getavg()
>  - cpufreq_update_turbo()
>  - cpufreq_get_turbo_status()
> We introduce a new Kconfig CONFIG_PM_OP to wrap the new file.
> 
> Also, although the following helpers are only called by do_pm_op(), they have
> dependency on local variable, we wrap them with CONFIG_PM_OP in place:
>  - write_userspace_scaling_setspeed()
>  - write_ondemand_sampling_rate()
>  - write_ondemand_up_threshold()
>  - get_cpufreq_ondemand_para()
>  - cpufreq_driver.update()
>  - get_hwp_para()
> Various style corrections shall be applied at the same time while moving these
> functions, including:
>  - add extra space before and after bracket of if() and switch()
>  - fix indentation
> 
> Signed-off-by: Penny Zheng <Penny.Zheng@xxxxxxx>
> ---
> v2 -> v3
> - new commit
> ---
>  xen/arch/x86/acpi/cpufreq/hwp.c              |   6 +
>  xen/arch/x86/acpi/cpufreq/powernow.c         |   4 +
>  xen/common/Kconfig                           |   7 +
>  xen/common/sysctl.c                          |   4 +-
>  xen/drivers/acpi/Makefile                    |   1 +
>  xen/drivers/acpi/pm_op.c                     | 409 +++++++++++++++++++
>  xen/drivers/acpi/pmstat.c                    | 357 ----------------
>  xen/drivers/cpufreq/cpufreq_misc_governors.c |   2 +
>  xen/drivers/cpufreq/cpufreq_ondemand.c       |   2 +
>  xen/drivers/cpufreq/utility.c                |  41 --
>  xen/include/acpi/cpufreq/cpufreq.h           |   3 -
>  11 files changed, 434 insertions(+), 402 deletions(-)
>  create mode 100644 xen/drivers/acpi/pm_op.c

I'm pretty sure I said "pm-op.c" in replies, maybe even moret than once. Now
you still used an underscore instead of the dash that's preferred.

> --- a/xen/common/sysctl.c
> +++ b/xen/common/sysctl.c
> @@ -181,13 +181,15 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xen_sysctl_t) 
> u_sysctl)
>      case XEN_SYSCTL_get_pmstat:
>          ret = do_get_pm_info(&op->u.get_pmstat);
>          break;
> +#endif
>  
> +#ifdef CONFIG_PM_OP
>      case XEN_SYSCTL_pm_op:
>          ret = do_pm_op(&op->u.pm_op);
>          if ( ret == -EAGAIN )
>              copyback = 1;
>          break;
> -#endif
> +#endif /* CONFIG_PM_OP */

Please can you be consistent here with the comment (or not) on the #endif?

> +int do_pm_op(struct xen_sysctl_pm_op *op)
> +{
> +    int ret = 0;
> +    const struct processor_pminfo *pmpt;
> +
> +    switch ( op->cmd )
> +    {
> +    case XEN_SYSCTL_pm_op_set_sched_opt_smt:
> +    {
> +        uint32_t saved_value = sched_smt_power_savings;
> +
> +        if ( op->cpuid != 0 )
> +            return -EINVAL;
> +        sched_smt_power_savings = !!op->u.set_sched_opt_smt;
> +        op->u.set_sched_opt_smt = saved_value;
> +        return 0;
> +    }
> +
> +    case XEN_SYSCTL_pm_op_get_max_cstate:
> +        BUILD_BUG_ON(XEN_SYSCTL_CX_UNLIMITED != UINT_MAX);
> +        if ( op->cpuid == 0 )
> +            op->u.get_max_cstate = acpi_get_cstate_limit();
> +        else if ( op->cpuid == 1 )
> +            op->u.get_max_cstate = acpi_get_csubstate_limit();
> +        else
> +            ret = -EINVAL;
> +        return ret;
> +
> +    case XEN_SYSCTL_pm_op_set_max_cstate:
> +        if ( op->cpuid == 0 )
> +            acpi_set_cstate_limit(op->u.set_max_cstate);
> +        else if ( op->cpuid == 1 )
> +            acpi_set_csubstate_limit(op->u.set_max_cstate);
> +        else
> +            ret = -EINVAL;
> +        return ret;
> +    }
> +
> +    if ( op->cpuid >= nr_cpu_ids || !cpu_online(op->cpuid) )
> +        return -EINVAL;
> +    pmpt = processor_pminfo[op->cpuid];
> +
> +    switch ( op->cmd & PM_PARA_CATEGORY_MASK )
> +    {
> +    case CPUFREQ_PARA:
> +        if ( !(xen_processor_pmbits & XEN_PROCESSOR_PM_PX) )
> +            return -ENODEV;
> +        if ( !pmpt || !(pmpt->perf.init & XEN_PX_INIT) )
> +            return -EINVAL;
> +        break;
> +    }
> +
> +    switch ( op->cmd )
> +    {
> +    case GET_CPUFREQ_PARA:
> +    {
> +        ret = get_cpufreq_para(op);
> +        break;
> +    }
> +
> +    case SET_CPUFREQ_GOV:
> +    {
> +        ret = set_cpufreq_gov(op);
> +        break;
> +    }
> +
> +    case SET_CPUFREQ_PARA:
> +    {
> +        ret = set_cpufreq_para(op);
> +        break;
> +    }
> +
> +    case SET_CPUFREQ_CPPC:
> +        ret = set_cpufreq_cppc(op);
> +        break;
> +
> +    case GET_CPUFREQ_AVGFREQ:
> +    {
> +        op->u.get_avgfreq = cpufreq_driver_getavg(op->cpuid, USR_GETAVG);
> +        break;
> +    }
> +
> +    case XEN_SYSCTL_pm_op_enable_turbo:
> +    {
> +        ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_ENABLED);
> +        break;
> +    }
> +
> +    case XEN_SYSCTL_pm_op_disable_turbo:
> +    {
> +        ret = cpufreq_update_turbo(op->cpuid, CPUFREQ_TURBO_DISABLED);
> +        break;
> +    }

Please can you drop all the unnecessary inner figure braces here? They hamper
readability without - imo - providing any gain at all.

With all of the adjustments:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan