From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1CDB545B99; Tue, 22 Oct 2024 05:03:10 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id E3C23402CA; Tue, 22 Oct 2024 05:03:09 +0200 (CEST) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id C64ED402A3 for ; Tue, 22 Oct 2024 05:03:07 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.88.163]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4XXcQb64Ktz1SD79; Tue, 22 Oct 2024 11:01:43 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id E041E180042; Tue, 22 Oct 2024 11:03:05 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm600004.china.huawei.com (7.193.23.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Tue, 22 Oct 2024 11:03:05 +0800 Message-ID: Date: Tue, 22 Oct 2024 11:03:04 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v7 1/5] power: refactor core power management library To: Sivaprasad Tummala , , CC: , , , , , , References: <20241020092233.2906612-1-sivaprasad.tummala@amd.com> <20241021040724.3325876-1-sivaprasad.tummala@amd.com> <20241021040724.3325876-2-sivaprasad.tummala@amd.com> From: "lihuisong (C)" In-Reply-To: <20241021040724.3325876-2-sivaprasad.tummala@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemm600004.china.huawei.com (7.193.23.242) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > --- > 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 > #include > > -#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 > +#include > #include > > #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 > #include > +#include > > #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 > #include > > +#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 > +}; > <...>