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

["Tummala, Sivaprasad" <Sivaprasad.Tummala@amd.com>]

[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).
.
> >   {
> >       int ret = -1;
> >
> <...>