Re: [dpdk-dev] [PATCH v2 07/10] librte_power common interface for Guest and Host

[Neil Horman <nhorman@tuxdriver.com>]

On Wed, Sep 24, 2014 at 06:26:13PM +0100, Alan Carew wrote:
> Moved the current librte_power implementation to rte_power_acpi_cpufreq, with
> renaming of functions only.
> Added rte_power_kvm_vm implmentation to support Power Management from a VM.
> 
> librte_power now hides the implementation based on the environment used.
> A new call rte_power_set_env() can explicidly set the environment, if not
> called then auto-detection takes place.
> 
> rte_power_kvm_vm is subset of the librte_power APIs, the following is supported:
>  rte_power_init(unsigned lcore_id)
>  rte_power_exit(unsigned lcore_id)
>  rte_power_freq_up(unsigned lcore_id)
>  rte_power_freq_down(unsigned lcore_id)
>  rte_power_freq_min(unsigned lcore_id)
>  rte_power_freq_max(unsigned lcore_id)
> 
> The other unsupported APIs return -ENOTSUP
> 
> Signed-off-by: Alan Carew <alan.carew@intel.com>
> ---
>  lib/librte_power/rte_power.c              | 540 ++++-------------------------
>  lib/librte_power/rte_power.h              | 120 +++++--
>  lib/librte_power/rte_power_acpi_cpufreq.c | 545 ++++++++++++++++++++++++++++++
>  lib/librte_power/rte_power_acpi_cpufreq.h | 192 +++++++++++
>  lib/librte_power/rte_power_common.h       |  39 +++
>  lib/librte_power/rte_power_kvm_vm.c       | 160 +++++++++
>  lib/librte_power/rte_power_kvm_vm.h       | 179 ++++++++++
>  7 files changed, 1273 insertions(+), 502 deletions(-)
>  create mode 100644 lib/librte_power/rte_power_acpi_cpufreq.c
>  create mode 100644 lib/librte_power/rte_power_acpi_cpufreq.h
>  create mode 100644 lib/librte_power/rte_power_common.h
>  create mode 100644 lib/librte_power/rte_power_kvm_vm.c
>  create mode 100644 lib/librte_power/rte_power_kvm_vm.h
> 
> diff --git a/lib/librte_power/rte_power.c b/lib/librte_power/rte_power.c
> index 856da9a..998ed1c 100644
> --- a/lib/librte_power/rte_power.c
> +++ b/lib/librte_power/rte_power.c
> @@ -31,515 +31,113 @@
>   *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   */
>  
> -#include <stdio.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -#include <fcntl.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <unistd.h>
> -#include <signal.h>
> -#include <limits.h>
> -
> -#include <rte_memcpy.h>
>  #include <rte_atomic.h>
>  
>  #include "rte_power.h"
> +#include "rte_power_acpi_cpufreq.h"
> +#include "rte_power_kvm_vm.h"
> +#include "rte_power_common.h"
>  
> -#ifdef RTE_LIBRTE_POWER_DEBUG
> -#define POWER_DEBUG_TRACE(fmt, args...) do { \
> -		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
> -	} while (0)
> -#else
> -#define POWER_DEBUG_TRACE(fmt, args...)
> -#endif
> -
> -#define FOPEN_OR_ERR_RET(f, retval) do { \
> -	if ((f) == NULL) { \
> -		RTE_LOG(ERR, POWER, "File not openned\n"); \
> -		return (retval); \
> -	} \
> -} while(0)
> -
> -#define FOPS_OR_NULL_GOTO(ret, label) do { \
> -	if ((ret) == NULL) { \
> -		RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
> -		goto label; \
> -	} \
> -} while(0)
> -
> -#define FOPS_OR_ERR_GOTO(ret, label) do { \
> -	if ((ret) < 0) { \
> -		RTE_LOG(ERR, POWER, "File operations failed\n"); \
> -		goto label; \
> -	} \
> -} while(0)
> -
> -#define STR_SIZE     1024
> -#define POWER_CONVERT_TO_DECIMAL 10
> +enum power_management_env global_default_env = PM_ENV_NOT_SET;
>  
> -#define POWER_GOVERNOR_USERSPACE "userspace"
> -#define POWER_SYSFILE_GOVERNOR   \
> -	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
> -#define POWER_SYSFILE_AVAIL_FREQ \
> -	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequencies"
> -#define POWER_SYSFILE_SETSPEED   \
> -	"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_setspeed"
> +volatile uint32_t global_env_cfg_status = 0;
>  
> -enum power_state {
> -	POWER_IDLE = 0,
> -	POWER_ONGOING,
> -	POWER_USED,
> -	POWER_UNKNOWN
> -};
> +/* 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;
>  
> -/**
> - * Power info per lcore.
> - */
> -struct rte_power_info {
> -	unsigned lcore_id;                   /**< Logical core id */
> -	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
> -	uint32_t nb_freqs;                   /**< number of available freqs */
> -	FILE *f;                             /**< FD of scaling_setspeed */
> -	char governor_ori[32];               /**< Original governor name */
> -	uint32_t curr_idx;                   /**< Freq index in freqs array */
> -	volatile uint32_t state;             /**< Power in use state */
> -} __rte_cache_aligned;
> -
> -static struct rte_power_info lcore_power_info[RTE_MAX_LCORE];
> -
> -/**
> - * It is to set specific freq for specific logical core, according to the index
> - * of supported frequencies.
> - */
> -static int
> -set_freq_internal(struct rte_power_info *pi, uint32_t idx)
> +int
> +rte_power_set_env(enum power_management_env env)
>  {
> -	if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) {
> -		RTE_LOG(ERR, POWER, "Invalid frequency index %u, which "
> -			"should be less than %u\n", idx, pi->nb_freqs);
> -		return -1;
> -	}
> -
> -	/* Check if it is the same as current */
> -	if (idx == pi->curr_idx)
> +	if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
>  		return 0;
> -
1 Nit here.  If an invalid environment value is passed in on the first config
attempt here, you won't ever be able to set it.  Maybe add some logic to return
us to an initial state if a value env isn't selected?

Neil