Re: [PATCH v9 1/6] power: refactor core power management library

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

在 2024/10/26 13:22, Tummala, Sivaprasad 写道:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Huisong,
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Saturday, October 26, 2024 8:37 AM
>> To: Tummala, Sivaprasad <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; Yigit, Ferruh <Ferruh.Yigit@amd.com>;
>> gakhil@marvell.com
>> Subject: Re: [PATCH v9 1/6] 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,
>>
>> 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.
> API now consistently returns 0 on success, rather than an index in the table. It will return a negative value on error.
> I'll update the documentation to reflect this change and avoid any confusion.
Ack
>>> + */
>>> +__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?
> The "not set" is default state and indicates no specific cpufreq management driver is active.
> If the specific driver (located in drivers/power/*) is disabled during the build process,
> the API will fail to configure the environment, leaving it in the "not set" state.
ok
>>> +
>>> +/* 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.
> Same as above.
>>>    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>