Hi Sivaprasa,
I have a inline question, please take a look.
在 2024/10/8 14:19, Tummala, Sivaprasad 写道:
[AMD Official Use Only - AMD Internal Distribution Only]
Hi Lihuisong,
-----Original Message-----
From: lihuisong (C) <lihuis...@huawei.com>
Sent: Tuesday, August 27, 2024 6:33 PM
To: Tummala, Sivaprasad <sivaprasad.tumm...@amd.com>
Cc: dev@dpdk.org; david.h...@intel.com; anatoly.bura...@intel.com;
radu.nico...@intel.com; jer...@marvell.com; cristian.dumitre...@intel.com;
konstantin.anan...@huawei.com; Yigit, Ferruh <ferruh.yi...@amd.com>;
gak...@marvell.com
Subject: Re: [PATCH v2 2/4] power: refactor uncore power management library
Caution: This message originated from an External Source. Use proper caution
when opening attachments, clicking links, or responding.
Hi Sivaprasad,
Suggest to split this patch into two patches for easiler to review:
patch-1: abstract a file for uncore dvfs core level, namely, the
rte_power_uncore_ops.c you did.
patch-2: move and rename, lib/power/power_intel_uncore.c =>
drivers/power/intel_uncore/intel_uncore.c
patch[1/4] is also too big and not good to review.
In addition, I have some question and am not sure if we can adjust uncore init
process.
/Huisong
在 2024/8/26 21:06, Sivaprasad Tummala 写道:
This patch refactors the power management library, addressing uncore
power management. The primary changes involve the creation of
dedicated directories for each driver within 'drivers/power/uncore/*'.
The adjustment of meson.build files enables the selective activation
of individual drivers.
This refactor significantly improves code organization, enhances
clarity and boosts maintainability. It lays the foundation for more
focused development on individual drivers and facilitates seamless
integration of future enhancements, particularly the AMD uncore driver.
Signed-off-by: Sivaprasad Tummala <sivaprasad.tumm...@amd.com>
---
.../power/intel_uncore/intel_uncore.c | 18 +-
.../power/intel_uncore/intel_uncore.h | 8 +-
drivers/power/intel_uncore/meson.build | 6 +
drivers/power/meson.build | 3 +-
lib/power/meson.build | 2 +-
lib/power/rte_power_uncore.c | 205 ++++++---------
lib/power/rte_power_uncore.h | 87 ++++---
lib/power/rte_power_uncore_ops.h | 239 ++++++++++++++++++
lib/power/version.map | 1 +
9 files changed, 405 insertions(+), 164 deletions(-)
rename lib/power/power_intel_uncore.c =>
drivers/power/intel_uncore/intel_uncore.c (95%)
rename lib/power/power_intel_uncore.h =>
drivers/power/intel_uncore/intel_uncore.h (97%)
create mode 100644 drivers/power/intel_uncore/meson.build
create mode 100644 lib/power/rte_power_uncore_ops.h
diff --git a/lib/power/power_intel_uncore.c
b/drivers/power/intel_uncore/intel_uncore.c
similarity index 95%
rename from lib/power/power_intel_uncore.c rename to
drivers/power/intel_uncore/intel_uncore.c
index 4eb9c5900a..804ad5d755 100644
--- a/lib/power/power_intel_uncore.c
+++ b/drivers/power/intel_uncore/intel_uncore.c
@@ -8,7 +8,7 @@
#include <rte_memcpy.h>
-#include "power_intel_uncore.h"
+#include "intel_uncore.h"
#include "power_common.h"
#define MAX_NUMA_DIE 8
@@ -475,3 +475,19 @@ power_intel_uncore_get_num_dies(unsigned int pkg)
return count;
}
<...>
-#endif /* POWER_INTEL_UNCORE_H */
+#endif /* INTEL_UNCORE_H */
diff --git a/drivers/power/intel_uncore/meson.build
b/drivers/power/intel_uncore/meson.build
new file mode 100644
index 0000000000..876df8ad14
--- /dev/null
+++ b/drivers/power/intel_uncore/meson.build
@@ -0,0 +1,6 @@
+# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2017 Intel
+Corporation # Copyright(c) 2024 Advanced Micro Devices, Inc.
+
+sources = files('intel_uncore.c')
+deps += ['power']
diff --git a/drivers/power/meson.build b/drivers/power/meson.build
index 8c7215c639..c83047af94 100644
--- a/drivers/power/meson.build
+++ b/drivers/power/meson.build
@@ -6,7 +6,8 @@ drivers = [
'amd_pstate',
'cppc',
'kvm_vm',
- 'pstate'
+ 'pstate',
+ 'intel_uncore'
The cppc, amd_pstate and so on belong to cpufreq scope.
And intel_uncore belongs to uncore dvfs scope.
They are not the same level. So I proposes that we need to create one directory
called like cpufreq or core.
This 'intel_uncore' name don't seems appropriate. what do you think the
following
directory structure:
drivers/power/uncore/intel_uncore.c
drivers/power/uncore/amd_uncore.c (according to the patch[4/4]).
At present, Meson does not support detecting an additional level of
subdirectories within drivers/*.
All the drivers maintain a consistent subdirectory structure.
]
std_deps = ['power']
diff --git a/lib/power/meson.build b/lib/power/meson.build index
f3e3451cdc..9b13d98810 100644
--- a/lib/power/meson.build
+++ b/lib/power/meson.build
@@ -13,7 +13,6 @@ if not is_linux
endif
sources = files(
'power_common.c',
- 'power_intel_uncore.c',
'rte_power.c',
'rte_power_uncore.c',
'rte_power_pmd_mgmt.c',
@@ -24,6 +23,7 @@ headers = files(
'rte_power_guest_channel.h',
'rte_power_pmd_mgmt.h',
'rte_power_uncore.h',
+ 'rte_power_uncore_ops.h',
)
if cc.has_argument('-Wno-cast-qual')
cflags += '-Wno-cast-qual'
diff --git a/lib/power/rte_power_uncore.c
b/lib/power/rte_power_uncore.c index 48c75a5da0..9f8771224f 100644
--- a/lib/power/rte_power_uncore.c
+++ b/lib/power/rte_power_uncore.c
@@ -1,6 +1,7 @@
/* SPDX-License-Identifier: BSD-3-Clause
* Copyright(c) 2010-2014 Intel Corporation
* Copyright(c) 2023 AMD Corporation
+ * Copyright(c) 2024 Advanced Micro Devices, Inc.
*/
#include <errno.h>
@@ -12,98 +13,50 @@
#include "rte_power_uncore.h"
#include "power_intel_uncore.h"
-enum rte_uncore_power_mgmt_env default_uncore_env =
RTE_UNCORE_PM_ENV_NOT_SET;
+static enum rte_uncore_power_mgmt_env global_uncore_env =
+RTE_UNCORE_PM_ENV_NOT_SET; static struct rte_power_uncore_ops
+*global_uncore_ops;
static rte_spinlock_t global_env_cfg_lock =
RTE_SPINLOCK_INITIALIZER;
+static RTE_TAILQ_HEAD(, rte_power_uncore_ops) uncore_ops_list =
+ TAILQ_HEAD_INITIALIZER(uncore_ops_list);
-static uint32_t
-power_get_dummy_uncore_freq(unsigned int pkg __rte_unused,
- unsigned int die __rte_unused)
-{
- return 0;
-}
-
-static int
-power_set_dummy_uncore_freq(unsigned int pkg __rte_unused,
- unsigned int die __rte_unused, uint32_t index __rte_unused)
-{
- return 0;
-}
+const char *uncore_env_str[] = {
+ "not set",
+ "auto-detect",
+ "intel-uncore",
+ "amd-hsmp"
+};
Why open the "auto-detect" mode to user?
Why not set this automatically at framework initialization?
After all, the uncore driver is fixed for one platform.
The auto-detection feature has been implemented to enable seamless migration
across platforms
without requiring any changes to the application
-static int
-power_dummy_uncore_freq_max(unsigned int pkg __rte_unused,
- unsigned int die __rte_unused)
-{
- return 0;
-}
-
<...>
-static int
-power_dummy_uncore_get_num_freqs(unsigned int pkg __rte_unused,
- unsigned int die __rte_unused)
+/* register the ops struct in rte_power_uncore_ops, return 0 on
+success. */ int rte_power_register_uncore_ops(struct
+rte_power_uncore_ops *driver_ops)
{
- return 0;
-}
+ if (!driver_ops->init || !driver_ops->exit || !driver_ops->get_num_pkgs ||
+ !driver_ops->get_num_dies || !driver_ops->get_num_freqs ||
+ !driver_ops->get_avail_freqs || !driver_ops->get_freq ||
+ !driver_ops->set_freq || !driver_ops->freq_max ||
+ !driver_ops->freq_min) {
+ POWER_LOG(ERR, "Missing callbacks while registering power ops");
+ return -1;
+ }
+ if (driver_ops->cb)
+ driver_ops->cb();
-static unsigned int
-power_dummy_uncore_get_num_pkgs(void)
-{
- return 0;
-}
+ TAILQ_INSERT_TAIL(&uncore_ops_list, driver_ops, next);
-static unsigned int
-power_dummy_uncore_get_num_dies(unsigned int pkg __rte_unused) -{
return 0;
}
-
-/* function pointers */
-rte_power_get_uncore_freq_t rte_power_get_uncore_freq =
power_get_dummy_uncore_freq; -rte_power_set_uncore_freq_t
rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
-rte_power_uncore_freq_change_t rte_power_uncore_freq_max =
power_dummy_uncore_freq_max; -rte_power_uncore_freq_change_t
rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
-rte_power_uncore_freqs_t rte_power_uncore_freqs =
power_dummy_uncore_freqs; -rte_power_uncore_get_num_freqs_t
rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
-rte_power_uncore_get_num_pkgs_t rte_power_uncore_get_num_pkgs =
power_dummy_uncore_get_num_pkgs; -rte_power_uncore_get_num_dies_t
rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
-
-static void
-reset_power_uncore_function_ptrs(void)
-{
- rte_power_get_uncore_freq = power_get_dummy_uncore_freq;
- rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
- rte_power_uncore_freq_max = power_dummy_uncore_freq_max;
- rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
- rte_power_uncore_freqs = power_dummy_uncore_freqs;
- rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
- rte_power_uncore_get_num_pkgs = power_dummy_uncore_get_num_pkgs;
- rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
-}
-
int
rte_power_set_uncore_env(enum rte_uncore_power_mgmt_env env)
{
- int ret;
+ int ret = -1;
+ struct rte_power_uncore_ops *ops;
rte_spinlock_lock(&global_env_cfg_lock);
- if (default_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
+ if (global_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
POWER_LOG(ERR, "Uncore Power Management Env already set.");
- rte_spinlock_unlock(&global_env_cfg_lock);
- return -1;
+ goto out;
}
<...>
+ if (env <= RTE_DIM(uncore_env_str)) {
+ RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next)
+ if (strncmp(ops->name, uncore_env_str[env],
+ RTE_POWER_UNCORE_DRIVER_NAMESZ) == 0) {
+ global_uncore_env = env;
+ global_uncore_ops = ops;
+ ret = 0;
+ goto out;
+ }
+ POWER_LOG(ERR, "Power Management (%s) not supported",
+ uncore_env_str[env]);
+ } else
+ POWER_LOG(ERR, "Invalid Power Management Environment");
- default_uncore_env = env;
out:
rte_spinlock_unlock(&global_env_cfg_lock);
return ret;
@@ -139,15 +89,22 @@ void
rte_power_unset_uncore_env(void)
{
rte_spinlock_lock(&global_env_cfg_lock);
- default_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
- reset_power_uncore_function_ptrs();
+ global_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
rte_spinlock_unlock(&global_env_cfg_lock);
}
How about abstract an ABI interface to intialize or set the uncore driver on
platform
by automatical.
And later do power_intel_uncore_init_on_die() for each die on different package.
enum rte_uncore_power_mgmt_env
rte_power_get_uncore_env(void)
{
- return default_uncore_env;
+ return global_uncore_env;
+}
+
+struct rte_power_uncore_ops *
+rte_power_get_uncore_ops(void)
+{
+ RTE_ASSERT(global_uncore_ops != NULL);
+
+ return global_uncore_ops;
}
int
@@ -155,27 +112,29 @@ rte_power_uncore_init(unsigned int pkg, unsigned
int die)
This pkg means the socket id on the platform, right?
If so, I am not sure that the
uncore_info[RTE_MAX_NUMA_NODES][MAX_NUMA_DIE] used in uncore lib is
universal for all uncore driver.
For example, uncore driver just support do uncore dvfs based on the socket unit.
What shoud we do for this? we may need to think twice.
Yes, pkg represents a socket id. In platforms with a single uncore controller
per socket,
the die ID should be set to '0' for the corresponding socket ID (pkg).
.
So just use the die ID 0 on one socket ID(namely, uncore_info[0][0],
uncore_info[1][0]) to initialize the uncore power info on sockets, right?
From the implement in l3fwd-power, it set all die ID and all sockets.
For the platform with a single uncore controller per socket, their
uncore driver in DPDK have to ignore other die IDs except die-0 on one
socket. right?
{
int ret = -1;
<...>
