[AMD Official Use Only - AMD Internal Distribution Only]
Hi Huisong,
Please find my comments inline.
> -----Original Message-----
> From: lihuisong (C) <lihuis...@huawei.com>
> Sent: Tuesday, October 22, 2024 8:33 AM
> To: Tummala, Sivaprasad <sivaprasad.tumm...@amd.com>;
> david.h...@intel.com; konstantin.anan...@huawei.com
> Cc: dev@dpdk.org; anatoly.bura...@intel.com; jer...@marvell.com;
> radu.nico...@intel.com; gak...@marvell.com; cristian.dumitre...@intel.com;
> Yigit,
> Ferruh <ferruh.yi...@amd.com>
> Subject: Re: [PATCH v7 1/5] power: refactor core power management library
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 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.tumm...@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.
OK!
> > +
> > +__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.
Yes, I had split the rte_power.h as part of refactor to avoid exposing internal
functions.
Renaming rte_power.* to rte_power_cpufreq.* can be considered but not merge
with rte_power_cpufreq_api.h
> > +
> > #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
> > +};
> >
> <...>
