From: "Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>
To: "lihuisong (C)" <lihuisong@huawei.com>,
"david.hunt@intel.com" <david.hunt@intel.com>,
"anatoly.burakov@intel.com" <anatoly.burakov@intel.com>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"radu.nicolau@intel.com" <radu.nicolau@intel.com>,
"gakhil@marvell.com" <gakhil@marvell.com>,
"cristian.dumitrescu@intel.com" <cristian.dumitrescu@intel.com>,
"Yigit, Ferruh" <Ferruh.Yigit@amd.com>,
"konstantin.ananyev@huawei.com" <konstantin.ananyev@huawei.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: RE: [RFC PATCH 1/2] power: refactor core power management library
Date: Tue, 5 Mar 2024 04:35:52 +0000 [thread overview]
Message-ID: <CH3PR12MB823398A5A19DF302C2DFA01686222@CH3PR12MB8233.namprd12.prod.outlook.com> (raw)
In-Reply-To: <6c7c0383-3eb4-049c-fedd-7d66b1b738da@huawei.com>
[AMD Official Use Only - General]
Hi Lihuisong,
> -----Original Message-----
> From: lihuisong (C) <lihuisong@huawei.com>
> Sent: Friday, March 1, 2024 8:27 AM
> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>;
> david.hunt@intel.com; anatoly.burakov@intel.com; jerinj@marvell.com;
> radu.nicolau@intel.com; gakhil@marvell.com; cristian.dumitrescu@intel.com; Yigit,
> Ferruh <Ferruh.Yigit@amd.com>; konstantin.ananyev@huawei.com
> Cc: dev@dpdk.org
> Subject: Re: [RFC PATCH 1/2] power: refactor core power management library
>
> Caution: This message originated from an External Source. Use proper caution
> when opening attachments, clicking links, or responding.
>
>
> 在 2024/2/20 23:33, 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.
>
> Good job. +1 to refacotor.
>
> <...>
>
> > diff --git a/drivers/meson.build b/drivers/meson.build index
> > f2be71bc05..e293c3945f 100644
> > --- a/drivers/meson.build
> > +++ b/drivers/meson.build
> > @@ -28,6 +28,7 @@ subdirs = [
> > 'event', # depends on common, bus, mempool and net.
> > 'baseband', # depends on common and bus.
> > 'gpu', # depends on common and bus.
> > + 'power', # depends on common (in future).
> > ]
> >
> > if meson.is_cross_build()
> > diff --git a/drivers/power/core/acpi/meson.build
> > b/drivers/power/core/acpi/meson.build
> > new file mode 100644
> > index 0000000000..d10ec8ee94
> > --- /dev/null
> > +++ b/drivers/power/core/acpi/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +sources = files('power_acpi_cpufreq.c')
> > +
> > +headers = files('power_acpi_cpufreq.h')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_acpi_cpufreq.c
> > b/drivers/power/core/acpi/power_acpi_cpufreq.c
> > similarity index 95%
> > rename from lib/power/power_acpi_cpufreq.c rename to
> > drivers/power/core/acpi/power_acpi_cpufreq.c
> This file is in power lib.
> How about remove the 'power' prefix of this file name?
> like acpi_cpufreq.c, cppc_cpufreq.c.
ACK
> > index f8d978d03d..69d80ad2ae 100644
> > --- a/lib/power/power_acpi_cpufreq.c
> > +++ b/drivers/power/core/acpi/power_acpi_cpufreq.c
> > @@ -577,3 +577,22 @@ int power_acpi_get_capabilities(unsigned int
> > lcore_id,
> >
> > return 0;
> > }
> > +
> > +static struct rte_power_ops acpi_ops = {
> How about use the following structure name?
> "struct rte_power_cpufreq_ops" or "struct rte_power_core_ops"
> After all, we also have other power ops, like uncore, right?
Agreed.
> > + .init = power_acpi_cpufreq_init,
> > + .exit = power_acpi_cpufreq_exit,
> > + .check_env_support = power_acpi_cpufreq_check_supported,
> > + .get_avail_freqs = power_acpi_cpufreq_freqs,
> > + .get_freq = power_acpi_cpufreq_get_freq,
> > + .set_freq = power_acpi_cpufreq_set_freq,
> > + .freq_down = power_acpi_cpufreq_freq_down,
> > + .freq_up = power_acpi_cpufreq_freq_up,
> > + .freq_max = power_acpi_cpufreq_freq_max,
> > + .freq_min = power_acpi_cpufreq_freq_min,
> > + .turbo_status = power_acpi_turbo_status,
> > + .enable_turbo = power_acpi_enable_turbo,
> > + .disable_turbo = power_acpi_disable_turbo,
> > + .get_caps = power_acpi_get_capabilities };
> > +
> > +RTE_POWER_REGISTER_OPS(acpi_ops);
> > diff --git a/lib/power/power_acpi_cpufreq.h
> > b/drivers/power/core/acpi/power_acpi_cpufreq.h
> > similarity index 100%
> > rename from lib/power/power_acpi_cpufreq.h rename to
> > drivers/power/core/acpi/power_acpi_cpufreq.h
> > diff --git a/drivers/power/core/amd-pstate/meson.build
> > b/drivers/power/core/amd-pstate/meson.build
> > new file mode 100644
> > index 0000000000..8ec4c960f5
> > --- /dev/null
> > +++ b/drivers/power/core/amd-pstate/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +sources = files('power_amd_pstate_cpufreq.c')
> > +
> > +headers = files('power_amd_pstate_cpufreq.h')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_amd_pstate_cpufreq.c
> > b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
> > similarity index 95%
> > rename from lib/power/power_amd_pstate_cpufreq.c
> > rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
> > index 028f84416b..9938de72a6 100644
> > --- a/lib/power/power_amd_pstate_cpufreq.c
> > +++ b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.c
> > @@ -700,3 +700,22 @@ power_amd_pstate_get_capabilities(unsigned int
> > lcore_id,
> >
> > return 0;
> > }
> > +
> > +static struct rte_power_ops amd_pstate_ops = {
> > + .init = power_amd_pstate_cpufreq_init,
> > + .exit = power_amd_pstate_cpufreq_exit,
> > + .check_env_support = power_amd_pstate_cpufreq_check_supported,
> > + .get_avail_freqs = power_amd_pstate_cpufreq_freqs,
> > + .get_freq = power_amd_pstate_cpufreq_get_freq,
> > + .set_freq = power_amd_pstate_cpufreq_set_freq,
> > + .freq_down = power_amd_pstate_cpufreq_freq_down,
> > + .freq_up = power_amd_pstate_cpufreq_freq_up,
> > + .freq_max = power_amd_pstate_cpufreq_freq_max,
> > + .freq_min = power_amd_pstate_cpufreq_freq_min,
> > + .turbo_status = power_amd_pstate_turbo_status,
> > + .enable_turbo = power_amd_pstate_enable_turbo,
> > + .disable_turbo = power_amd_pstate_disable_turbo,
> > + .get_caps = power_amd_pstate_get_capabilities };
> > +
> > +RTE_POWER_REGISTER_OPS(amd_pstate_ops);
> > diff --git a/lib/power/power_amd_pstate_cpufreq.h
> > b/drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h
> > similarity index 100%
> > rename from lib/power/power_amd_pstate_cpufreq.h
> > rename to drivers/power/core/amd-pstate/power_amd_pstate_cpufreq.h
> > diff --git a/drivers/power/core/cppc/meson.build
> > b/drivers/power/core/cppc/meson.build
> > new file mode 100644
> > index 0000000000..06f3b99bb8
> > --- /dev/null
> > +++ b/drivers/power/core/cppc/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +sources = files('power_cppc_cpufreq.c')
> > +
> > +headers = files('power_cppc_cpufreq.h')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_cppc_cpufreq.c
> > b/drivers/power/core/cppc/power_cppc_cpufreq.c
> > similarity index 96%
> > rename from lib/power/power_cppc_cpufreq.c rename to
> > drivers/power/core/cppc/power_cppc_cpufreq.c
> > index 3ddf39bd76..605f633309 100644
> > --- a/lib/power/power_cppc_cpufreq.c
> > +++ b/drivers/power/core/cppc/power_cppc_cpufreq.c
> > @@ -685,3 +685,22 @@ power_cppc_get_capabilities(unsigned int
> > lcore_id,
> >
> > return 0;
> > }
> > +
> > +static struct rte_power_ops cppc_ops = {
> > + .init = power_cppc_cpufreq_init,
> > + .exit = power_cppc_cpufreq_exit,
> > + .check_env_support = power_cppc_cpufreq_check_supported,
> > + .get_avail_freqs = power_cppc_cpufreq_freqs,
> > + .get_freq = power_cppc_cpufreq_get_freq,
> > + .set_freq = power_cppc_cpufreq_set_freq,
> > + .freq_down = power_cppc_cpufreq_freq_down,
> > + .freq_up = power_cppc_cpufreq_freq_up,
> > + .freq_max = power_cppc_cpufreq_freq_max,
> > + .freq_min = power_cppc_cpufreq_freq_min,
> > + .turbo_status = power_cppc_turbo_status,
> > + .enable_turbo = power_cppc_enable_turbo,
> > + .disable_turbo = power_cppc_disable_turbo,
> > + .get_caps = power_cppc_get_capabilities };
> > +
> > +RTE_POWER_REGISTER_OPS(cppc_ops);
> > diff --git a/lib/power/power_cppc_cpufreq.h
> > b/drivers/power/core/cppc/power_cppc_cpufreq.h
> > similarity index 100%
> > rename from lib/power/power_cppc_cpufreq.h rename to
> > drivers/power/core/cppc/power_cppc_cpufreq.h
> > diff --git a/lib/power/guest_channel.c
> > b/drivers/power/core/kvm-vm/guest_channel.c
> > similarity index 100%
> > rename from lib/power/guest_channel.c
> > rename to drivers/power/core/kvm-vm/guest_channel.c
> > diff --git a/lib/power/guest_channel.h
> > b/drivers/power/core/kvm-vm/guest_channel.h
> > similarity index 100%
> > rename from lib/power/guest_channel.h
> > rename to drivers/power/core/kvm-vm/guest_channel.h
> > diff --git a/drivers/power/core/kvm-vm/meson.build
> > b/drivers/power/core/kvm-vm/meson.build
> > new file mode 100644
> > index 0000000000..3150c6674b
> > --- /dev/null
> > +++ b/drivers/power/core/kvm-vm/meson.build
> > @@ -0,0 +1,20 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(C) 2024 AMD
> > +Limited.
> > +#
> > +
> > +if not is_linux
> > + build = false
> > + reason = 'only supported on Linux'
> > + subdir_done()
> > +endif
> > +
> > +sources = files(
> > + 'guest_channel.c',
> > + 'power_kvm_vm.c',
> > +)
> > +
> > +headers = files(
> > + 'guest_channel.h',
> > + 'power_kvm_vm.h',
> > +)
> > +deps += ['power']
> > diff --git a/lib/power/power_kvm_vm.c
> > b/drivers/power/core/kvm-vm/power_kvm_vm.c
> > similarity index 83%
> > rename from lib/power/power_kvm_vm.c
> > rename to drivers/power/core/kvm-vm/power_kvm_vm.c
> > index f15be8fac5..a5d6984d26 100644
> > --- a/lib/power/power_kvm_vm.c
> > +++ b/drivers/power/core/kvm-vm/power_kvm_vm.c
> > @@ -137,3 +137,22 @@ int power_kvm_vm_get_capabilities(__rte_unused
> unsigned int lcore_id,
> > POWER_LOG(ERR, "rte_power_get_capabilities is not implemented for Virtual
> Machine Power Management");
> > return -ENOTSUP;
> > }
> > +
> > +static struct rte_power_ops kvm_vm_ops = {
> > + .init = power_kvm_vm_init,
> > + .exit = power_kvm_vm_exit,
> > + .check_env_support = power_kvm_vm_check_supported,
> > + .get_avail_freqs = power_kvm_vm_freqs,
> > + .get_freq = power_kvm_vm_get_freq,
> > + .set_freq = power_kvm_vm_set_freq,
> > + .freq_down = power_kvm_vm_freq_down,
> > + .freq_up = power_kvm_vm_freq_up,
> > + .freq_max = power_kvm_vm_freq_max,
> > + .freq_min = power_kvm_vm_freq_min,
> > + .turbo_status = power_kvm_vm_turbo_status,
> > + .enable_turbo = power_kvm_vm_enable_turbo,
> > + .disable_turbo = power_kvm_vm_disable_turbo,
> > + .get_caps = power_kvm_vm_get_capabilities };
> > +
> > +RTE_POWER_REGISTER_OPS(kvm_vm_ops);
> > diff --git a/lib/power/power_kvm_vm.h
> > b/drivers/power/core/kvm-vm/power_kvm_vm.h
> > similarity index 100%
> > rename from lib/power/power_kvm_vm.h
> > rename to drivers/power/core/kvm-vm/power_kvm_vm.h
> > diff --git a/drivers/power/core/meson.build
> > b/drivers/power/core/meson.build new file mode 100644 index
> > 0000000000..4081dafaa0
> > --- /dev/null
> > +++ b/drivers/power/core/meson.build
> > @@ -0,0 +1,12 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +drivers = [
> > + 'acpi',
> > + 'amd-pstate',
> > + 'cppc',
> > + 'kvm-vm',
> > + 'pstate'
> > +]
> > +
> > +std_deps = ['power']
> > diff --git a/drivers/power/core/pstate/meson.build
> > b/drivers/power/core/pstate/meson.build
> > new file mode 100644
> > index 0000000000..1025c64e48
> > --- /dev/null
> > +++ b/drivers/power/core/pstate/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +sources = files('power_pstate_cpufreq.c')
> > +
> > +headers = files('power_pstate_cpufreq.h')
> > +
> > +deps += ['power']
> > diff --git a/lib/power/power_pstate_cpufreq.c
> > b/drivers/power/core/pstate/power_pstate_cpufreq.c
> > similarity index 96%
> > rename from lib/power/power_pstate_cpufreq.c rename to
> > drivers/power/core/pstate/power_pstate_cpufreq.c
> > index 73138dc4e4..d4c3645ff8 100644
> > --- a/lib/power/power_pstate_cpufreq.c
> > +++ b/drivers/power/core/pstate/power_pstate_cpufreq.c
> > @@ -888,3 +888,22 @@ int power_pstate_get_capabilities(unsigned int
> > lcore_id,
> >
> > return 0;
> > }
> > +
> > +static struct rte_power_ops pstate_ops = {
> > + .init = power_pstate_cpufreq_init,
> > + .exit = power_pstate_cpufreq_exit,
> > + .check_env_support = power_pstate_cpufreq_check_supported,
> > + .get_avail_freqs = power_pstate_cpufreq_freqs,
> > + .get_freq = power_pstate_cpufreq_get_freq,
> > + .set_freq = power_pstate_cpufreq_set_freq,
> > + .freq_down = power_pstate_cpufreq_freq_down,
> > + .freq_up = power_pstate_cpufreq_freq_up,
> > + .freq_max = power_pstate_cpufreq_freq_max,
> > + .freq_min = power_pstate_cpufreq_freq_min,
> > + .turbo_status = power_pstate_turbo_status,
> > + .enable_turbo = power_pstate_enable_turbo,
> > + .disable_turbo = power_pstate_disable_turbo,
> > + .get_caps = power_pstate_get_capabilities };
> > +
> > +RTE_POWER_REGISTER_OPS(pstate_ops);
> > diff --git a/lib/power/power_pstate_cpufreq.h
> > b/drivers/power/core/pstate/power_pstate_cpufreq.h
> > similarity index 100%
> > rename from lib/power/power_pstate_cpufreq.h rename to
> > drivers/power/core/pstate/power_pstate_cpufreq.h
> > diff --git a/drivers/power/meson.build b/drivers/power/meson.build new
> > file mode 100644 index 0000000000..7d9034c7ac
> > --- /dev/null
> > +++ b/drivers/power/meson.build
> > @@ -0,0 +1,8 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2024 AMD
> > +Limited
> > +
> > +drivers = [
> > + 'core',
> > +]
> > +
> > +std_deps = ['power']
> > diff --git a/lib/power/meson.build b/lib/power/meson.build index
> > b8426589b2..207d96d877 100644
> > --- a/lib/power/meson.build
> > +++ b/lib/power/meson.build
> > @@ -12,14 +12,8 @@ if not is_linux
> > reason = 'only supported on Linux'
> > endif
> > sources = files(
> > - 'guest_channel.c',
> > - 'power_acpi_cpufreq.c',
> > - 'power_amd_pstate_cpufreq.c',
> > 'power_common.c',
> > - 'power_cppc_cpufreq.c',
> > - 'power_kvm_vm.c',
> > 'power_intel_uncore.c',
> > - 'power_pstate_cpufreq.c',
> > 'rte_power.c',
> > 'rte_power_uncore.c',
> > 'rte_power_pmd_mgmt.c',
> > diff --git a/lib/power/power_common.h b/lib/power/power_common.h index
> > 30966400ba..c90b611f4f 100644
> > --- a/lib/power/power_common.h
> > +++ b/lib/power/power_common.h
> > @@ -23,13 +23,24 @@ extern int power_logtype;
> > #endif
> >
> > /* check if scaling driver matches one we want */
> > +__rte_internal
> > int cpufreq_check_scaling_driver(const char *driver);
> > +
> > +__rte_internal
> > int power_set_governor(unsigned int lcore_id, const char *new_governor,
> > char *orig_governor, size_t orig_governor_len);
> > +
> > +__rte_internal
> > int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
> > __rte_format_printf(3, 4);
> > +
> > +__rte_internal
> > int read_core_sysfs_u32(FILE *f, uint32_t *val);
> > +
> > +__rte_internal
> > int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
> > +
> > +__rte_internal
> > int write_core_sysfs_s(FILE *f, const char *str);
> >
> > #endif /* _POWER_COMMON_H_ */
> > diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c index
> > 36c3f3da98..70176807f4 100644
> > --- a/lib/power/rte_power.c
> > +++ b/lib/power/rte_power.c
> > @@ -8,64 +8,80 @@
> > #include <rte_spinlock.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;
> use a pointer to save the current power cpufreq ops?
ACK
> >
> > static rte_spinlock_t global_env_cfg_lock =
> > RTE_SPINLOCK_INITIALIZER;
> > +static struct rte_power_ops rte_power_ops[PM_ENV_MAX];
> >
> > -/* 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)
> > +/* register the ops struct in rte_power_ops, return 0 on success. */
> > +int rte_power_register_ops(const struct rte_power_ops *op) {
> > + struct rte_power_ops *ops;
> > +
> > + if (op->env >= PM_ENV_MAX) {
> > + POWER_LOG(ERR, "Unsupported power management environment\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (op->status != 0) {
> > + POWER_LOG(ERR, "Power management env[%d] ops registered
> already\n",
> > + op->env);
> > + return -EINVAL;
> > + }
> > +
> > + if (!op->init || !op->exit || !op->check_env_support ||
> > + !op->get_avail_freqs || !op->get_freq || !op->set_freq ||
> > + !op->freq_up || !op->freq_down || !op->freq_max ||
> > + !op->freq_min || !op->turbo_status || !op->enable_turbo ||
> > + !op->disable_turbo || !op->get_caps) {
> > + POWER_LOG(ERR, "Missing callbacks while registering power ops\n");
> > + return -EINVAL;
> > + }
> > +
> > + ops = &rte_power_ops[op->env];
> It is better to use a global linked list instead of an array.
> And we should extract a list structure including this ops structure and this ops's
> owner.
> > + ops->env = op->env;
> > + ops->init = op->init;
> > + ops->exit = op->exit;
> > + ops->check_env_support = op->check_env_support;
> > + ops->get_avail_freqs = op->get_avail_freqs;
> > + ops->get_freq = op->get_freq;
> > + ops->set_freq = op->set_freq;
> > + ops->freq_up = op->freq_up;
> > + ops->freq_down = op->freq_down;
> > + ops->freq_max = op->freq_max;
> > + ops->freq_min = op->freq_min;
> > + ops->turbo_status = op->turbo_status;
> > + ops->enable_turbo = op->enable_turbo;
> > + ops->disable_turbo = op->disable_turbo;
> *ops = *op?
> > + ops->status = 1; /* registered */
> status --> registered?
> But if use ops linked list, this flag also can be removed.
> > +
> > + return 0;
> > +}
> > +
> > +struct rte_power_ops *
> > +rte_power_get_ops(int ops_index)
> AFAICS, there is only one cpufreq driver on one platform and just have one
> power_cpufreq_ops to use for user.
> We don't need user to get other power ops, and user just want to know the power
> ops using currently, right?
> So using 'index' toget this ops is not good.
Agreed! I will rework this to make it global.
> > {
> > - 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;
> > + RTE_VERIFY((ops_index >= PM_ENV_NOT_SET) && (ops_index <
> PM_ENV_MAX));
> > + RTE_VERIFY(rte_power_ops[ops_index].status != 0);
> > +
> > + return &rte_power_ops[ops_index];
> > }
> >
> > 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_ops *ops;
> > +
> > + if ((env > PM_ENV_NOT_SET) && (env < PM_ENV_MAX)) {
> > + ops = rte_power_get_ops(env);
> > + return ops->check_env_support();
> > }
> > +
> > + rte_errno = EINVAL;
> > + return -1;
> > }
> >
> > int
> > @@ -80,80 +96,26 @@ rte_power_set_env(enum power_management_env
> env)
> > }
> >
> > int ret = 0;
> > + struct rte_power_ops *ops;
> > +
> > + if ((env == PM_ENV_NOT_SET) || (env >= PM_ENV_MAX)) {
> > + POWER_LOG(ERR, "Invalid Power Management Environment(%d)"
> > + " set\n", env);
> > + ret = -1;
> > + }
> >
> <...>
> > + ops = rte_power_get_ops(env);
> To find the target ops from the global list according to the env?
> > + if (ops->status == 0) {
> > + POWER_LOG(ERR, WER,
> > + "Power Management Environment(%d) not"
> > + " registered\n", env);
> > ret = -1;
> > }
> >
> > if (ret == 0)
> > global_default_env = env;
> It is more convenient to use a global variable to point to the default power_cpufreq
> ops or its list node.
Agreed
> > - else {
> > + else
> > global_default_env = PM_ENV_NOT_SET;
> > - reset_power_function_ptrs();
> > - }
> >
> > rte_spinlock_unlock(&global_env_cfg_lock);
> > return ret;
> > @@ -164,7 +126,6 @@ rte_power_unset_env(void)
> > {
> > rte_spinlock_lock(&global_env_cfg_lock);
> > global_default_env = PM_ENV_NOT_SET;
> > - reset_power_function_ptrs();
> > rte_spinlock_unlock(&global_env_cfg_lock);
> > }
> >
> > @@ -177,59 +138,76 @@ int
> > rte_power_init(unsigned int lcore_id)
> > {
> > int ret = -1;
> > + struct rte_power_ops *ops;
> >
> > - switch (global_default_env) {
> > - case PM_ENV_ACPI_CPUFREQ:
> > - return power_acpi_cpufreq_init(lcore_id);
> > - case PM_ENV_KVM_VM:
> > - return power_kvm_vm_init(lcore_id);
> > - case PM_ENV_PSTATE_CPUFREQ:
> > - return power_pstate_cpufreq_init(lcore_id);
> > - case PM_ENV_CPPC_CPUFREQ:
> > - return power_cppc_cpufreq_init(lcore_id);
> > - case PM_ENV_AMD_PSTATE_CPUFREQ:
> > - return power_amd_pstate_cpufreq_init(lcore_id);
> > - default:
> > - POWER_LOG(INFO, "Env isn't set yet!");
> > + if (global_default_env != PM_ENV_NOT_SET) {
> > + ops = &rte_power_ops[global_default_env];
> > + if (!ops->status) {
> > + POWER_LOG(ERR, "Power management env[%d] not"
> > + " supported\n", global_default_env);
> > + goto out;
> > + }
> > + return ops->init(lcore_id);
> > }
> > + POWER_LOG(INFO, POWER, "Env isn't set yet!\n");
> >
> > /* Auto detect Environment */
> > - POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq power
> management...");
> > - ret = power_acpi_cpufreq_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_ACPI_CPUFREQ);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise ACPI cpufreq"
> > + " power management...\n");
> > + ops = &rte_power_ops[PM_ENV_ACPI_CPUFREQ];
> > + if (ops->status) {
> > + ret = ops->init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_ACPI_CPUFREQ);
> > + goto out;
> > + }
> > }
> >
> > - POWER_LOG(INFO, "Attempting to initialise PSTAT power management...");
> > - ret = power_pstate_cpufreq_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise PSTAT"
> > + " power management...\n");
> > + ops = &rte_power_ops[PM_ENV_PSTATE_CPUFREQ];
> > + if (ops->status) {
> > + ret = ops->init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
> > + goto out;
> > + }
> > }
> >
> > - POWER_LOG(INFO, "Attempting to initialise AMD PSTATE power
> management...");
> > - ret = power_amd_pstate_cpufreq_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise AMD PSTATE"
> > + " power management...\n");
> > + ops = &rte_power_ops[PM_ENV_AMD_PSTATE_CPUFREQ];
> > + if (ops->status) {
> > + ret = ops->init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_AMD_PSTATE_CPUFREQ);
> > + goto out;
> > + }
> > }
> >
> > - POWER_LOG(INFO, "Attempting to initialise CPPC power management...");
> > - ret = power_cppc_cpufreq_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_CPPC_CPUFREQ);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise CPPC power"
> > + " management...\n");
> > + ops = &rte_power_ops[PM_ENV_CPPC_CPUFREQ];
> > + if (ops->status) {
> > + ret = ops->init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_CPPC_CPUFREQ);
> > + goto out;
> > + }
> > }
> >
> > - POWER_LOG(INFO, "Attempting to initialise VM power management...");
> > - ret = power_kvm_vm_init(lcore_id);
> > - if (ret == 0) {
> > - rte_power_set_env(PM_ENV_KVM_VM);
> > - goto out;
> > + POWER_LOG(INFO, "Attempting to initialise VM power"
> > + " management...\n");
> > + ops = &rte_power_ops[PM_ENV_KVM_VM];
> > + if (ops->status) {
> > + ret = ops->init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_KVM_VM);
> > + goto out;
> > + }
> > }
> If we use a linked list, above code can be simpled like this:
> ->
> for_each_power_cpufreq_ops(ops, ...) {
> ret = ops->init()
> if (ret) {
> ....
> }
> }
ACK
> > - POWER_LOG(ERR, "Unable to set Power Management Environment for lcore "
> > - "%u", lcore_id);
> > + POWER_LOG(ERR, "Unable to set Power Management Environment"
> > + " for lcore %u\n", lcore_id);
> > out:
> > return ret;
> > }
> > @@ -237,21 +215,14 @@ rte_power_init(unsigned int lcore_id)
> > int
> > rte_power_exit(unsigned int lcore_id)
> > {
> > - switch (global_default_env) {
> > - case PM_ENV_ACPI_CPUFREQ:
> > - return power_acpi_cpufreq_exit(lcore_id);
> > - case PM_ENV_KVM_VM:
> > - return power_kvm_vm_exit(lcore_id);
> > - case PM_ENV_PSTATE_CPUFREQ:
> > - return power_pstate_cpufreq_exit(lcore_id);
> > - case PM_ENV_CPPC_CPUFREQ:
> > - return power_cppc_cpufreq_exit(lcore_id);
> > - case PM_ENV_AMD_PSTATE_CPUFREQ:
> > - return power_amd_pstate_cpufreq_exit(lcore_id);
> > - default:
> > - POWER_LOG(ERR, "Environment has not been set, unable to exit
> gracefully");
> > + struct rte_power_ops *ops;
> >
> > + if (global_default_env != PM_ENV_NOT_SET) {
> > + ops = &rte_power_ops[global_default_env];
> > + return ops->exit(lcore_id);
> > }
> > - return -1;
> > + POWER_LOG(ERR, "Environment has not been set, unable "
> > + "to exit gracefully\n");
> >
> > + return -1;
> > }
> > diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h index
> > 4fa4afe399..749bb823ab 100644
> > --- a/lib/power/rte_power.h
> > +++ b/lib/power/rte_power.h
> > @@ -1,5 +1,6 @@
> > /* SPDX-License-Identifier: BSD-3-Clause
> > * Copyright(c) 2010-2014 Intel Corporation
> > + * Copyright(c) 2024 AMD Limited
> > */
> >
> > #ifndef _RTE_POWER_H
> > @@ -21,7 +22,7 @@ extern "C" {
> > /* Power Management Environment State */
> > enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ,
> PM_ENV_KVM_VM,
> > PM_ENV_PSTATE_CPUFREQ, PM_ENV_CPPC_CPUFREQ,
> > - PM_ENV_AMD_PSTATE_CPUFREQ};
> > + PM_ENV_AMD_PSTATE_CPUFREQ, PM_ENV_MAX};
> "enum power_management_env" is not good. may be like "enum
> power_cpufreq_driver_type"?
> In previous linked list structure to be defined, may be directly use a string name
> instead of a fixed enum is better.
> Becuase the new "PM_ENV_MAX" will lead to break ABI when add a new cpufreq
> driver.
I will rework this to remove the max macro.
How changing the enum power_management_env requires ABI versioning.
Will consider this change in future.
> >
> > /**
> > * Check if a specific power management environment type is
> > supported on a @@ -66,6 +67,97 @@ void rte_power_unset_env(void);
> > */
> > enum power_management_env rte_power_get_env(void);
> >
> > +typedef int (*rte_power_cpufreq_init_t)(unsigned int lcore_id);
> > +typedef int (*rte_power_cpufreq_exit_t)(unsigned int lcore_id);
> > +typedef int (*rte_power_check_env_support_t)(void);
> > +
> > +typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, uint32_t *freqs,
> > + uint32_t num); typedef uint32_t
> > +(*rte_power_get_freq_t)(unsigned int lcore_id); typedef int
> > +(*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t index);
> > +typedef int (*rte_power_freq_change_t)(unsigned int lcore_id);
> > +
> > +/**
> > + * Function pointer definition for generic frequency change
> > +functions. Review
> > + * each environments specific documentation for usage.
> > + *
> > + * @param lcore_id
> > + * lcore id.
> > + *
> > + * @return
> > + * - 1 on success with frequency changed.
> > + * - 0 on success without frequency changed.
> > + * - Negative on error.
> > + */
> > +
> > +/**
> > + * Power capabilities summary.
> > + */
> > +struct rte_power_core_capabilities {
> > + union {
> > + uint64_t capabilities;
> > + struct {
> > + uint64_t turbo:1; /**< Turbo can be enabled. */
> > + uint64_t priority:1; /**< SST-BF high freq core */
> > + };
> > + };
> > +};
> > +
> > +typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id,
> > + struct rte_power_core_capabilities
> > +*caps);
> > +
> > +/** Structure defining core power operations structure */ struct
> > +rte_power_ops {
> > +uint8_t status; /**< ops register status. */
> > + enum power_management_env env; /**< power mgmt env. */
> > + rte_power_cpufreq_init_t init; /**< Initialize power management. */
> > + rte_power_cpufreq_exit_t exit; /**< Exit power management. */
> > + rte_power_check_env_support_t check_env_support; /**< verify env is
> supported. */
> > + rte_power_freqs_t get_avail_freqs; /**< Get the available frequencies. */
> > + rte_power_get_freq_t get_freq; /**< Get frequency index. */
> > + rte_power_set_freq_t set_freq; /**< Set frequency index. */
> > + rte_power_freq_change_t freq_up; /**< Scale up frequency. */
> > + rte_power_freq_change_t freq_down; /**< Scale down frequency. */
> > + rte_power_freq_change_t freq_max; /**< Scale up frequency to highest.
> */
> > + rte_power_freq_change_t freq_min; /**< Scale up frequency to lowest. */
> > + rte_power_freq_change_t turbo_status; /**< Get Turbo status. */
> > + rte_power_freq_change_t enable_turbo; /**< Enable Turbo. */
> > + rte_power_freq_change_t disable_turbo; /**< Disable Turbo. */
> > + rte_power_get_capabilities_t get_caps; /**< power capabilities.
> > +*/ } __rte_cache_aligned;
> Suggest that fix this sturcture, like:
> struct rte_power_cpufreq_list {
> char name[]; // like "cppc_cpufreq", "pstate_cpufreq"
> struct rte_power_cpufreq *ops;
> struct rte_power_cpufreq_list *node; }
ACK
> > +
> > +/**
> > + * 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.
> > + */
> > +__rte_internal
> > +int rte_power_register_ops(const struct rte_power_ops *ops);
> > +
> > +/**
> > + * Macro to statically register the ops of a cpufreq driver.
> > + */
> > +#define RTE_POWER_REGISTER_OPS(ops) \
> > + (RTE_INIT(power_hdlr_init_##ops) \
> > + { \
> > + rte_power_register_ops(&ops); \
> > + })
> > +
> > +/**
> > + * @internal Get the power ops struct from its index.
> > + *
> > + * @param ops_index
> > + * The index of the ops struct in the ops struct table.
> > + * @return
> > + * The pointer to the ops struct in the table if registered.
> > + */
> > +struct rte_power_ops *
> > +rte_power_get_ops(int ops_index);
> > +
> > /**
> > * Initialize power management for a specific lcore. If rte_power_set_env() has
> > * not been called then an auto-detect of the environment will start
> > and @@ -108,10 +200,14 @@ int rte_power_exit(unsigned int lcore_id);
> > * @return
> > * The number of available frequencies.
> > */
> > -typedef uint32_t (*rte_power_freqs_t)(unsigned int lcore_id, uint32_t *freqs,
> > - uint32_t num);
> > +static inline uint32_t
> > +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n) {
> > + struct rte_power_ops *ops;
> >
> > -extern rte_power_freqs_t rte_power_freqs;
> > + ops = rte_power_get_ops(rte_power_get_env());
> > + return ops->get_avail_freqs(lcore_id, freqs, n); }
> nice.
> <...>
next prev parent reply other threads:[~2024-03-05 4:36 UTC|newest]
Thread overview: 12+ 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 [this message]
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
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=CH3PR12MB823398A5A19DF302C2DFA01686222@CH3PR12MB8233.namprd12.prod.outlook.com \
--to=sivaprasad.tummala@amd.com \
--cc=Ferruh.Yigit@amd.com \
--cc=anatoly.burakov@intel.com \
--cc=cristian.dumitrescu@intel.com \
--cc=david.hunt@intel.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=jerinj@marvell.com \
--cc=konstantin.ananyev@huawei.com \
--cc=lihuisong@huawei.com \
--cc=radu.nicolau@intel.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).