RE: [PATCH v1 1/4] power: refactor core power management library

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

[-- Attachment #1: Type: text/plain, Size: 14178 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]

Hi Dave,

From: Hunt, David <david.hunt@intel.com>
Sent: Tuesday, July 23, 2024 3:34 PM
To: Tummala, Sivaprasad <Sivaprasad.Tummala@amd.com>; anatoly.burakov@intel.com; jerinj@marvell.com; lihuisong@huawei.com; david.marchand@redhat.com; Yigit, Ferruh <Ferruh.Yigit@amd.com>; konstantin.ananyev@huawei.com
Cc: dev@dpdk.org
Subject: Re: [PATCH v1 1/4] 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,

A couple of comments below:
On 20/07/2024 17:50, Sivaprasad Tummala wrote:

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.



Signed-off-by: Sivaprasad Tummala <sivaprasad.tummala@amd.com><mailto: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     |   8 +-

 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              |  16 +

 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                      |  16 +-

 lib/power/rte_power.c                         | 287 ++++++------------

 lib/power/rte_power.h                         | 139 ++++++---

 lib/power/rte_power_core_ops.h                | 208 +++++++++++++

 lib/power/version.map                         |  14 +

 26 files changed, 618 insertions(+), 270 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 (97%)

 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_core_ops.h

--snip--

diff --git a/lib/power/rte_power.c b/lib/power/rte_power.c

index 36c3f3da98..8afb5949b9 100644

--- a/lib/power/rte_power.c

+++ b/lib/power/rte_power.c

@@ -8,153 +8,86 @@

 #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;

+static enum power_management_env global_default_env = PM_ENV_NOT_SET;

+static struct rte_power_core_ops *global_power_core_ops;



Suggest initialising this to NULL so we can check in rte_power_get_core_ops if it's null and throw an error.

[Siva] rte_power_core_ops as static global is initialized to NULL at runtime. Not sure, if it’s still required to initialize to NULL.

--snip--

+struct rte_power_core_ops *

+rte_power_get_core_ops(void)

+{



Need a check here to see if rte_power_get_core_ops is NULL. If it is, then the developer has probably called a frequency change API before the relevant init function, so throw an error.

Also, all the functions that call this need to check if it returns NULL so as to avoid a segfault when they attempts to call the op function.

[Siva] ACK. Will fix this in next version.





+       return global_power_core_ops;

+}

+


--snip--



diff --git a/lib/power/rte_power.h b/lib/power/rte_power.h

index 4fa4afe399..5e4aacf08b 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_core_ops.h"

+

 #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

+};



 /**

  * Check if a specific power management environment type is supported on a

@@ -66,6 +74,15 @@ void rte_power_unset_env(void);

  */

 enum power_management_env rte_power_get_env(void);



+/**

+ * @internal Get the power ops struct from its index.

+ *

+ * @return

+ *   The pointer to the ops struct in the table if registered.

+ */

+struct rte_power_core_ops *

+rte_power_get_core_ops(void);

+

 /**

  * 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 +125,13 @@ 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_core_ops *ops = rte_power_get_core_ops();



-extern rte_power_freqs_t rte_power_freqs;

+       return ops->get_avail_freqs(lcore_id, freqs, n);



This function will segfault if is called before the appropriate init is performed. See comments above on global_power_core_ops.

Same for all the functions below that call global_power_core_ops().





+}



 /**

  * Return the current index of available frequencies of a specific lcore.

@@ -124,9 +144,13 @@ extern rte_power_freqs_t rte_power_freqs;

  * @return

  *  The current index of available frequencies.

  */

-typedef uint32_t (*rte_power_get_freq_t)(unsigned int lcore_id);

+static inline uint32_t

+rte_power_get_freq(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();



-extern rte_power_get_freq_t rte_power_get_freq;

+       return ops->get_freq(lcore_id);

+}



 /**

  * Set the new frequency for a specific lcore by indicating the index of

@@ -144,82 +168,101 @@ extern rte_power_get_freq_t rte_power_get_freq;

  *  - 0 on success without frequency changed.

  *  - Negative on error.

  */

-typedef int (*rte_power_set_freq_t)(unsigned int lcore_id, uint32_t index);

-

-extern rte_power_set_freq_t rte_power_set_freq;

+static inline uint32_t

+rte_power_set_freq(unsigned int lcore_id, uint32_t index)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();



-/**

- * 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.

- */

-typedef int (*rte_power_freq_change_t)(unsigned int lcore_id);

+       return ops->set_freq(lcore_id, index);

+}



 /**

  * Scale up the frequency of a specific lcore according to the available

  * frequencies.

  * Review each environments specific documentation for usage.

  */

-extern rte_power_freq_change_t rte_power_freq_up;

+static inline int

+rte_power_freq_up(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->freq_up(lcore_id);

+}



 /**

  * Scale down the frequency of a specific lcore according to the available

  * frequencies.

  * Review each environments specific documentation for usage.

  */

-extern rte_power_freq_change_t rte_power_freq_down;

+static inline int

+rte_power_freq_down(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->freq_down(lcore_id);

+}



 /**

  * Scale up the frequency of a specific lcore to the highest according to the

  * available frequencies.

  * Review each environments specific documentation for usage.

  */

-extern rte_power_freq_change_t rte_power_freq_max;

+static inline int

+rte_power_freq_max(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->freq_max(lcore_id);

+}



 /**

  * Scale down the frequency of a specific lcore to the lowest according to the

  * available frequencies.

  * Review each environments specific documentation for usage..

  */

-extern rte_power_freq_change_t rte_power_freq_min;

+static inline int

+rte_power_freq_min(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->freq_min(lcore_id);

+}



 /**

  * Query the Turbo Boost status of a specific lcore.

  * Review each environments specific documentation for usage..

  */

-extern rte_power_freq_change_t rte_power_turbo_status;

+static inline int

+rte_power_turbo_status(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->turbo_status(lcore_id);

+}



 /**

  * Enable Turbo Boost for this lcore.

  * Review each environments specific documentation for usage..

  */

-extern rte_power_freq_change_t rte_power_freq_enable_turbo;

+static inline int

+rte_power_freq_enable_turbo(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();

+

+       return ops->enable_turbo(lcore_id);

+}



 /**

  * Disable Turbo Boost for this lcore.

  * Review each environments specific documentation for usage..

  */

-extern rte_power_freq_change_t rte_power_freq_disable_turbo;

+static inline int

+rte_power_freq_disable_turbo(unsigned int lcore_id)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();



-/**

- * 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 */

-               };

-       };

-};

+       return ops->disable_turbo(lcore_id);

+}



 /**

  * Returns power capabilities for a specific lcore.

@@ -235,10 +278,14 @@ struct rte_power_core_capabilities {

  *  - 0 on success.

  *  - Negative on error.

  */

-typedef int (*rte_power_get_capabilities_t)(unsigned int lcore_id,

-               struct rte_power_core_capabilities *caps);

+static inline int

+rte_power_get_capabilities(unsigned int lcore_id,

+               struct rte_power_core_capabilities *caps)

+{

+       struct rte_power_core_ops *ops = rte_power_get_core_ops();



-extern rte_power_get_capabilities_t rte_power_get_capabilities;

+       return ops->get_caps(lcore_id, caps);

+}



 #ifdef __cplusplus

 }



--snip--



[-- Attachment #2: Type: text/html, Size: 29984 bytes --]