Re: [PATCH v2 2/4] power: refactor uncore power management library

["lihuisong (C)" <lihuisong@huawei.com>]

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.tummala@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]).
>   ]
>   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.
>   
> -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.
>   {
>   	int ret = -1;
>   
<...>