-----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.
+
+__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.
