Re: [PATCH v2 2/4] power: refactor uncore power management library

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

Hi Sivaprasa,

I have a inline question, please take a look.

在 2024/10/8 14:19, Tummala, Sivaprasad 写道:
> [AMD Official Use Only - AMD Internal Distribution Only]
>
> Hi Lihuisong,
>
>> -----Original Message-----
>> From: lihuisong (C) <lihuisong@huawei.com>
>> Sent: Tuesday, August 27, 2024 6:33 PM
>> To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>
>> Cc: dev@dpdk.org; david.hunt@intel.com; anatoly.burakov@intel.com;
>> radu.nicolau@intel.com; jerinj@marvell.com; cristian.dumitrescu@intel.com;
>> konstantin.ananyev@huawei.com; Yigit, Ferruh <Ferruh.Yigit@amd.com>;
>> gakhil@marvell.com
>> Subject: Re: [PATCH v2 2/4] power: refactor uncore power management library
>>
>> Caution: This message originated from an External Source. Use proper caution
>> when opening attachments, clicking links, or responding.
>>
>>
>> Hi Sivaprasad,
>>
>> Suggest to split this patch into two patches for easiler to review:
>> patch-1: abstract a file for uncore dvfs core level, namely, the
>> rte_power_uncore_ops.c you did.
>> patch-2: move and rename, lib/power/power_intel_uncore.c =>
>> drivers/power/intel_uncore/intel_uncore.c
>>
>> patch[1/4] is also too big and not good to review.
>>
>> In addition, I have some question and am not sure if we can adjust uncore init
>> process.
>>
>> /Huisong
>>
>>
>> 在 2024/8/26 21:06, Sivaprasad Tummala 写道:
>>> This patch refactors the power management library, addressing uncore
>>> power management. The primary changes involve the creation of
>>> dedicated directories for each driver within 'drivers/power/uncore/*'.
>>> The adjustment of meson.build files enables the selective activation
>>> of individual drivers.
>>>
>>> This refactor significantly improves code organization, enhances
>>> clarity and boosts maintainability. It lays the foundation for more
>>> focused development on individual drivers and facilitates seamless
>>> integration of future enhancements, particularly the AMD uncore driver.
>>>
>>> Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com>
>>> ---
>>>    .../power/intel_uncore/intel_uncore.c         |  18 +-
>>>    .../power/intel_uncore/intel_uncore.h         |   8 +-
>>>    drivers/power/intel_uncore/meson.build        |   6 +
>>>    drivers/power/meson.build                     |   3 +-
>>>    lib/power/meson.build                         |   2 +-
>>>    lib/power/rte_power_uncore.c                  | 205 ++++++---------
>>>    lib/power/rte_power_uncore.h                  |  87 ++++---
>>>    lib/power/rte_power_uncore_ops.h              | 239 ++++++++++++++++++
>>>    lib/power/version.map                         |   1 +
>>>    9 files changed, 405 insertions(+), 164 deletions(-)
>>>    rename lib/power/power_intel_uncore.c =>
>> drivers/power/intel_uncore/intel_uncore.c (95%)
>>>    rename lib/power/power_intel_uncore.h =>
>> drivers/power/intel_uncore/intel_uncore.h (97%)
>>>    create mode 100644 drivers/power/intel_uncore/meson.build
>>>    create mode 100644 lib/power/rte_power_uncore_ops.h
>>>
>>> diff --git a/lib/power/power_intel_uncore.c
>>> b/drivers/power/intel_uncore/intel_uncore.c
>>> similarity index 95%
>>> rename from lib/power/power_intel_uncore.c rename to
>>> drivers/power/intel_uncore/intel_uncore.c
>>> index 4eb9c5900a..804ad5d755 100644
>>> --- a/lib/power/power_intel_uncore.c
>>> +++ b/drivers/power/intel_uncore/intel_uncore.c
>>> @@ -8,7 +8,7 @@
>>>
>>>    #include <rte_memcpy.h>
>>>
>>> -#include "power_intel_uncore.h"
>>> +#include "intel_uncore.h"
>>>    #include "power_common.h"
>>>
>>>    #define MAX_NUMA_DIE 8
>>> @@ -475,3 +475,19 @@ power_intel_uncore_get_num_dies(unsigned int pkg)
>>>
>>>        return count;
>>>    }
>> <...>
>>> -#endif /* POWER_INTEL_UNCORE_H */
>>> +#endif /* INTEL_UNCORE_H */
>>> diff --git a/drivers/power/intel_uncore/meson.build
>>> b/drivers/power/intel_uncore/meson.build
>>> new file mode 100644
>>> index 0000000000..876df8ad14
>>> --- /dev/null
>>> +++ b/drivers/power/intel_uncore/meson.build
>>> @@ -0,0 +1,6 @@
>>> +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2017 Intel
>>> +Corporation # Copyright(c) 2024 Advanced Micro Devices, Inc.
>>> +
>>> +sources = files('intel_uncore.c')
>>> +deps += ['power']
>>> diff --git a/drivers/power/meson.build b/drivers/power/meson.build
>>> index 8c7215c639..c83047af94 100644
>>> --- a/drivers/power/meson.build
>>> +++ b/drivers/power/meson.build
>>> @@ -6,7 +6,8 @@ drivers = [
>>>            'amd_pstate',
>>>            'cppc',
>>>            'kvm_vm',
>>> -        'pstate'
>>> +        'pstate',
>>> +        'intel_uncore'
>> The cppc, amd_pstate and so on belong to cpufreq scope.
>> And intel_uncore belongs to uncore dvfs scope.
>> They are not the same level. So I proposes that we need to create one directory
>> called like cpufreq or core.
>> This 'intel_uncore' name don't seems appropriate. what do you think the following
>> directory structure:
>> drivers/power/uncore/intel_uncore.c
>> drivers/power/uncore/amd_uncore.c (according to the patch[4/4]).
> At present, Meson does not support detecting an additional level of subdirectories within drivers/*.
> All the drivers maintain a consistent subdirectory structure.
>>>    ]
>>>    std_deps = ['power']
>>> diff --git a/lib/power/meson.build b/lib/power/meson.build index
>>> f3e3451cdc..9b13d98810 100644
>>> --- a/lib/power/meson.build
>>> +++ b/lib/power/meson.build
>>> @@ -13,7 +13,6 @@ if not is_linux
>>>    endif
>>>    sources = files(
>>>            'power_common.c',
>>> -        'power_intel_uncore.c',
>>>            'rte_power.c',
>>>            'rte_power_uncore.c',
>>>            'rte_power_pmd_mgmt.c',
>>> @@ -24,6 +23,7 @@ headers = files(
>>>            'rte_power_guest_channel.h',
>>>            'rte_power_pmd_mgmt.h',
>>>            'rte_power_uncore.h',
>>> +        'rte_power_uncore_ops.h',
>>>    )
>>>    if cc.has_argument('-Wno-cast-qual')
>>>        cflags += '-Wno-cast-qual'
>>> diff --git a/lib/power/rte_power_uncore.c
>>> b/lib/power/rte_power_uncore.c index 48c75a5da0..9f8771224f 100644
>>> --- a/lib/power/rte_power_uncore.c
>>> +++ b/lib/power/rte_power_uncore.c
>>> @@ -1,6 +1,7 @@
>>>    /* SPDX-License-Identifier: BSD-3-Clause
>>>     * Copyright(c) 2010-2014 Intel Corporation
>>>     * Copyright(c) 2023 AMD Corporation
>>> + * Copyright(c) 2024 Advanced Micro Devices, Inc.
>>>     */
>>>
>>>    #include <errno.h>
>>> @@ -12,98 +13,50 @@
>>>    #include "rte_power_uncore.h"
>>>    #include "power_intel_uncore.h"
>>>
>>> -enum rte_uncore_power_mgmt_env default_uncore_env =
>>> RTE_UNCORE_PM_ENV_NOT_SET;
>>> +static enum rte_uncore_power_mgmt_env global_uncore_env =
>>> +RTE_UNCORE_PM_ENV_NOT_SET; static struct rte_power_uncore_ops
>>> +*global_uncore_ops;
>>>
>>>    static rte_spinlock_t global_env_cfg_lock =
>>> RTE_SPINLOCK_INITIALIZER;
>>> +static RTE_TAILQ_HEAD(, rte_power_uncore_ops) uncore_ops_list =
>>> +                     TAILQ_HEAD_INITIALIZER(uncore_ops_list);
>>>
>>> -static uint32_t
>>> -power_get_dummy_uncore_freq(unsigned int pkg __rte_unused,
>>> -            unsigned int die __rte_unused)
>>> -{
>>> -     return 0;
>>> -}
>>> -
>>> -static int
>>> -power_set_dummy_uncore_freq(unsigned int pkg __rte_unused,
>>> -            unsigned int die __rte_unused, uint32_t index __rte_unused)
>>> -{
>>> -     return 0;
>>> -}
>>> +const char *uncore_env_str[] = {
>>> +     "not set",
>>> +     "auto-detect",
>>> +     "intel-uncore",
>>> +     "amd-hsmp"
>>> +};
>> Why open the "auto-detect" mode to user?
>> Why not set this automatically at framework initialization?
>> After all, the uncore driver is fixed for one platform.
> The auto-detection feature has been implemented to enable seamless migration across platforms
> without requiring any changes to the application
>>> -static int
>>> -power_dummy_uncore_freq_max(unsigned int pkg __rte_unused,
>>> -            unsigned int die __rte_unused)
>>> -{
>>> -     return 0;
>>> -}
>>> -
>> <...>
>>> -static int
>>> -power_dummy_uncore_get_num_freqs(unsigned int pkg __rte_unused,
>>> -            unsigned int die __rte_unused)
>>> +/* register the ops struct in rte_power_uncore_ops, return 0 on
>>> +success. */ int rte_power_register_uncore_ops(struct
>>> +rte_power_uncore_ops *driver_ops)
>>>    {
>>> -     return 0;
>>> -}
>>> +     if (!driver_ops->init || !driver_ops->exit || !driver_ops->get_num_pkgs ||
>>> +             !driver_ops->get_num_dies || !driver_ops->get_num_freqs ||
>>> +             !driver_ops->get_avail_freqs || !driver_ops->get_freq ||
>>> +             !driver_ops->set_freq || !driver_ops->freq_max ||
>>> +             !driver_ops->freq_min) {
>>> +             POWER_LOG(ERR, "Missing callbacks while registering power ops");
>>> +             return -1;
>>> +     }
>>> +     if (driver_ops->cb)
>>> +             driver_ops->cb();
>>>
>>> -static unsigned int
>>> -power_dummy_uncore_get_num_pkgs(void)
>>> -{
>>> -     return 0;
>>> -}
>>> +     TAILQ_INSERT_TAIL(&uncore_ops_list, driver_ops, next);
>>>
>>> -static unsigned int
>>> -power_dummy_uncore_get_num_dies(unsigned int pkg __rte_unused) -{
>>>        return 0;
>>>    }
>>> -
>>> -/* function pointers */
>>> -rte_power_get_uncore_freq_t rte_power_get_uncore_freq =
>>> power_get_dummy_uncore_freq; -rte_power_set_uncore_freq_t
>>> rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
>>> -rte_power_uncore_freq_change_t rte_power_uncore_freq_max =
>>> power_dummy_uncore_freq_max; -rte_power_uncore_freq_change_t
>>> rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
>>> -rte_power_uncore_freqs_t rte_power_uncore_freqs =
>>> power_dummy_uncore_freqs; -rte_power_uncore_get_num_freqs_t
>>> rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
>>> -rte_power_uncore_get_num_pkgs_t rte_power_uncore_get_num_pkgs =
>>> power_dummy_uncore_get_num_pkgs; -rte_power_uncore_get_num_dies_t
>>> rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
>>> -
>>> -static void
>>> -reset_power_uncore_function_ptrs(void)
>>> -{
>>> -     rte_power_get_uncore_freq = power_get_dummy_uncore_freq;
>>> -     rte_power_set_uncore_freq = power_set_dummy_uncore_freq;
>>> -     rte_power_uncore_freq_max = power_dummy_uncore_freq_max;
>>> -     rte_power_uncore_freq_min = power_dummy_uncore_freq_min;
>>> -     rte_power_uncore_freqs  = power_dummy_uncore_freqs;
>>> -     rte_power_uncore_get_num_freqs = power_dummy_uncore_get_num_freqs;
>>> -     rte_power_uncore_get_num_pkgs = power_dummy_uncore_get_num_pkgs;
>>> -     rte_power_uncore_get_num_dies = power_dummy_uncore_get_num_dies;
>>> -}
>>> -
>>>    int
>>>    rte_power_set_uncore_env(enum rte_uncore_power_mgmt_env env)
>>>    {
>>> -     int ret;
>>> +     int ret = -1;
>>> +     struct rte_power_uncore_ops *ops;
>>>
>>>        rte_spinlock_lock(&global_env_cfg_lock);
>>>
>>> -     if (default_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
>>> +     if (global_uncore_env != RTE_UNCORE_PM_ENV_NOT_SET) {
>>>                POWER_LOG(ERR, "Uncore Power Management Env already set.");
>>> -             rte_spinlock_unlock(&global_env_cfg_lock);
>>> -             return -1;
>>> +             goto out;
>>>        }
>>>
>> <...>
>>> +     if (env <= RTE_DIM(uncore_env_str)) {
>>> +             RTE_TAILQ_FOREACH(ops, &uncore_ops_list, next)
>>> +                     if (strncmp(ops->name, uncore_env_str[env],
>>> +                             RTE_POWER_UNCORE_DRIVER_NAMESZ) == 0) {
>>> +                             global_uncore_env = env;
>>> +                             global_uncore_ops = ops;
>>> +                             ret = 0;
>>> +                             goto out;
>>> +                     }
>>> +             POWER_LOG(ERR, "Power Management (%s) not supported",
>>> +                             uncore_env_str[env]);
>>> +     } else
>>> +             POWER_LOG(ERR, "Invalid Power Management Environment");
>>>
>>> -     default_uncore_env = env;
>>>    out:
>>>        rte_spinlock_unlock(&global_env_cfg_lock);
>>>        return ret;
>>> @@ -139,15 +89,22 @@ void
>>>    rte_power_unset_uncore_env(void)
>>>    {
>>>        rte_spinlock_lock(&global_env_cfg_lock);
>>> -     default_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
>>> -     reset_power_uncore_function_ptrs();
>>> +     global_uncore_env = RTE_UNCORE_PM_ENV_NOT_SET;
>>>        rte_spinlock_unlock(&global_env_cfg_lock);
>>>    }
>>>
>> How about abstract an ABI interface to intialize or set the uncore driver on platform
>> by automatical.
>>
>> And later do power_intel_uncore_init_on_die() for each die on different package.
>>>    enum rte_uncore_power_mgmt_env
>>>    rte_power_get_uncore_env(void)
>>>    {
>>> -     return default_uncore_env;
>>> +     return global_uncore_env;
>>> +}
>>> +
>>> +struct rte_power_uncore_ops *
>>> +rte_power_get_uncore_ops(void)
>>> +{
>>> +     RTE_ASSERT(global_uncore_ops != NULL);
>>> +
>>> +     return global_uncore_ops;
>>>    }
>>>
>>>    int
>>> @@ -155,27 +112,29 @@ rte_power_uncore_init(unsigned int pkg, unsigned
>>> int die)
>> This pkg means the socket id on the platform, right?
>> If so, I am not sure that the
>> uncore_info[RTE_MAX_NUMA_NODES][MAX_NUMA_DIE] used in uncore lib is
>> universal for all uncore driver.
>> For example, uncore driver just support do uncore dvfs based on the socket unit.
>> What shoud we do for this? we may need to think twice.
> Yes, pkg represents a socket id. In platforms with a single uncore controller per socket,
> the die ID should be set to '0' for the corresponding socket ID (pkg).
> .
So just use the die ID 0 on one socket ID(namely, uncore_info[0][0], 
uncore_info[1][0]) to initialize the uncore power info on sockets, right?
 From the implement in l3fwd-power, it set all die ID and all sockets.
For the platform with a single uncore controller per socket, their 
uncore driver in DPDK have to ignore other die IDs except die-0 on one 
socket. right?
>>>    {
>>>        int ret = -1;
>>>
>> <...>