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

["Hunt, David" <david.hunt@intel.com>]

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

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


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


> +	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: 14043 bytes --]