[PATCH v7 1/5] power: refactor core power management library
lihuisong (C)
lihuisong at huawei.com
Tue Oct 22 10:36:00 CEST 2024
在 2024/10/22 15:13, Tummala, Sivaprasad 写道:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Huisong,
>
> Please find my comments inline.
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong at huawei.com>
>> Sent: Tuesday, October 22, 2024 8:33 AM
>> To: Tummala, Sivaprasad <Sivaprasad.Tummala at amd.com>;
>> david.hunt at intel.com; konstantin.ananyev at huawei.com
>> Cc: dev at dpdk.org; anatoly.burakov at intel.com; jerinj at marvell.com;
>> radu.nicolau at intel.com; gakhil at marvell.com; cristian.dumitrescu at intel.com; Yigit,
>> Ferruh <Ferruh.Yigit at 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.tummala at 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
What is your plan? I feel it is not very hard and just rename the file.
>>> +
>>> #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
>>> +};
>>>
>> <...>
More information about the dev
mailing list