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 6B10C45B9E; Tue, 22 Oct 2024 10:36:07 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id F34034029A; Tue, 22 Oct 2024 10:36:06 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 3C8D640285 for ; Tue, 22 Oct 2024 10:36:04 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.162.254]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4XXlnR5MxYzfdVV; Tue, 22 Oct 2024 16:33:31 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id 7EB9C180106; Tue, 22 Oct 2024 16:36:01 +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 16:36:00 +0800 Message-ID: <4377f1a3-b0a5-1304-1318-10a935f040d2@huawei.com> Date: Tue, 22 Oct 2024 16:36:00 +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: "Tummala, Sivaprasad" , "david.hunt@intel.com" , "konstantin.ananyev@huawei.com" CC: "dev@dpdk.org" , "anatoly.burakov@intel.com" , "jerinj@marvell.com" , "radu.nicolau@intel.com" , "gakhil@marvell.com" , "cristian.dumitrescu@intel.com" , "Yigit, Ferruh" 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: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) 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 在 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) >> Sent: Tuesday, October 22, 2024 8:33 AM >> To: Tummala, Sivaprasad ; >> david.hunt@intel.com; konstantin.ananyev@huawei.com >> Cc: dev@dpdk.org; anatoly.burakov@intel.com; jerinj@marvell.com; >> radu.nicolau@intel.com; gakhil@marvell.com; cristian.dumitrescu@intel.com; Yigit, >> Ferruh >> 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 >>> --- >>> 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. > 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 >>> #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. > 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 >>> +}; >>> >> <...>