Re: [PATCH v7 1/5] power: refactor core power management library

["lihuisong (C)" <lihuisong@huawei.com>]

在 2024/10/22 15:13, Tummala, Sivaprasad 写道:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Huisong,
>
> Please find my comments inline.
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Tuesday, October 22, 2024 8:33 AM
>> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>;
>> david.hunt@intel.com; konstantin.ananyev@huawei.com
>> Cc: dev@dpdk.org; anatoly.burakov@intel.com; jerinj@marvell.com;
>> radu.nicolau@intel.com; gakhil@marvell.com; cristian.dumitrescu@intel.com; Yigit,
>> Ferruh <Ferruh.Yigit@amd.com>
>> Subject: Re: [PATCH v7 1/5] 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,
>>
>> Some comments inline.
>>
>> 在 2024/10/21 12:07, 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.
>>>
>>> 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>
>>> ---
>>>    drivers/meson.build                           |   1 +
>>>    .../power/acpi/acpi_cpufreq.c                 |  22 +-
>>>    .../power/acpi/acpi_cpufreq.h                 |   6 +-
>>>    drivers/power/acpi/meson.build                |  10 +
>>>    .../power/amd_pstate/amd_pstate_cpufreq.c     |  24 +-
>>>    .../power/amd_pstate/amd_pstate_cpufreq.h     |  10 +-
>>>    drivers/power/amd_pstate/meson.build          |  10 +
>>>    .../power/cppc/cppc_cpufreq.c                 |  22 +-
>>>    .../power/cppc/cppc_cpufreq.h                 |   8 +-
>>>    drivers/power/cppc/meson.build                |  10 +
>>>    .../power/kvm_vm}/guest_channel.c             |   0
>>>    .../power/kvm_vm}/guest_channel.h             |   0
>>>    .../power/kvm_vm/kvm_vm.c                     |  22 +-
>>>    .../power/kvm_vm/kvm_vm.h                     |   6 +-
>>>    drivers/power/kvm_vm/meson.build              |  14 +
>>>    drivers/power/meson.build                     |  12 +
>>>    drivers/power/pstate/meson.build              |  10 +
>>>    .../power/pstate/pstate_cpufreq.c             |  22 +-
>>>    .../power/pstate/pstate_cpufreq.h             |   6 +-
>>>    lib/power/meson.build                         |   7 +-
>>>    lib/power/power_common.c                      |   2 +-
>>>    lib/power/power_common.h                      |  18 +-
>>>    lib/power/rte_power.c                         | 355 ++++++++----------
>>>    lib/power/rte_power.h                         | 116 +++---
>>>    lib/power/rte_power_cpufreq_api.h             | 206 ++++++++++
>>>    lib/power/version.map                         |  15 +
>>>    26 files changed, 665 insertions(+), 269 deletions(-)
>>>    rename lib/power/power_acpi_cpufreq.c => drivers/power/acpi/acpi_cpufreq.c
>> (95%)
>>>    rename lib/power/power_acpi_cpufreq.h => drivers/power/acpi/acpi_cpufreq.h
>> (98%)
>>>    create mode 100644 drivers/power/acpi/meson.build
>>>    rename lib/power/power_amd_pstate_cpufreq.c =>
>> drivers/power/amd_pstate/amd_pstate_cpufreq.c (95%)
>>>    rename lib/power/power_amd_pstate_cpufreq.h =>
>> drivers/power/amd_pstate/amd_pstate_cpufreq.h (96%)
>>>    create mode 100644 drivers/power/amd_pstate/meson.build
>>>    rename lib/power/power_cppc_cpufreq.c => drivers/power/cppc/cppc_cpufreq.c
>> (95%)
>>>    rename lib/power/power_cppc_cpufreq.h => drivers/power/cppc/cppc_cpufreq.h
>> (97%)
>>>    create mode 100644 drivers/power/cppc/meson.build
>>>    rename {lib/power => drivers/power/kvm_vm}/guest_channel.c (100%)
>>>    rename {lib/power => drivers/power/kvm_vm}/guest_channel.h (100%)
>>>    rename lib/power/power_kvm_vm.c => drivers/power/kvm_vm/kvm_vm.c (82%)
>>>    rename lib/power/power_kvm_vm.h => drivers/power/kvm_vm/kvm_vm.h (98%)
>>>    create mode 100644 drivers/power/kvm_vm/meson.build
>>>    create mode 100644 drivers/power/meson.build
>>>    create mode 100644 drivers/power/pstate/meson.build
>>>    rename lib/power/power_pstate_cpufreq.c =>
>> drivers/power/pstate/pstate_cpufreq.c (96%)
>>>    rename lib/power/power_pstate_cpufreq.h =>
>> drivers/power/pstate/pstate_cpufreq.h (98%)
>>>    create mode 100644 lib/power/rte_power_cpufreq_api.h
>>>
>>> diff --git a/drivers/meson.build b/drivers/meson.build index
>>> 2733306698..7ef4f581a0 100644
>>> --- a/drivers/meson.build
>>> +++ b/drivers/meson.build
>>> @@ -29,6 +29,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/lib/power/power_acpi_cpufreq.c
>>> b/drivers/power/acpi/acpi_cpufreq.c
>>> similarity index 95%
>>> rename from lib/power/power_acpi_cpufreq.c rename to
>>> drivers/power/acpi/acpi_cpufreq.c index ae809fbb60..974fbb7ba8 100644
>>> --- a/lib/power/power_acpi_cpufreq.c
>>> +++ b/drivers/power/acpi/acpi_cpufreq.c
>>> @@ -10,7 +10,7 @@
>>>    #include <rte_stdatomic.h>
>>>    #include <rte_string_fns.h>
>>>
>>> -#include "power_acpi_cpufreq.h"
>>> +#include "acpi_cpufreq.h"
>>>    #include "power_common.h"
>>>
>> <...>
>>> diff --git a/lib/power/power_common.c b/lib/power/power_common.c index
>>> b47c63a5f1..e482f71c64 100644
>>> --- a/lib/power/power_common.c
>>> +++ b/lib/power/power_common.c
>>> @@ -13,7 +13,7 @@
>>>
>>>    #include "power_common.h"
>>>
>>> -RTE_LOG_REGISTER_DEFAULT(power_logtype, INFO);
>>> +RTE_LOG_REGISTER_DEFAULT(rte_power_logtype, INFO);
>>>
>>>    #define POWER_SYSFILE_SCALING_DRIVER   \
>>>                "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
>>> diff --git a/lib/power/power_common.h b/lib/power/power_common.h index
>>> 82fb94d0c0..c294f561bb 100644
>>> --- a/lib/power/power_common.h
>>> +++ b/lib/power/power_common.h
>>> @@ -6,12 +6,13 @@
>>>    #define _POWER_COMMON_H_
>>>
>>>    #include <rte_common.h>
>>> +#include <rte_compat.h>
>>>    #include <rte_log.h>
>>>
>>>    #define RTE_POWER_INVALID_FREQ_INDEX (~0)
>>>
>>> -extern int power_logtype;
>>> -#define RTE_LOGTYPE_POWER power_logtype
>>> +extern int rte_power_logtype;
>>> +#define RTE_LOGTYPE_POWER rte_power_logtype
>>>    #define POWER_LOG(level, ...) \
>>>        RTE_LOG_LINE(level, POWER, "" __VA_ARGS__)
>>>
>>> @@ -23,14 +24,27 @@ 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);
>> cpufreq_check_scaling_driver and power_set_governor are just used for cpufreq,
>> they shouldn't be put in this common header file.
>> We've come to an aggrement in patch V2 1/4.
>> I guess you forget it😁
>> suggest that move these two APIs to rte_power_cpufreq_api.h.
> OK!
>>> +
>>> +__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);
>>> +
>>> +__rte_internal
>>>    int power_get_lcore_mapped_cpu_id(uint32_t lcore_id, uint32_t
>>> *cpu_id);
>>>
>>>    #endif /* _POWER_COMMON_H_ */
>>> diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c index
>>> 36c3f3da98..416f0148a3 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_core_ops *global_power_core_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_core_ops) core_ops_list =
>>> +                     TAILQ_HEAD_INITIALIZER(core_ops_list);
>>> +
>>> +const char *power_env_str[] = {
>>> +     "not set",
>>> +     "acpi",
>>> +     "kvm-vm",
>>> +     "pstate",
>>> +     "cppc",
>>> +     "amd-pstate"
>>> +};
>>> +
>> <...>
>>> +uint32_t
>>> +rte_power_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t n) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->get_avail_freqs(lcore_id, freqs,
>>> +n); }
>>> +
>>> +uint32_t
>>> +rte_power_get_freq(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->get_freq(lcore_id);
>>> +}
>>> +
>>> +uint32_t
>>> +rte_power_set_freq(unsigned int lcore_id, uint32_t index) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->set_freq(lcore_id, index); }
>>> +
>>> +int
>>> +rte_power_freq_up(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->freq_up(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_freq_down(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->freq_down(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_freq_max(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->freq_max(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_freq_min(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->freq_min(lcore_id);
>>> +}
>>>
>>> +int
>>> +rte_power_turbo_status(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->turbo_status(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_freq_enable_turbo(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->enable_turbo(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_freq_disable_turbo(unsigned int lcore_id) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->disable_turbo(lcore_id);
>>> +}
>>> +
>>> +int
>>> +rte_power_get_capabilities(unsigned int lcore_id,
>>> +             struct rte_power_core_capabilities *caps) {
>>> +     RTE_ASSERT(global_power_core_ops != NULL);
>>> +     return global_power_core_ops->get_caps(lcore_id, caps);
>>>    }
>>> diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h index
>>> 4fa4afe399..e9a72b92ad 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 Advanced Micro Devices, Inc.
>>>     */
>>>
>>>    #ifndef _RTE_POWER_H
>>> @@ -14,14 +15,21 @@
>>>    #include <rte_log.h>
>>>    #include <rte_power_guest_channel.h>
>>>
>>> +#include "rte_power_cpufreq_api.h"
>>   From the name of rte_power.c and rte_power.h, they are supposed to work for all
>> power libraries I also proposed in previous version.
>> But rte_power.* currently just work for cpufreq lib. If we need to put all power
>> components togeter and create it.
>> Now that the rte_power_cpufreq_api.h has been created for cpufreq library.
>> How about directly rename rte_power.c to rte_poer_cpufreq_api.c and rte_power.h
>> to rte_power_cpufreq_api.h?
>> There will be ABI changes, but it is allowed in this 24.11. If we plan to do it later, we'll
>> have to wait another year.
> Yes, I had split the rte_power.h as part of refactor to avoid exposing internal functions.
> Renaming rte_power.*  to rte_power_cpufreq.* can be considered but not merge with rte_power_cpufreq_api.h
What is your plan? I feel it is not very hard and just rename the file.
>>> +
>>>    #ifdef __cplusplus
>>>    extern "C" {
>>>    #endif
>>>
>>>    /* 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};
>>> +enum power_management_env {
>>> +     PM_ENV_NOT_SET = 0,
>>> +     PM_ENV_ACPI_CPUFREQ,
>>> +     PM_ENV_KVM_VM,
>>> +     PM_ENV_PSTATE_CPUFREQ,
>>> +     PM_ENV_CPPC_CPUFREQ,
>>> +     PM_ENV_AMD_PSTATE_CPUFREQ
>>> +};
>>>
>> <...>