From: "lihuisong (C)" <lihuisong@huawei.com>
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>
Subject: Re: [PATCH v9 1/6] power: refactor core power management library
Date: Sat, 26 Oct 2024 11:06:57 +0800 [thread overview]
Message-ID: <de8a4f5c-d22b-1922-922b-d8ae9bc2071b@huawei.com> (raw)
In-Reply-To: <20241023051139.1066426-2-sivaprasad.tummala@amd.com>
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>
next prev parent reply other threads:[~2024-10-26 3:07 UTC|newest]
Thread overview: 121+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-20 15:33 [RFC PATCH 0/2] power: refactor " Sivaprasad Tummala
2024-02-20 15:33 ` Sivaprasad Tummala
2024-02-20 15:33 ` [RFC PATCH 1/2] power: refactor core " Sivaprasad Tummala
2024-02-27 16:18 ` Ferruh Yigit
2024-02-29 7:10 ` Tummala, Sivaprasad
2024-02-28 12:51 ` Ferruh Yigit
2024-03-01 2:56 ` lihuisong (C)
2024-03-01 10:39 ` Hunt, David
2024-03-05 4:35 ` Tummala, Sivaprasad
2024-02-20 15:33 ` [RFC PATCH 2/2] power: refactor uncore " Sivaprasad Tummala
2024-03-01 3:33 ` lihuisong (C)
2024-03-01 6:06 ` Tummala, Sivaprasad
2024-07-20 16:50 ` [PATCH v1 0/4] power: refactor " Sivaprasad Tummala
2024-07-20 16:50 ` [PATCH v1 1/4] power: refactor core " Sivaprasad Tummala
2024-07-23 10:03 ` Hunt, David
2024-07-27 18:44 ` Tummala, Sivaprasad
2024-07-20 16:50 ` [PATCH v1 2/4] power: refactor uncore " Sivaprasad Tummala
2024-07-23 10:26 ` Hunt, David
2024-07-20 16:50 ` [PATCH v1 3/4] test/power: removed function pointer validations Sivaprasad Tummala
2024-07-22 10:49 ` Hunt, David
2024-07-27 18:45 ` Tummala, Sivaprasad
2024-07-20 16:50 ` [PATCH v1 4/4] power/amd_uncore: uncore power management support for AMD EPYC processors Sivaprasad Tummala
2024-07-23 10:33 ` Hunt, David
2024-07-27 18:46 ` Tummala, Sivaprasad
2024-07-20 16:50 ` [PATCH v1 0/4] power: refactor power management library Sivaprasad Tummala
2024-08-26 13:06 ` [PATCH v2 " Sivaprasad Tummala
2024-08-26 13:06 ` [PATCH v2 1/4] power: refactor core " Sivaprasad Tummala
2024-08-26 15:26 ` Stephen Hemminger
2024-10-07 19:25 ` Tummala, Sivaprasad
2024-08-27 8:21 ` lihuisong (C)
2024-09-12 11:17 ` Tummala, Sivaprasad
2024-09-13 7:34 ` lihuisong (C)
2024-09-18 8:37 ` Tummala, Sivaprasad
2024-09-19 3:37 ` lihuisong (C)
2024-08-26 13:06 ` [PATCH v2 2/4] power: refactor uncore " Sivaprasad Tummala
2024-08-27 13:02 ` lihuisong (C)
2024-10-08 6:19 ` Tummala, Sivaprasad
2024-10-22 2:05 ` lihuisong (C)
2024-08-26 13:06 ` [PATCH v2 3/4] test/power: removed function pointer validations Sivaprasad Tummala
2024-08-26 13:06 ` [PATCH v2 4/4] power/amd_uncore: uncore power management support for AMD EPYC processors Sivaprasad Tummala
2024-08-26 13:06 ` [PATCH v2 0/4] power: refactor power management library Sivaprasad Tummala
2024-10-07 18:01 ` Stephen Hemminger
2024-10-08 17:27 ` [PATCH v3 0/5] " Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 1/5] power: refactor core " Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 4/5] power/amd_uncore: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-08 17:27 ` [PATCH v3 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-08 17:43 ` Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 1/5] power: refactor core " Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 4/5] power/amd_uncore: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-08 17:43 ` [PATCH v3 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-12 17:44 ` Stephen Hemminger
2024-10-15 2:49 ` [PATCH v4 " Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 1/5] power: refactor core " Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 4/5] power/amd_uncore: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-15 2:49 ` [PATCH v4 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-15 3:15 ` Stephen Hemminger
2024-10-17 10:26 ` [PATCH v5 " Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 1/5] power: refactor core " Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 4/5] drivers/power: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-17 10:26 ` [PATCH v5 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-17 16:17 ` Stephen Hemminger
2024-10-20 9:22 ` [PATCH v6 " Sivaprasad Tummala
2024-10-20 9:22 ` [PATCH v6 1/5] power: refactor core " Sivaprasad Tummala
2024-10-20 9:22 ` [PATCH v6 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-20 23:25 ` Stephen Hemminger
2024-10-20 23:28 ` Stephen Hemminger
2024-10-20 9:22 ` [PATCH v6 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-20 9:22 ` [PATCH v6 4/5] drivers/power: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-20 9:22 ` [PATCH v6 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-20 9:22 ` [PATCH v6 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-21 4:07 ` [PATCH v7 " Sivaprasad Tummala
2024-10-21 4:07 ` [PATCH v7 1/5] power: refactor core " Sivaprasad Tummala
2024-10-22 1:20 ` Stephen Hemminger
2024-10-22 6:45 ` Tummala, Sivaprasad
2024-10-22 3:03 ` lihuisong (C)
2024-10-22 7:13 ` Tummala, Sivaprasad
2024-10-22 8:36 ` lihuisong (C)
2024-10-21 4:07 ` [PATCH v7 2/5] power: refactor uncore " Sivaprasad Tummala
2024-10-22 1:18 ` Stephen Hemminger
2024-10-22 6:45 ` Tummala, Sivaprasad
2024-10-22 3:17 ` lihuisong (C)
2024-10-22 6:46 ` Tummala, Sivaprasad
2024-10-21 4:07 ` [PATCH v7 3/5] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-21 4:07 ` [PATCH v7 4/5] drivers/power: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-21 4:07 ` [PATCH v7 5/5] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-21 4:07 ` [PATCH v7 0/5] power: refactor power management library Sivaprasad Tummala
2024-10-22 1:34 ` Stephen Hemminger
2024-10-22 18:41 ` [PATCH v8 0/6] " Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 1/6] power: refactor core " Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 2/6] power: refactor uncore " Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 3/6] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 4/6] drivers/power: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 5/6] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 6/6] power: rename library sources for cpu frequency management Sivaprasad Tummala
2024-10-22 18:41 ` [PATCH v8 0/6] power: refactor power management library Sivaprasad Tummala
2024-10-23 1:40 ` Stephen Hemminger
2024-10-23 5:11 ` [PATCH v9 " Sivaprasad Tummala
2024-10-23 5:11 ` [PATCH v9 1/6] power: refactor core " Sivaprasad Tummala
2024-10-26 3:06 ` lihuisong (C) [this message]
2024-10-26 5:22 ` Tummala, Sivaprasad
2024-10-26 7:03 ` lihuisong (C)
2024-10-23 5:11 ` [PATCH v9 2/6] power: refactor uncore " Sivaprasad Tummala
2024-10-26 3:12 ` lihuisong (C)
2024-10-23 5:11 ` [PATCH v9 3/6] test/power: removed function pointer validations Sivaprasad Tummala
2024-10-23 5:11 ` [PATCH v9 4/6] drivers/power: uncore support for AMD EPYC processors Sivaprasad Tummala
2024-10-23 5:11 ` [PATCH v9 5/6] maintainers: update for drivers/power Sivaprasad Tummala
2024-10-23 5:11 ` [PATCH v9 6/6] power: rename library sources for cpu frequency management Sivaprasad Tummala
2024-10-26 4:09 ` lihuisong (C)
2024-10-23 5:11 ` [PATCH v9 0/6] power: refactor power management library Sivaprasad Tummala
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=de8a4f5c-d22b-1922-922b-d8ae9bc2071b@huawei.com \
--to=lihuisong@huawei.com \
--cc=anatoly.burakov@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=david.hunt@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=gakhil@marvell.com \
--cc=jerinj@marvell.com \
--cc=konstantin.ananyev@huawei.com \
--cc=radu.nicolau@intel.com \
--cc=sivaprasad.tummala@amd.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).