Re: [PATCH v7 1/5] power: refactor core power management library

["lihuisong (C)" <lihuisong@huawei.com>]

Hi Sivaprasad,

Some comments inline.

在 2024/10/21 12:07, Sivaprasad Tummala 写道:
> This patch introduces a comprehensive refactor to the core power
> management library. The primary focus is on improving modularity
> and organization by relocating specific driver implementations
> from the 'lib/power' directory to dedicated directories within
> 'drivers/power/core/*'. The adjustment of meson.build files
> enables the selective activation of individual drivers.
>
> These changes contribute to a significant enhancement in code
> organization, providing a clearer structure for driver implementations.
> The refactor aims to improve overall code clarity and boost
> maintainability. Additionally, it establishes a foundation for
> future development, allowing for more focused work on individual
> drivers and seamless integration of forthcoming enhancements.
>
> v6:
>   - fixed compilation error with symbol export in API
>   - exported power_get_lcore_mapped_cpu_id as internal API to be
>     used in drivers/power/*
>
> v5:
>   - fixed code style warning
>
> v4:
>   - fixed build error with RTE_ASSERT
>
> v3:
>   - renamed rte_power_core_ops.h as rte_power_cpufreq_api.h
>   - re-worked on auto detection logic
>
> v2:
>   - added NULL check for global_core_ops in rte_power_get_core_ops
>
> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
> ---
>   drivers/meson.build                           |   1 +
>   .../power/acpi/acpi_cpufreq.c                 |  22 +-
>   .../power/acpi/acpi_cpufreq.h                 |   6 +-
>   drivers/power/acpi/meson.build                |  10 +
>   .../power/amd_pstate/amd_pstate_cpufreq.c     |  24 +-
>   .../power/amd_pstate/amd_pstate_cpufreq.h     |  10 +-
>   drivers/power/amd_pstate/meson.build          |  10 +
>   .../power/cppc/cppc_cpufreq.c                 |  22 +-
>   .../power/cppc/cppc_cpufreq.h                 |   8 +-
>   drivers/power/cppc/meson.build                |  10 +
>   .../power/kvm_vm}/guest_channel.c             |   0
>   .../power/kvm_vm}/guest_channel.h             |   0
>   .../power/kvm_vm/kvm_vm.c                     |  22 +-
>   .../power/kvm_vm/kvm_vm.h                     |   6 +-
>   drivers/power/kvm_vm/meson.build              |  14 +
>   drivers/power/meson.build                     |  12 +
>   drivers/power/pstate/meson.build              |  10 +
>   .../power/pstate/pstate_cpufreq.c             |  22 +-
>   .../power/pstate/pstate_cpufreq.h             |   6 +-
>   lib/power/meson.build                         |   7 +-
>   lib/power/power_common.c                      |   2 +-
>   lib/power/power_common.h                      |  18 +-
>   lib/power/rte_power.c                         | 355 ++++++++----------
>   lib/power/rte_power.h                         | 116 +++---
>   lib/power/rte_power_cpufreq_api.h             | 206 ++++++++++
>   lib/power/version.map                         |  15 +
>   26 files changed, 665 insertions(+), 269 deletions(-)
>   rename lib/power/power_acpi_cpufreq.c => drivers/power/acpi/acpi_cpufreq.c (95%)
>   rename lib/power/power_acpi_cpufreq.h => drivers/power/acpi/acpi_cpufreq.h (98%)
>   create mode 100644 drivers/power/acpi/meson.build
>   rename lib/power/power_amd_pstate_cpufreq.c => drivers/power/amd_pstate/amd_pstate_cpufreq.c (95%)
>   rename lib/power/power_amd_pstate_cpufreq.h => drivers/power/amd_pstate/amd_pstate_cpufreq.h (96%)
>   create mode 100644 drivers/power/amd_pstate/meson.build
>   rename lib/power/power_cppc_cpufreq.c => drivers/power/cppc/cppc_cpufreq.c (95%)
>   rename lib/power/power_cppc_cpufreq.h => drivers/power/cppc/cppc_cpufreq.h (97%)
>   create mode 100644 drivers/power/cppc/meson.build
>   rename {lib/power => drivers/power/kvm_vm}/guest_channel.c (100%)
>   rename {lib/power => drivers/power/kvm_vm}/guest_channel.h (100%)
>   rename lib/power/power_kvm_vm.c => drivers/power/kvm_vm/kvm_vm.c (82%)
>   rename lib/power/power_kvm_vm.h => drivers/power/kvm_vm/kvm_vm.h (98%)
>   create mode 100644 drivers/power/kvm_vm/meson.build
>   create mode 100644 drivers/power/meson.build
>   create mode 100644 drivers/power/pstate/meson.build
>   rename lib/power/power_pstate_cpufreq.c => drivers/power/pstate/pstate_cpufreq.c (96%)
>   rename lib/power/power_pstate_cpufreq.h => drivers/power/pstate/pstate_cpufreq.h (98%)
>   create mode 100644 lib/power/rte_power_cpufreq_api.h
>
> diff --git a/drivers/meson.build b/drivers/meson.build
> index 2733306698..7ef4f581a0 100644
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -29,6 +29,7 @@ subdirs = [
>           'event',          # depends on common, bus, mempool and net.
>           'baseband',       # depends on common and bus.
>           'gpu',            # depends on common and bus.
> +        'power',          # depends on common (in future).
>   ]
>   
>   if meson.is_cross_build()
> diff --git a/lib/power/power_acpi_cpufreq.c b/drivers/power/acpi/acpi_cpufreq.c
> similarity index 95%
> rename from lib/power/power_acpi_cpufreq.c
> rename to drivers/power/acpi/acpi_cpufreq.c
> index ae809fbb60..974fbb7ba8 100644
> --- a/lib/power/power_acpi_cpufreq.c
> +++ b/drivers/power/acpi/acpi_cpufreq.c
> @@ -10,7 +10,7 @@
>   #include <rte_stdatomic.h>
>   #include <rte_string_fns.h>
>   
> -#include "power_acpi_cpufreq.h"
> +#include "acpi_cpufreq.h"
>   #include "power_common.h"
>   
<...>
> diff --git a/lib/power/power_common.c b/lib/power/power_common.c
> index b47c63a5f1..e482f71c64 100644
> --- a/lib/power/power_common.c
> +++ b/lib/power/power_common.c
> @@ -13,7 +13,7 @@
>   
>   #include "power_common.h"
>   
> -RTE_LOG_REGISTER_DEFAULT(power_logtype, INFO);
> +RTE_LOG_REGISTER_DEFAULT(rte_power_logtype, INFO);
>   
>   #define POWER_SYSFILE_SCALING_DRIVER   \
>   		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index 82fb94d0c0..c294f561bb 100644
> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -6,12 +6,13 @@
>   #define _POWER_COMMON_H_
>   
>   #include <rte_common.h>
> +#include <rte_compat.h>
>   #include <rte_log.h>
>   
>   #define RTE_POWER_INVALID_FREQ_INDEX (~0)
>   
> -extern int power_logtype;
> -#define RTE_LOGTYPE_POWER power_logtype
> +extern int rte_power_logtype;
> +#define RTE_LOGTYPE_POWER rte_power_logtype
>   #define POWER_LOG(level, ...) \
>   	RTE_LOG_LINE(level, POWER, "" __VA_ARGS__)
>   
> @@ -23,14 +24,27 @@ extern int power_logtype;
>   #endif
>   
>   /* check if scaling driver matches one we want */
> +__rte_internal
>   int cpufreq_check_scaling_driver(const char *driver);
> +
> +__rte_internal
>   int power_set_governor(unsigned int lcore_id, const char *new_governor,
>   		char *orig_governor, size_t orig_governor_len);
cpufreq_check_scaling_driver and power_set_governor are just used for 
cpufreq, they shouldn't be put in this common header file.
We've come to an aggrement in patch V2 1/4.
I guess you forget it😁
suggest that move these two APIs to rte_power_cpufreq_api.h.
> +
> +__rte_internal
>   int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
>   		__rte_format_printf(3, 4);
> +
> +__rte_internal
>   int read_core_sysfs_u32(FILE *f, uint32_t *val);
> +
> +__rte_internal
>   int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
> +
> +__rte_internal
>   int write_core_sysfs_s(FILE *f, const char *str);
> +
> +__rte_internal
>   int power_get_lcore_mapped_cpu_id(uint32_t lcore_id, uint32_t *cpu_id);
>   
>   #endif /* _POWER_COMMON_H_ */
> diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c
> index 36c3f3da98..416f0148a3 100644
> --- a/lib/power/rte_power.c
> +++ b/lib/power/rte_power.c
> @@ -6,155 +6,88 @@
>   
>   #include <rte_errno.h>
>   #include <rte_spinlock.h>
> +#include <rte_debug.h>
>   
>   #include "rte_power.h"
> -#include "power_acpi_cpufreq.h"
> -#include "power_cppc_cpufreq.h"
>   #include "power_common.h"
> -#include "power_kvm_vm.h"
> -#include "power_pstate_cpufreq.h"
> -#include "power_amd_pstate_cpufreq.h"
>   
> -enum power_management_env global_default_env = PM_ENV_NOT_SET;
> +static enum power_management_env global_default_env = PM_ENV_NOT_SET;
> +static struct rte_power_core_ops *global_power_core_ops;
>   
>   static rte_spinlock_t global_env_cfg_lock = RTE_SPINLOCK_INITIALIZER;
> -
> -/* function pointers */
> -rte_power_freqs_t rte_power_freqs  = NULL;
> -rte_power_get_freq_t rte_power_get_freq = NULL;
> -rte_power_set_freq_t rte_power_set_freq = NULL;
> -rte_power_freq_change_t rte_power_freq_up = NULL;
> -rte_power_freq_change_t rte_power_freq_down = NULL;
> -rte_power_freq_change_t rte_power_freq_max = NULL;
> -rte_power_freq_change_t rte_power_freq_min = NULL;
> -rte_power_freq_change_t rte_power_turbo_status;
> -rte_power_freq_change_t rte_power_freq_enable_turbo;
> -rte_power_freq_change_t rte_power_freq_disable_turbo;
> -rte_power_get_capabilities_t rte_power_get_capabilities;
> -
> -static void
> -reset_power_function_ptrs(void)
> +static RTE_TAILQ_HEAD(, rte_power_core_ops) core_ops_list =
> +			TAILQ_HEAD_INITIALIZER(core_ops_list);
> +
> +const char *power_env_str[] = {
> +	"not set",
> +	"acpi",
> +	"kvm-vm",
> +	"pstate",
> +	"cppc",
> +	"amd-pstate"
> +};
> +

