From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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: <de8a4f5c-d22b-1922-922b-d8ae9bc2071b@huawei.com>
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 <sivaprasad.tummala@amd.com>
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>,
<ferruh.yigit@amd.com>, <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)" <lihuisong@huawei.com>
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=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 <lihuisong@huawei.com>
/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 <sivaprasad.tummala@amd.com>
> ---
<snip>
> +
> +/**
> + * 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 <rte_errno.h>
> #include <rte_spinlock.h>
> +#include <rte_debug.h>
>
> #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;
> }
>
<snip>