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 BCA8145BD9; Sat, 26 Oct 2024 05:07:02 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 52B924026C; Sat, 26 Oct 2024 05:07:02 +0200 (CEST) Received: from szxga07-in.huawei.com (szxga07-in.huawei.com [45.249.212.35]) by mails.dpdk.org (Postfix) with ESMTP id C24FC40265 for ; Sat, 26 Oct 2024 05:07:00 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.163.17]) by szxga07-in.huawei.com (SkyGuard) with ESMTP id 4Xb4K75TJGz1SD0Q; Sat, 26 Oct 2024 11:05:31 +0800 (CST) Received: from kwepemm600004.china.huawei.com (unknown [7.193.23.242]) by mail.maildlp.com (Postfix) with ESMTPS id E97901A0188; Sat, 26 Oct 2024 11:06:58 +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 11:06:58 +0800 Message-ID: Date: Sat, 26 Oct 2024 11:06:57 +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: Sivaprasad Tummala CC: , , , , , , , , 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: <20241023051139.1066426-2-sivaprasad.tummala@amd.com> 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 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. > + */ > +__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? > + > +/* 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. > > 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; > } >