<...>
> +uint32_t
> +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->get_avail_freqs(lcore_id, freqs, n);
> +}
> +
> +uint32_t
> +rte_power_get_freq(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->get_freq(lcore_id);
> +}
> +
> +uint32_t
> +rte_power_set_freq(unsigned int lcore_id, uint32_t index)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->set_freq(lcore_id, index);
> +}
> +
> +int
> +rte_power_freq_up(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->freq_up(lcore_id);
> +}
> +
> +int
> +rte_power_freq_down(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->freq_down(lcore_id);
> +}
> +
> +int
> +rte_power_freq_max(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->freq_max(lcore_id);
> +}
> +
> +int
> +rte_power_freq_min(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->freq_min(lcore_id);
> +}
>   
> +int
> +rte_power_turbo_status(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->turbo_status(lcore_id);
> +}
> +
> +int
> +rte_power_freq_enable_turbo(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->enable_turbo(lcore_id);
> +}
> +
> +int
> +rte_power_freq_disable_turbo(unsigned int lcore_id)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->disable_turbo(lcore_id);
> +}
> +
> +int
> +rte_power_get_capabilities(unsigned int lcore_id,
> +		struct rte_power_core_capabilities *caps)
> +{
> +	RTE_ASSERT(global_power_core_ops != NULL);
> +	return global_power_core_ops->get_caps(lcore_id, caps);
>   }
> diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h
> index 4fa4afe399..e9a72b92ad 100644
> --- a/lib/power/rte_power.h
> +++ b/lib/power/rte_power.h
> @@ -1,5 +1,6 @@
>   /* SPDX-License-Identifier: BSD-3-Clause
>    * Copyright(c) 2010-2014 Intel Corporation
> + * Copyright(c) 2024 Advanced Micro Devices, Inc.
>    */
>   
>   #ifndef _RTE_POWER_H
> @@ -14,14 +15,21 @@
>   #include <rte_log.h>
>   #include <rte_power_guest_channel.h>
>   
> +#include "rte_power_cpufreq_api.h"
 From the name of rte_power.c and rte_power.h, they are supposed to work 
for all power libraries I also proposed in previous version.
But rte_power.* currently just work for cpufreq lib. If we need to put 
all power components togeter and create it.
Now that the rte_power_cpufreq_api.h has been created for cpufreq library.
How about directly rename rte_power.c to rte_poer_cpufreq_api.c and 
rte_power.h to rte_power_cpufreq_api.h?
There will be ABI changes, but it is allowed in this 24.11. If we plan 
to do it later, we'll have to wait another year.
> +
>   #ifdef __cplusplus
>   extern "C" {
>   #endif
>   
>   /* Power Management Environment State */
> -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
> -		PM_ENV_PSTATE_CPUFREQ, PM_ENV_CPPC_CPUFREQ,
> -		PM_ENV_AMD_PSTATE_CPUFREQ};
> +enum power_management_env {
> +	PM_ENV_NOT_SET = 0,
> +	PM_ENV_ACPI_CPUFREQ,
> +	PM_ENV_KVM_VM,
> +	PM_ENV_PSTATE_CPUFREQ,
> +	PM_ENV_CPPC_CPUFREQ,
> +	PM_ENV_AMD_PSTATE_CPUFREQ
> +};
>   
<...>