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 D5FAD45B99; Tue, 22 Oct 2024 04:06:36 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 664B3402BC; Tue, 22 Oct 2024 04:06:36 +0200 (CEST) Received: from szxga02-in.huawei.com (szxga02-in.huawei.com [45.249.212.188]) by mails.dpdk.org (Postfix) with ESMTP id 66D4640295 for ; Tue, 22 Oct 2024 04:06:32 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.88.105]) by szxga02-in.huawei.com (SkyGuard) with ESMTP id 4XXb7K3dMnzfdWb; Tue, 22 Oct 2024 10:03:25 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id 3243B1403A0; Tue, 22 Oct 2024 10:05:55 +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 10:05:54 +0800 Message-ID: Date: Tue, 22 Oct 2024 10:05:36 +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 v2 2/4] power: refactor uncore power management library To: "Tummala, Sivaprasad" CC: "dev@dpdk.org" , "david.hunt@intel.com" , "anatoly.burakov@intel.com" , "radu.nicolau@intel.com" , "jerinj@marvell.com" , "cristian.dumitrescu@intel.com" , "konstantin.ananyev@huawei.com" , "Yigit, Ferruh" , "gakhil@marvell.com" References: <20240720165030.246294-1-sivaprasad.tummala@amd.com> <20240826130650.320130-1-sivaprasad.tummala@amd.com> <20240826130650.320130-3-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: dggems702-chm.china.huawei.com (10.3.19.179) 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 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) >> Sent: Tuesday, August 27, 2024 6:33 PM >> To: Tummala, Sivaprasad >> Cc: dev@dpdk.org; david.hunt@intel.com; anatoly.burakov@intel.com; >> radu.nicolau@intel.com; jerinj@marvell.com; cristian.dumitrescu@intel.com; >> konstantin.ananyev@huawei.com; Yigit, Ferruh ; >> gakhil@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 >>> --- >>> .../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 >>> >>> -#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 >>> @@ -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; >>> >> <...>