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 346DA45BDC; Sat, 26 Oct 2024 09:03:06 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 04B2040265; Sat, 26 Oct 2024 09:03:06 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id A48E940264 for ; Sat, 26 Oct 2024 09:03:04 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.162.254]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Xb9Xm4FLSz10Nkt; Sat, 26 Oct 2024 15:00:56 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id 40376180105; Sat, 26 Oct 2024 15:03:02 +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; Sat, 26 Oct 2024 15:03:01 +0800 Message-ID: Date: Sat, 26 Oct 2024 15:03: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 v9 1/6] power: refactor core power management library To: "Tummala, Sivaprasad" CC: "dev@dpdk.org" , "david.hunt@intel.com" , "anatoly.burakov@intel.com" , "jerinj@marvell.com" , "radu.nicolau@intel.com" , "cristian.dumitrescu@intel.com" , "konstantin.ananyev@huawei.com" , "Yigit, Ferruh" , "gakhil@marvell.com" References: <20241022184133.700367-1-sivaprasad.tummala@amd.com> <20241023051139.1066426-1-sivaprasad.tummala@amd.com> <20241023051139.1066426-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: dggems703-chm.china.huawei.com (10.3.19.180) 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/26 13:22, Tummala, Sivaprasad 写道: > [AMD Official Use Only - AMD Internal Distribution Only] > > Hi Huisong, > >> -----Original Message----- >> From: lihuisong (C) >> Sent: Saturday, October 26, 2024 8:37 AM >> To: Tummala, Sivaprasad >> Cc: dev@dpdk.org; david.hunt@intel.com; anatoly.burakov@intel.com; >> jerinj@marvell.com; radu.nicolau@intel.com; cristian.dumitrescu@intel.com; >> konstantin.ananyev@huawei.com; Yigit, Ferruh ; >> gakhil@marvell.com >> Subject: Re: [PATCH v9 1/6] 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, >> >> LGTM except for some trivial comments inline, With belows to change, you can add >> Acked-by: Huisong Li >> >> /Huisong >> 在 2024/10/23 13:11, 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. >>> >>> v8: >>> - marked rte_power_logtype as internal >>> - removed c++ guards for internal header files >>> - renamed rte_power_cpufreq_api.h for naming convention >>> - renamed rte_power_register_ops for naming convention >>> >>> 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 >>> --- >> >>> + >>> +/** >>> + * Register power cpu frequency operations. >>> + * >>> + * @param ops >>> + * Pointer to an ops structure to register. >>> + * @return >>> + * - >=0: Success; return the index of the ops struct in the table. >>> + * - -EINVAL - error while registering ops struct. >> Not the index in the table, need to fix it. >> BTW, this API always success now. so no return value. > API now consistently returns 0 on success, rather than an index in the table. It will return a negative value on error. > I'll update the documentation to reflect this change and avoid any confusion. Ack >>> + */ >>> +__rte_internal >>> +int rte_power_register_cpufreq_ops(struct rte_power_cpufreq_ops >>> +*ops); >>> + >>> +/** >>> + * Macro to statically register the ops of a cpufreq driver. >>> + */ >>> +#define RTE_POWER_REGISTER_CPUFREQ_OPS(ops) \ >>> +RTE_INIT(power_hdlr_init_##ops) \ >>> +{ \ >>> + rte_power_register_cpufreq_ops(&ops); \ } >>> + >>> +#endif >>> diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c index >>> 36c3f3da98..3168b6d301 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_cpufreq_ops *global_cpufreq_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_cpufreq_ops) cpufreq_ops_list = >>> + TAILQ_HEAD_INITIALIZER(cpufreq_ops_list); >>> + >>> +const char *power_env_str[] = { >>> + "not set", >>> + "acpi", >>> + "kvm-vm", >>> + "pstate", >>> + "cppc", >>> + "amd-pstate" >>> +}; >> How use the "not set"? I don't know what its usage is. Do we need to consider >> removing it later? > The "not set" is default state and indicates no specific cpufreq management driver is active. > If the specific driver (located in drivers/power/*) is disabled during the build process, > the API will fail to configure the environment, leaving it in the "not set" state. ok >>> + >>> +/* register the ops struct in rte_power_cpufreq_ops, return 0 on >>> +success. */ int rte_power_register_cpufreq_ops(struct >>> +rte_power_cpufreq_ops *driver_ops) >>> { >>> - rte_power_freqs = NULL; >>> - rte_power_get_freq = NULL; >>> - rte_power_set_freq = NULL; >>> - rte_power_freq_up = NULL; >>> - rte_power_freq_down = NULL; >>> - rte_power_freq_max = NULL; >>> - rte_power_freq_min = NULL; >>> - rte_power_turbo_status = NULL; >>> - rte_power_freq_enable_turbo = NULL; >>> - rte_power_freq_disable_turbo = NULL; >>> - rte_power_get_capabilities = NULL; >>> + if (!driver_ops->init || !driver_ops->exit || >>> + !driver_ops->check_env_support || !driver_ops->get_avail_freqs || >>> + !driver_ops->get_freq || !driver_ops->set_freq || >>> + !driver_ops->freq_up || !driver_ops->freq_down || >>> + !driver_ops->freq_max || !driver_ops->freq_min || >>> + !driver_ops->turbo_status || !driver_ops->enable_turbo || >>> + !driver_ops->disable_turbo || !driver_ops->get_caps) { >>> + POWER_LOG(ERR, "Missing callbacks while registering cpufreq ops"); >>> + return -EINVAL; >>> + } >>> + >>> + TAILQ_INSERT_TAIL(&cpufreq_ops_list, driver_ops, next); >>> + >>> + return 0; >>> } >> suggest that change function return value as above mention. > Same as above. >>> int >>> rte_power_check_env_supported(enum power_management_env env) >>> { >>> - switch (env) { >>> - case PM_ENV_ACPI_CPUFREQ: >>> - return power_acpi_cpufreq_check_supported(); >>> - case PM_ENV_PSTATE_CPUFREQ: >>> - return power_pstate_cpufreq_check_supported(); >>> - case PM_ENV_KVM_VM: >>> - return power_kvm_vm_check_supported(); >>> - case PM_ENV_CPPC_CPUFREQ: >>> - return power_cppc_cpufreq_check_supported(); >>> - case PM_ENV_AMD_PSTATE_CPUFREQ: >>> - return power_amd_pstate_cpufreq_check_supported(); >>> - default: >>> - rte_errno = EINVAL; >>> - return -1; >>> - } >>> + struct rte_power_cpufreq_ops *ops; >>> + >>> + if (env >= RTE_DIM(power_env_str)) >>> + return 0; >>> + >>> + RTE_TAILQ_FOREACH(ops, &cpufreq_ops_list, next) >>> + if (strncmp(ops->name, power_env_str[env], >>> + RTE_POWER_DRIVER_NAMESZ) == 0) >>> + return ops->check_env_support(); >>> + >>> + return 0; >>> } >>> >>