libs/power: add p-state driver compatibility
Checks
Commit Message
Previously, in order to use the power library, it was necessary
for the user to disable the intel_pstate driver by adding
“intel_pstate=disable” to the kernel command line for the system,
which causes the acpi_cpufreq driver to be loaded in its place.
This patch adds the ability for the power library use the intel-pstate
driver.
It adds a new suite of functions behind the current power library API,
and will seamlessly set up the user facing API function pointers to
the relevant functions depending on whether the system is running with
acpi_cpufreq kernel driver, intel_pstate kernel driver or in a guest,
using kvm. The library API and ABI is unchanged.
Signed-off-by: Liang Ma <liang.j.ma@intel.com>
---
lib/librte_power/Makefile | 2 +
lib/librte_power/meson.build | 4 +-
lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
lib/librte_power/rte_power.c | 43 +-
lib/librte_power/rte_power.h | 3 +-
6 files changed, 1039 insertions(+), 9 deletions(-)
create mode 100644 lib/librte_power/power_pstate_cpufreq.c
create mode 100644 lib/librte_power/power_pstate_cpufreq.h
Comments
On 23-Nov-18 11:33 AM, Liang Ma wrote:
> Previously, in order to use the power library, it was necessary
> for the user to disable the intel_pstate driver by adding
> “intel_pstate=disable” to the kernel command line for the system,
> which causes the acpi_cpufreq driver to be loaded in its place.
>
> This patch adds the ability for the power library use the intel-pstate
> driver.
>
> It adds a new suite of functions behind the current power library API,
> and will seamlessly set up the user facing API function pointers to
> the relevant functions depending on whether the system is running with
> acpi_cpufreq kernel driver, intel_pstate kernel driver or in a guest,
> using kvm. The library API and ABI is unchanged.
>
> Signed-off-by: Liang Ma <liang.j.ma@intel.com>
Hi Liang,
General comment: please do not break up log strings onto multiple lines.
It makes it harder to grep the codebase. Checkpatch will not warn about
log strings going over 80 characters.
> ---
> lib/librte_power/Makefile | 2 +
> lib/librte_power/meson.build | 4 +-
> lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
> lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
> lib/librte_power/rte_power.c | 43 +-
> lib/librte_power/rte_power.h | 3 +-
> 6 files changed, 1039 insertions(+), 9 deletions(-)
> create mode 100644 lib/librte_power/power_pstate_cpufreq.c
> create mode 100644 lib/librte_power/power_pstate_cpufreq.h
<snip>
> sources = files('rte_power.c', 'power_acpi_cpufreq.c',
> 'power_kvm_vm.c', 'guest_channel.c',
> - 'rte_power_empty_poll.c')
> + 'rte_power_empty_poll.c',
> + 'power_pstate_cpufreq.c')
> headers = files('rte_power.h','rte_power_empty_poll.h')
> -deps += ['timer']
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> new file mode 100644
> index 0000000..f1dada8
> --- /dev/null
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -0,0 +1,778 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
I believe it should be 2018.
> + */
> +
> +#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 <errno.h>
> +
> +#include <rte_memcpy.h>
> +#include <rte_atomic.h>
> +
> +#include "power_pstate_cpufreq.h"
> +#include "power_common.h"
> +
<snip>
> +
> +static struct pstate_power_info lcore_power_info[RTE_MAX_LCORE];
> +
> +/**
> + * It is to read the specific MSR.
> + */
> +
> +static int32_t
> +power_rdmsr(int msr, uint64_t *val, unsigned int lcore_id)
> +{
> + int fd;
> + int ret;
int fd, ret?
> + char fullpath[PATH_MAX];
> +
> + snprintf(fullpath, sizeof(fullpath), POWER_MSR_PATH, lcore_id);
> +
> + fd = open(fullpath, O_RDONLY);
> +
> + if (fd < 0) {
> +
> + if (errno == EACCES)
> + RTE_LOG(ERR, POWER, "No access to %s\n", fullpath);
> +
> + if (errno == ENXIO)
> + RTE_LOG(ERR, POWER, "%s Not Exist!\n", fullpath);
How about:
RTE_LOG(ERR, POWER, "Error opening '%s': %s\n", fullpath, strerror(errno));
?
> +
> + return fd;
> + }
> +
> + ret = pread(fd, val, sizeof(uint64_t), msr);
Can pread fail?
> +
> + close(fd);
> +
> + POWER_DEBUG_TRACE("MSR Path %s, offset 0x%X for lcore %u\n",
> + fullpath, msr, lcore_id);
> +
> + POWER_DEBUG_TRACE("Ret value %d, content is 0x%lx\n", ret, *val);
> +
> + return ret;
> +}
> +
<snip>
> + if (fseek(pi->f_cur_max, 0, SEEK_SET) < 0) {
> + RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
> + "for setting frequency for lcore %u\n",
> + pi->lcore_id);
> + return -1;
> + }
> +
> + /* Turbo is available and enabled, first freq bucket is sys max freq */
> + if (pi->turbo_available && pi->turbo_enable && (idx == 0))
> +
> + target_freq = pi->sys_max_freq;
> +
> + else
> +
> + target_freq = pi->freqs[idx];
Unneeded whitespace?
> +
> +
> + /* Decrease freq, the min freq should be updated first */
> + if (idx > pi->curr_idx) {
> +
> + if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
> + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> + "lcore %u\n", pi->lcore_id);
> + return -1;
> + }
> +
> + if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
> + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> + "lcore %u\n", pi->lcore_id);
> + return -1;
> + }
> +
> + POWER_DEBUG_TRACE("Freqency[%u] to be set for lcore %u\n",
> + target_freq, pi->lcore_id);
Frequency :)
Also, i believe "Frequency[%u]" is misleading in this case, because the
value is not an index into an array, but is an actual target frequency.
I think "Frequency '%u'" would be more descriptive.
> +
> + fflush(pi->f_cur_min);
> + fflush(pi->f_cur_max);
> +
> + }
> +
> + /* Increase freq, the max freq should be updated first */
> + if (idx < pi->curr_idx) {
Else if?
> +
> + if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
> + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> + "lcore %u\n", pi->lcore_id);
> + return -1;
> + }
> +
> + if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
> + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> + "lcore %u\n", pi->lcore_id);
> + return -1;
> + }
Forgot POWER_DEBUG_TRACE?
> +
> + fflush(pi->f_cur_max);
> + fflush(pi->f_cur_min);
> + }
> +
> + pi->curr_idx = idx;
> +
> + return 1;
> +}
> +
<snip>
> + snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> + pi->lcore_id);
> + f = fopen(fullpath, "rw+");
> + FOPEN_OR_ERR_RET(f, ret);
> +
> + s = fgets(buf, sizeof(buf), f);
> + FOPS_OR_NULL_GOTO(s, out);
> +
> + /* Check if current governor is performance */
> + if (strncmp(buf, POWER_GOVERNOR_PERF,
> + sizeof(POWER_GOVERNOR_PERF)) == 0) {
Nitpick, but probably should be strlen, not sizeof?
> + ret = 0;
> + POWER_DEBUG_TRACE("Power management governor of lcore %u is "
> + "already performance\n", pi->lcore_id);
> + goto out;
> + }
> + /* Save the original governor */
> + snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
> +
> + /* Write 'performance' to the governor */
> + val = fseek(f, 0, SEEK_SET);
> + FOPS_OR_ERR_GOTO(val, out);
> +
> + val = fputs(POWER_GOVERNOR_PERF, f);
> + FOPS_OR_ERR_GOTO(val, out);
> +
> + ret = 0;
> + RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
> + "set to performance successfully\n", pi->lcore_id);
Do we want this as INFO? (it's OK if we do, just asking!)
> +out:
> + fclose(f);
> +
> + return ret;
> +}
> +
> +/**
> + * It is to check the governor and then set the original governor back if
> + * needed by writing the sys file.
> + */
> +static int
> +power_set_governor_original(struct pstate_power_info *pi)
> +{
<snip>
> + /* Write back the original governor */
> + val = fseek(f, 0, SEEK_SET);
> + FOPS_OR_ERR_GOTO(val, out);
> +
> + val = fputs(pi->governor_ori, f);
> + FOPS_OR_ERR_GOTO(val, out);
> +
> + ret = 0;
> + RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
> + "has been set back to %s successfully\n",
> + pi->lcore_id, pi->governor_ori);
Same - do we want this as INFO?
> +out:
> + fclose(f);
> +
> + return ret;
> +}
> +
> +/**
> + * It is to get the available frequencies of the specific lcore by reading the
> + * sys file.
> + */
> +static int
> +power_get_available_freqs(struct pstate_power_info *pi)
> +{
> + FILE *f_min, *f_max;
> + int ret = -1;
> + char *p_min, *p_max;
> + char buf_min[BUFSIZ];
> + char buf_max[BUFSIZ];
> + char fullpath_min[PATH_MAX];
> + char fullpath_max[PATH_MAX];
> + char *s_min, *s_max;
> + uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
> + uint32_t i, num_freqs = 0;
> +
> + snprintf(fullpath_max, sizeof(fullpath_max),
> + POWER_SYSFILE_BASE_MAX_FREQ,
> + pi->lcore_id);
> + snprintf(fullpath_min, sizeof(fullpath_min),
> + POWER_SYSFILE_BASE_MIN_FREQ,
> + pi->lcore_id);
> +
> + f_min = fopen(fullpath_min, "r");
> + FOPEN_OR_ERR_RET(f_min, ret);
> +
> + s_min = fgets(buf_min, sizeof(buf_min), f_min);
> + FOPS_OR_NULL_GOTO(s_min, out);
> +
> + f_max = fopen(fullpath_max, "r");
> + FOPEN_OR_ERR_RET(f_max, ret);
> +
> + s_max = fgets(buf_max, sizeof(buf_max), f_max);
> + FOPS_OR_NULL_GOTO(s_max, out);
> +
> +
> + /* Strip the line break if there is */
> + p_min = strchr(buf_min, '\n');
> + if (p_min != NULL)
> + *p_min = 0;
> +
> + p_max = strchr(buf_max, '\n');
> + if (p_max != NULL)
> + *p_max = 0;
Probably '\0' would be more correct.
> +
> + sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
> + sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
> +
> + if (sys_max_freq < sys_min_freq)
> + goto out;
This leaks fds. See below.
> +
> +
> + pi->sys_max_freq = sys_max_freq;
> +
> + base_max_freq = pi->non_turbo_max_ratio*BUS_FREQ;
spacing around multiplication :)
> +
> + POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
> + sys_min_freq,
> + sys_max_freq,
> + base_max_freq);
> +
> + if (base_max_freq < sys_max_freq)
> +
> + pi->turbo_available = 1;
Extra whitespace.
> + else
> + pi->turbo_available = 0;
> +
> +
> + /* If turbo is available then there is one extra freq bucket
> + * to store the sys max freq which value is base_max +1
> + */
> + num_freqs = (base_max_freq - sys_min_freq)/BUS_FREQ + 1
> + + pi->turbo_available;
The '+' should be on the preceding line, also spacing.
> +
> + /* Generate the freq bucket array.
> + * If turbo is available the freq bucket[0] value is base_max +1
> + * the bucket[1] is base_max, bucket[2] is base_max - BUS_FREQ
> + * and so on.
> + * If turbo is not available bucket[0] is base_max and so on
> + */
> + for (i = 0, pi->nb_freqs = 0; i < num_freqs; i++) {
> +
Extra whitespace.
> + if ((i == 0) && pi->turbo_available)
> + pi->freqs[pi->nb_freqs++] = base_max_freq + 1;
> + else
> + pi->freqs[pi->nb_freqs++] =
> + base_max_freq - (i - pi->turbo_available)*BUS_FREQ;
Misleading indentation, also spacing around multiplication operator.
> + }
> +
> + ret = 0;
> +
> + POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are available\n",
> + num_freqs, pi->lcore_id);
> +
> + fclose(f_min);
> + fclose(f_max);
> +
> +
> +out:
fclose should be after out, otherwise error path above will leak fd.
> + return ret;
> +}
> +
> +int
> +power_pstate_cpufreq_init(unsigned int lcore_id)
> +{
> + struct pstate_power_info *pi;
> +
> + if (lcore_id >= RTE_MAX_LCORE) {
> + RTE_LOG(ERR, POWER, "Lcore id %u can not exceeds %u\n",
Cannot exceed.
> + lcore_id, RTE_MAX_LCORE - 1U);
> + return -1;
> + }
> +
> + pi = &lcore_power_info[lcore_id];
> + if (rte_atomic32_cmpset(&(pi->state), POWER_IDLE, POWER_ONGOING)
> + == 0) {
> + RTE_LOG(INFO, POWER, "Power management of lcore %u is "
> + "in use\n", lcore_id);
> + return -1;
> + }
> +
> + pi->lcore_id = lcore_id;
Can we not set this until we're sure we didn't fail? Or do any of the
below calls rely on pi->lcore_id to be set?
> + /* Check and set the governor */
> + if (power_set_governor_performance(pi) < 0) {
> + RTE_LOG(ERR, POWER, "Cannot set governor of lcore %u to "
> + "performance\n", lcore_id);
> + goto fail;
> + }
> + /* Init for setting lcore frequency */
> + if (power_init_for_setting_freq(pi) < 0) {
> + RTE_LOG(ERR, POWER, "Cannot init for setting frequency for "
> + "lcore %u\n", lcore_id);
> + goto fail;
> + }
> +
> + /* Get the available frequencies */
> + if (power_get_available_freqs(pi) < 0) {
> + RTE_LOG(ERR, POWER, "Cannot get available frequencies of "
> + "lcore %u\n", lcore_id);
> + goto fail;
> + }
> +
> +
> + /* Set freq to max by default */
> + if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
> + RTE_LOG(ERR, POWER, "Cannot set frequency of lcore %u "
> + "to max\n", lcore_id);
> + goto fail;
> + }
> +
> + RTE_LOG(INFO, POWER, "Initialized successfully for lcore %u "
> + "power management\n", lcore_id);
Do we want this as INFO?
> + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_USED);
> +
> + return 0;
> +
> +fail:
> + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
> +
> + return -1;
> +}
> +
> +int
> +power_pstate_cpufreq_exit(unsigned int lcore_id)
> +{
> + struct pstate_power_info *pi;
> +
<snip>
> + /* Set the governor back to the original */
> + if (power_set_governor_original(pi) < 0) {
> + RTE_LOG(ERR, POWER, "Cannot set the governor of %u back "
> + "to the original\n", lcore_id);
> + goto fail;
> + }
> +
> + RTE_LOG(INFO, POWER, "Power management of lcore %u has exited from "
> + "'performance' mode and been set back to the "
> + "original\n", lcore_id);
Perhaps print out the original governor name? Also, i think it's better
to use the performance string define, rather than having it as part of
formatted message.
> + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_IDLE);
> +
> + return 0;
> +
> +fail:
> + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
> +
> + return -1;
> +}
> +
> +
> +uint32_t
> +power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
> +{
> + struct pstate_power_info *pi;
> +
> + if (lcore_id >= RTE_MAX_LCORE) {
> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> + return -1;
> + }
> +
> + pi = &lcore_power_info[lcore_id];
> + if (num < pi->nb_freqs) {
> + RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
> + return 0;
> + }
> + rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
Why rte_memcpy?
> +
> + return pi->nb_freqs;
> +}
> +
> +uint32_t
> +power_pstate_cpufreq_get_freq(unsigned int lcore_id)
> +{
> + if (lcore_id >= RTE_MAX_LCORE) {
> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> + return RTE_POWER_INVALID_FREQ_INDEX;
> + }
> +
> + return lcore_power_info[lcore_id].curr_idx;
> +}
> +
<snip>
> + caps->turbo = !!(pi->turbo_available);
> +
> + return 0;
> +}
> diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
> new file mode 100644
> index 0000000..0fc917a
> --- /dev/null
> +++ b/lib/librte_power/power_pstate_cpufreq.h
> @@ -0,0 +1,218 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2010-2018 Intel Corporation
I believe it should be just 2018.
> + */
> +
> +#ifndef _POWER_PSTATE_CPUFREQ_H
> +#define _POWER_PSTATE_CPUFREQ_H
> +
> +/**
> + * @file
> + * RTE Power Management via Intel Pstate driver
> + */
> +
> +#include <rte_common.h>
> +#include <rte_byteorder.h>
> +#include <rte_log.h>
> +#include <rte_string_fns.h>
> +#include "rte_power.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
I don't think this is necessary. These extern declarations are only for
public API headers, so that C++ compilers could parse them. Since this
is not a public header, there's no need for extern C declaration.
> +
> +/**
> + * Initialize power management for a specific lcore. It will check and set the
> + * governor to performance for the lcore, get the available frequencies, and
> + * prepare to set new lcore frequency.
> + *
> + * @param lcore_id
> + * lcore id.
> + *
> + * @return
> + * - 0 on success.
> + * - Negative on error.
> + */
<snip>
> + *
> + * @return
> + * The number of available frequencies.
> + */
> +uint32_t power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs,
> + uint32_t num);
> +
> +/**
> + * Return the current index of available frequencies of a specific lcore. It
> + * will return 'RTE_POWER_INVALID_FREQ_INDEX = (~0)' if error.
This isn't a public API, so it's nitpicking, but any notes about return
value should be in the @return section.
> + * It should be protected outside of this function for threadsafe.
> + *
> + * @param lcore_id
> + * lcore id.
> + *
> + * @return
> + * The current index of available frequencies.
> + */
> +uint32_t power_pstate_cpufreq_get_freq(unsigned int lcore_id);
> +
> +/**
<snip>
> #include "rte_power.h"
> #include "power_acpi_cpufreq.h"
> #include "power_kvm_vm.h"
> +#include "power_pstate_cpufreq.h"
> #include "power_common.h"
>
> enum power_management_env global_default_env = PM_ENV_NOT_SET;
> @@ -29,6 +30,8 @@ rte_power_get_capabilities_t rte_power_get_capabilities;
> int
> rte_power_set_env(enum power_management_env env)
> {
> +
> +
This is probably unintended whitespace change.
> if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
> return 0;
> }
> @@ -56,6 +59,19 @@ rte_power_set_env(enum power_management_env env)
> rte_power_freq_enable_turbo = power_kvm_vm_enable_turbo;
> rte_power_freq_disable_turbo = power_kvm_vm_disable_turbo;
> rte_power_get_capabilities = power_kvm_vm_get_capabilities;
> + } else if (env == PM_ENV_PSTATE_CPUFREQ) {
> + rte_power_freqs = power_pstate_cpufreq_freqs;
> + rte_power_get_freq = power_pstate_cpufreq_get_freq;
<snip>
> + if (global_default_env == PM_ENV_KVM_VM)
> return power_kvm_vm_init(lcore_id);
> - }
> +
> + if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
> + return power_pstate_cpufreq_init(lcore_id);
> +
switch?
> /* Auto detect Environment */
> RTE_LOG(INFO, POWER, "Attempting to initialise ACPI cpufreq power "
> "management...\n");
> @@ -99,13 +118,23 @@ rte_power_init(unsigned int lcore_id)
> goto out;
> }
>
> - RTE_LOG(INFO, POWER, "Attempting to initialise VM power management...\n");
> + RTE_LOG(INFO, POWER, "Attempting to initialise PSTAT power "
> + "management...\n");
> + ret = power_pstate_cpufreq_init(lcore_id);
> + if (ret == 0) {
> + rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
> + goto out;
> + }
> +
> + RTE_LOG(INFO, POWER, "Attempting to initialise VM power "
> + "management...\n");
> ret = power_kvm_vm_init(lcore_id);
> if (ret == 0) {
> rte_power_set_env(PM_ENV_KVM_VM);
> goto out;
> }
> - RTE_LOG(ERR, POWER, "Unable to set Power Management Environment for lcore "
> + RTE_LOG(ERR, POWER, "Unable to set Power Management "
> + "Environment for lcore "
> "%u\n", lcore_id);
Perhaps this could be moved to a separate function, so that the code
could look like:
switch (global_default_env) {
case PM_ENV_ACPI:
return acpi_init();
case PM_ENV_KVM:
return kvm_init();
case PM_ENV_PSTATE:
return pstate_init();
default:
if (autodetect() < 0)
RTE_LOG("error");
return 0;
}
Or something like that.
> out:
> return ret;
> @@ -118,6 +147,8 @@ rte_power_exit(unsigned int lcore_id)
> return power_acpi_cpufreq_exit(lcore_id);
> if (global_default_env == PM_ENV_KVM_VM)
> return power_kvm_vm_exit(lcore_id);
> + if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
> + return power_pstate_cpufreq_exit(lcore_id);
switch?
>
> RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
> "gracefully\n");
> diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
> index d70bc0b..c5e8f6b 100644
> --- a/lib/librte_power/rte_power.h
> +++ b/lib/librte_power/rte_power.h
> @@ -20,7 +20,8 @@ extern "C" {
> #endif
>
> /* Power Management Environment State */
> -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
> +enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
> + PM_ENV_PSTATE_CPUFREQ};
I don't like this approach. While it can be argued that application
needs to be explicitly aware of whether it's using native (ACPI or
pstate) vs. KVM power management, there's no real reason for an
application to differentiate between ACPI and pstate modes.
Changing this would of course require a deprecation notice, so for now,
can we hide all of this behind ACPI mode, and auto-detect whether we
want ACPI or pstate mode? IMO it would be better for the user
application to use a somewhat misnamed ACPI option for both ACPI and
pstate modes, than to needlessly care about whether one or the other is
in use.
What do you think?
>
> /**
> * Set the default power management implementation. If this is not called prior
>
On 10 Dec 16:08, Burakov, Anatoly wrote:
<snip>
> Hi Liang,
>
> General comment: please do not break up log strings onto multiple lines. It
> makes it harder to grep the codebase. Checkpatch will not warn about log
> strings going over 80 characters.
agree, I will update in next version
>
> > ---
> > lib/librte_power/Makefile | 2 +
> > lib/librte_power/meson.build | 4 +-
> > lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
> > lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
> > lib/librte_power/rte_power.c | 43 +-
> > lib/librte_power/rte_power.h | 3 +-
> > 6 files changed, 1039 insertions(+), 9 deletions(-)
> > create mode 100644 lib/librte_power/power_pstate_cpufreq.c
> > create mode 100644 lib/librte_power/power_pstate_cpufreq.h
>
> <snip>
>
> > sources = files('rte_power.c', 'power_acpi_cpufreq.c',
> > 'power_kvm_vm.c', 'guest_channel.c',
> > - 'rte_power_empty_poll.c')
> > + 'rte_power_empty_poll.c',
> > + 'power_pstate_cpufreq.c')
> > headers = files('rte_power.h','rte_power_empty_poll.h')
> > -deps += ['timer']
> > diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> > new file mode 100644
> > index 0000000..f1dada8
> > --- /dev/null
> > +++ b/lib/librte_power/power_pstate_cpufreq.c
> > @@ -0,0 +1,778 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
>
> I believe it should be 2018.
I didn't see anything wrong here.
>
> > + */
> > +
> > +#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 <errno.h>
> > +
> > +#include <rte_memcpy.h>
> > +#include <rte_atomic.h>
> > +
> > +#include "power_pstate_cpufreq.h"
> > +#include "power_common.h"
> > +
> <snip>
>
> > +
> > +static struct pstate_power_info lcore_power_info[RTE_MAX_LCORE];
> > +
> > +/**
> > + * It is to read the specific MSR.
> > + */
> > +
> > +static int32_t
> > +power_rdmsr(int msr, uint64_t *val, unsigned int lcore_id)
> > +{
> > + int fd;
> > + int ret;
>
> int fd, ret?a
should be fine.
>
> > + char fullpath[PATH_MAX];
> > +
> > + snprintf(fullpath, sizeof(fullpath), POWER_MSR_PATH, lcore_id);
> > +
> > + fd = open(fullpath, O_RDONLY);
> > +
> > + if (fd < 0) {
> > +
> > + if (errno == EACCES)
> > + RTE_LOG(ERR, POWER, "No access to %s\n", fullpath);
> > +
> > + if (errno == ENXIO)
> > + RTE_LOG(ERR, POWER, "%s Not Exist!\n", fullpath);
>
> How about:
>
> RTE_LOG(ERR, POWER, "Error opening '%s': %s\n", fullpath, strerror(errno));
agree, I will update in next version
>
> ?
>
> > +
> > + return fd;
> > + }
> > +
> > + ret = pread(fd, val, sizeof(uint64_t), msr);
>
> Can pread fail?
agree, I will add error checking here.
>
> > +
> > + close(fd);
> > +
> > + POWER_DEBUG_TRACE("MSR Path %s, offset 0x%X for lcore %u\n",
> > + fullpath, msr, lcore_id);
> > +
> > + POWER_DEBUG_TRACE("Ret value %d, content is 0x%lx\n", ret, *val);
> > +
> > + return ret;
> > +}
> > +
>
> <snip>
>
> > + if (fseek(pi->f_cur_max, 0, SEEK_SET) < 0) {
> > + RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
> > + "for setting frequency for lcore %u\n",
> > + pi->lcore_id);
> > + return -1;
> > + }
> > +
> > + /* Turbo is available and enabled, first freq bucket is sys max freq */
> > + if (pi->turbo_available && pi->turbo_enable && (idx == 0))
> > +
> > + target_freq = pi->sys_max_freq;
> > +
> > + else
> > +
> > + target_freq = pi->freqs[idx];
>
> Unneeded whitespace?
agree, I will remove it.
> > +
> > +
> > + /* Decrease freq, the min freq should be updated first */
> > + if (idx > pi->curr_idx) {
> > +
> > + if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
> > + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> > + "lcore %u\n", pi->lcore_id);
> > + return -1;
> > + }
> > +
> > + if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
> > + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> > + "lcore %u\n", pi->lcore_id);
> > + return -1;
> > + }
> > +
> > + POWER_DEBUG_TRACE("Freqency[%u] to be set for lcore %u\n",
> > + target_freq, pi->lcore_id);
>
> Frequency :)
>
> Also, i believe "Frequency[%u]" is misleading in this case, because the
> value is not an index into an array, but is an actual target frequency. I
> think "Frequency '%u'" would be more descriptive.
agree I will update in next version
>
> > +
> > + fflush(pi->f_cur_min);
> > + fflush(pi->f_cur_max);
> > +
> > + }
> > +
> > + /* Increase freq, the max freq should be updated first */
> > + if (idx < pi->curr_idx) {
>
> Else if?
no need here.
>
> > +
> > + if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
> > + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> > + "lcore %u\n", pi->lcore_id);
> > + return -1;
> > + }
> > +
> > + if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
> > + RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> > + "lcore %u\n", pi->lcore_id);
> > + return -1;
> > + }
>
> Forgot POWER_DEBUG_TRACE?
>
agree, I will add here.
> > +
> > + fflush(pi->f_cur_max);
> > + fflush(pi->f_cur_min);
> > + }
> > +
> > + pi->curr_idx = idx;
> > +
> > + return 1;
> > +}
> > +
>
> <snip>
>
> > + snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
> > + pi->lcore_id);
> > + f = fopen(fullpath, "rw+");
> > + FOPEN_OR_ERR_RET(f, ret);
> > +
> > + s = fgets(buf, sizeof(buf), f);
> > + FOPS_OR_NULL_GOTO(s, out);
> > +
> > + /* Check if current governor is performance */
> > + if (strncmp(buf, POWER_GOVERNOR_PERF,
> > + sizeof(POWER_GOVERNOR_PERF)) == 0) {
>
> Nitpick, but probably should be strlen, not sizeof?
sizeof should be fine
>
> > + ret = 0;
> > + POWER_DEBUG_TRACE("Power management governor of lcore %u is "
> > + "already performance\n", pi->lcore_id);
> > + goto out;
> > + }
> > + /* Save the original governor */
> > + snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
> > +
> > + /* Write 'performance' to the governor */
> > + val = fseek(f, 0, SEEK_SET);
> > + FOPS_OR_ERR_GOTO(val, out);
> > +
> > + val = fputs(POWER_GOVERNOR_PERF, f);
> > + FOPS_OR_ERR_GOTO(val, out);
> > +
> > + ret = 0;
> > + RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
> > + "set to performance successfully\n", pi->lcore_id);
>
> Do we want this as INFO? (it's OK if we do, just asking!)
just keep it as same as acpi driver
>
> > +out:
> > + fclose(f);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * It is to check the governor and then set the original governor back if
> > + * needed by writing the sys file.
> > + */
> > +static int
> > +power_set_governor_original(struct pstate_power_info *pi)
> > +{
>
> <snip>
>
> > + /* Write back the original governor */
> > + val = fseek(f, 0, SEEK_SET);
> > + FOPS_OR_ERR_GOTO(val, out);
> > +
> > + val = fputs(pi->governor_ori, f);
> > + FOPS_OR_ERR_GOTO(val, out);
> > +
> > + ret = 0;
> > + RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
> > + "has been set back to %s successfully\n",
> > + pi->lcore_id, pi->governor_ori);
>
> Same - do we want this as INFO?
keep it as same as acpi driver
>
> > +out:
> > + fclose(f);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * It is to get the available frequencies of the specific lcore by reading the
> > + * sys file.
> > + */
> > +static int
> > +power_get_available_freqs(struct pstate_power_info *pi)
> > +{
> > + FILE *f_min, *f_max;
> > + int ret = -1;
> > + char *p_min, *p_max;
> > + char buf_min[BUFSIZ];
> > + char buf_max[BUFSIZ];
> > + char fullpath_min[PATH_MAX];
> > + char fullpath_max[PATH_MAX];
> > + char *s_min, *s_max;
> > + uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
> > + uint32_t i, num_freqs = 0;
> > +
> > + snprintf(fullpath_max, sizeof(fullpath_max),
> > + POWER_SYSFILE_BASE_MAX_FREQ,
> > + pi->lcore_id);
> > + snprintf(fullpath_min, sizeof(fullpath_min),
> > + POWER_SYSFILE_BASE_MIN_FREQ,
> > + pi->lcore_id);
> > +
> > + f_min = fopen(fullpath_min, "r");
> > + FOPEN_OR_ERR_RET(f_min, ret);
> > +
> > + s_min = fgets(buf_min, sizeof(buf_min), f_min);
> > + FOPS_OR_NULL_GOTO(s_min, out);
> > +
> > + f_max = fopen(fullpath_max, "r");
> > + FOPEN_OR_ERR_RET(f_max, ret);
> > +
> > + s_max = fgets(buf_max, sizeof(buf_max), f_max);
> > + FOPS_OR_NULL_GOTO(s_max, out);
> > +
> > +
> > + /* Strip the line break if there is */
> > + p_min = strchr(buf_min, '\n');
> > + if (p_min != NULL)
> > + *p_min = 0;
> > +
> > + p_max = strchr(buf_max, '\n');
> > + if (p_max != NULL)
> > + *p_max = 0;
>
> Probably '\0' would be more correct.
should be fine
>
> > +
> > + sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
> > + sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
> > +
> > + if (sys_max_freq < sys_min_freq)
> > + goto out;
>
> This leaks fds. See below.
agree, I will adjust the postion of label "out"
>
> > +
> > +
> > + pi->sys_max_freq = sys_max_freq;
> > +
> > + base_max_freq = pi->non_turbo_max_ratio*BUS_FREQ;
>
> spacing around multiplication :)
agree. I will remove that
>
> > +
> > + POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
> > + sys_min_freq,
> > + sys_max_freq,
> > + base_max_freq);
> > +
> > + if (base_max_freq < sys_max_freq)
> > +
> > + pi->turbo_available = 1;
>
> Extra whitespace.
agree
>
> > + else
> > + pi->turbo_available = 0;
> > +
> > +
> > + /* If turbo is available then there is one extra freq bucket
> > + * to store the sys max freq which value is base_max +1
> > + */
> > + num_freqs = (base_max_freq - sys_min_freq)/BUS_FREQ + 1
> > + + pi->turbo_available;
>
> The '+' should be on the preceding line, also spacing.
agree
>
> > +
> > + /* Generate the freq bucket array.
> > + * If turbo is available the freq bucket[0] value is base_max +1
> > + * the bucket[1] is base_max, bucket[2] is base_max - BUS_FREQ
> > + * and so on.
> > + * If turbo is not available bucket[0] is base_max and so on
> > + */
> > + for (i = 0, pi->nb_freqs = 0; i < num_freqs; i++) {
> > +
>
> Extra whitespace.
agree
>
> > + if ((i == 0) && pi->turbo_available)
> > + pi->freqs[pi->nb_freqs++] = base_max_freq + 1;
> > + else
> > + pi->freqs[pi->nb_freqs++] =
> > + base_max_freq - (i - pi->turbo_available)*BUS_FREQ;
>
> Misleading indentation, also spacing around multiplication operator.
agree
> > + }
> > +
> > + ret = 0;
> > +
> > + POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are available\n",
> > + num_freqs, pi->lcore_id);
> > +
> > + fclose(f_min);
> > + fclose(f_max);
> > +
> > +
> > +out:
>
> fclose should be after out, otherwise error path above will leak fd.
agree
> > + return ret;
> > +}
> > +
> > +int
> > +power_pstate_cpufreq_init(unsigned int lcore_id)
> > +{
> > + struct pstate_power_info *pi;
> > +
> > + if (lcore_id >= RTE_MAX_LCORE) {
> > + RTE_LOG(ERR, POWER, "Lcore id %u can not exceeds %u\n",
>
> Cannot exceed.
>
agree
> > + lcore_id, RTE_MAX_LCORE - 1U);
> > + return -1;
> > + }
> > +
> > + pi = &lcore_power_info[lcore_id];
> > + if (rte_atomic32_cmpset(&(pi->state), POWER_IDLE, POWER_ONGOING)
> > + == 0) {
> > + RTE_LOG(INFO, POWER, "Power management of lcore %u is "
> > + "in use\n", lcore_id);
> > + return -1;
> > + }
> > +
> > + pi->lcore_id = lcore_id;
>
> Can we not set this until we're sure we didn't fail? Or do any of the below
> calls rely on pi->lcore_id to be set?
>
power_set_governor_performance will sue lcore_id
> > + /* Check and set the governor */
> > + if (power_set_governor_performance(pi) < 0) {
> > + RTE_LOG(ERR, POWER, "Cannot set governor of lcore %u to "
> > + "performance\n", lcore_id);
> > + goto fail;
> > + }
> > + /* Init for setting lcore frequency */
> > + if (power_init_for_setting_freq(pi) < 0) {
> > + RTE_LOG(ERR, POWER, "Cannot init for setting frequency for "
> > + "lcore %u\n", lcore_id);
> > + goto fail;
> > + }
> > +
> > + /* Get the available frequencies */
> > + if (power_get_available_freqs(pi) < 0) {
> > + RTE_LOG(ERR, POWER, "Cannot get available frequencies of "
> > + "lcore %u\n", lcore_id);
> > + goto fail;
> > + }
> > +
> > +
> > + /* Set freq to max by default */
> > + if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
> > + RTE_LOG(ERR, POWER, "Cannot set frequency of lcore %u "
> > + "to max\n", lcore_id);
> > + goto fail;
> > + }
> > +
> > + RTE_LOG(INFO, POWER, "Initialized successfully for lcore %u "
> > + "power management\n", lcore_id);
>
> Do we want this as INFO?
>
> > + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_USED);
> > +
> > + return 0;
> > +
> > +fail:
> > + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
> > +
> > + return -1;
> > +}
> > +
> > +int
> > +power_pstate_cpufreq_exit(unsigned int lcore_id)
> > +{
> > + struct pstate_power_info *pi;
> > +
>
> <snip>
>
> > + /* Set the governor back to the original */
> > + if (power_set_governor_original(pi) < 0) {
> > + RTE_LOG(ERR, POWER, "Cannot set the governor of %u back "
> > + "to the original\n", lcore_id);
> > + goto fail;
> > + }
> > +
> > + RTE_LOG(INFO, POWER, "Power management of lcore %u has exited from "
> > + "'performance' mode and been set back to the "
> > + "original\n", lcore_id);
>
> Perhaps print out the original governor name? Also, i think it's better to
> use the performance string define, rather than having it as part of
> formatted message.
original governor name is alread print out by power_set_governor_original
> > + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_IDLE);
> > +
> > + return 0;
> > +
> > +fail:
> > + rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
> > +
> > + return -1;
> > +}
> > +
> > +
> > +uint32_t
> > +power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
> > +{
> > + struct pstate_power_info *pi;
> > +
> > + if (lcore_id >= RTE_MAX_LCORE) {
> > + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> > + return -1;
> > + }
> > +
> > + pi = &lcore_power_info[lcore_id];
> > + if (num < pi->nb_freqs) {
> > + RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
> > + return 0;
> > + }
> > + rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
>
> Why rte_memcpy?
the table can be large
> > +
> > + return pi->nb_freqs;
> > +}
> > +
> > +uint32_t
> > +power_pstate_cpufreq_get_freq(unsigned int lcore_id)
> > +{
> > + if (lcore_id >= RTE_MAX_LCORE) {
> > + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> > + return RTE_POWER_INVALID_FREQ_INDEX;
> > + }
> > +
> > + return lcore_power_info[lcore_id].curr_idx;
> > +}
> > +
>
> <snip>
>
> > + caps->turbo = !!(pi->turbo_available);
> > +
> > + return 0;
> > +}
> > diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
> > new file mode 100644
> > index 0000000..0fc917a
> > --- /dev/null
> > +++ b/lib/librte_power/power_pstate_cpufreq.h
> > @@ -0,0 +1,218 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2010-2018 Intel Corporation
>
> I believe it should be just 2018.
>
> > + */
> > +
> > +#ifndef _POWER_PSTATE_CPUFREQ_H
> > +#define _POWER_PSTATE_CPUFREQ_H
> > +
> > +/**
> > + * @file
> > + * RTE Power Management via Intel Pstate driver
> > + */
> > +
> > +#include <rte_common.h>
> > +#include <rte_byteorder.h>
> > +#include <rte_log.h>
> > +#include <rte_string_fns.h>
> > +#include "rte_power.h"
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
>
> I don't think this is necessary. These extern declarations are only for
> public API headers, so that C++ compilers could parse them. Since this is
> not a public header, there's no need for extern C declaration.
just keep as same as other headers
>
> > +
> > +/**
> > + * Initialize power management for a specific lcore. It will check and set the
> > + * governor to performance for the lcore, get the available frequencies, and
> > + * prepare to set new lcore frequency.
> > + *
> > + * @param lcore_id
> > + * lcore id.
> > + *
> > + * @return
> > + * - 0 on success.
> > + * - Negative on error.
> > + */
>
> <snip>
>
> > + *
> > + * @return
> > + * The number of available frequencies.
> > + */
> > +uint32_t power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs,
> > + uint32_t num);
> > +
> > +/**
> > + * Return the current index of available frequencies of a specific lcore. It
> > + * will return 'RTE_POWER_INVALID_FREQ_INDEX = (~0)' if error.
>
> This isn't a public API, so it's nitpicking, but any notes about return
> value should be in the @return section.
agree
>
> > + * It should be protected outside of this function for threadsafe.
> > + *
> > + * @param lcore_id
> > + * lcore id.
> > + *
> > + * @return
> > + * The current index of available frequencies.
> > + */
> > +uint32_t power_pstate_cpufreq_get_freq(unsigned int lcore_id);
> > +
> > +/**
>
> <snip>
>
> > #include "rte_power.h"
> > #include "power_acpi_cpufreq.h"
> > #include "power_kvm_vm.h"
> > +#include "power_pstate_cpufreq.h"
> > #include "power_common.h"
> > enum power_management_env global_default_env = PM_ENV_NOT_SET;
> > @@ -29,6 +30,8 @@ rte_power_get_capabilities_t rte_power_get_capabilities;
> > int
> > rte_power_set_env(enum power_management_env env)
> > {
> > +
> > +
>
> This is probably unintended whitespace change.
>
agree
> > if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
> > return 0;
> > }
> > @@ -56,6 +59,19 @@ rte_power_set_env(enum power_management_env env)
> > rte_power_freq_enable_turbo = power_kvm_vm_enable_turbo;
> > rte_power_freq_disable_turbo = power_kvm_vm_disable_turbo;
> > rte_power_get_capabilities = power_kvm_vm_get_capabilities;
> > + } else if (env == PM_ENV_PSTATE_CPUFREQ) {
> > + rte_power_freqs = power_pstate_cpufreq_freqs;
> > + rte_power_get_freq = power_pstate_cpufreq_get_freq;
>
> <snip>
>
> > + if (global_default_env == PM_ENV_KVM_VM)
> > return power_kvm_vm_init(lcore_id);
> > - }
> > +
> > + if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
> > + return power_pstate_cpufreq_init(lcore_id);
> > +
>
> switch?
should be fine for only 3 enum type
>
> > /* Auto detect Environment */
> > RTE_LOG(INFO, POWER, "Attempting to initialise ACPI cpufreq power "
> > "management...\n");
> > @@ -99,13 +118,23 @@ rte_power_init(unsigned int lcore_id)
> > goto out;
> > }
> > - RTE_LOG(INFO, POWER, "Attempting to initialise VM power management...\n");
> > + RTE_LOG(INFO, POWER, "Attempting to initialise PSTAT power "
> > + "management...\n");
> > + ret = power_pstate_cpufreq_init(lcore_id);
> > + if (ret == 0) {
> > + rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
> > + goto out;
> > + }
> > +
> > + RTE_LOG(INFO, POWER, "Attempting to initialise VM power "
> > + "management...\n");
> > ret = power_kvm_vm_init(lcore_id);
> > if (ret == 0) {
> > rte_power_set_env(PM_ENV_KVM_VM);
> > goto out;
> > }
> > - RTE_LOG(ERR, POWER, "Unable to set Power Management Environment for lcore "
> > + RTE_LOG(ERR, POWER, "Unable to set Power Management "
> > + "Environment for lcore "
> > "%u\n", lcore_id);
>
> Perhaps this could be moved to a separate function, so that the code could
> look like:
>
> switch (global_default_env) {
> case PM_ENV_ACPI:
> return acpi_init();
> case PM_ENV_KVM:
> return kvm_init();
> case PM_ENV_PSTATE:
> return pstate_init();
> default:
> if (autodetect() < 0)
> RTE_LOG("error");
> return 0;
> }
>
> Or something like that.
I can move to switch
>
> > out:
> > return ret;
> > @@ -118,6 +147,8 @@ rte_power_exit(unsigned int lcore_id)
> > return power_acpi_cpufreq_exit(lcore_id);
> > if (global_default_env == PM_ENV_KVM_VM)
> > return power_kvm_vm_exit(lcore_id);
> > + if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
> > + return power_pstate_cpufreq_exit(lcore_id);
>
> switch?
>
> > RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
> > "gracefully\n");
> > diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
> > index d70bc0b..c5e8f6b 100644
> > --- a/lib/librte_power/rte_power.h
> > +++ b/lib/librte_power/rte_power.h
> > @@ -20,7 +20,8 @@ extern "C" {
> > #endif
> > /* Power Management Environment State */
> > -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
> > +enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
> > + PM_ENV_PSTATE_CPUFREQ};
>
> I don't like this approach. While it can be argued that application needs to
> be explicitly aware of whether it's using native (ACPI or pstate) vs. KVM
> power management, there's no real reason for an application to differentiate
> between ACPI and pstate modes.
>
> Changing this would of course require a deprecation notice, so for now, can
> we hide all of this behind ACPI mode, and auto-detect whether we want ACPI
> or pstate mode? IMO it would be better for the user application to use a
> somewhat misnamed ACPI option for both ACPI and pstate modes, than to
> needlessly care about whether one or the other is in use.
>
> What do you think?
acpi has significant difference with hwp. sysfs/xxxx/setspeed sysfs/xxxx/scaling_driver is different
I would like to make application aware the driver type.
>
> > /**
> > * Set the default power management implementation. If this is not called prior
> >
>
> --
> Thanks,
> Anatoly
On 13-Dec-18 10:58 AM, Liang, Ma wrote:
> On 10 Dec 16:08, Burakov, Anatoly wrote:
> <snip>
>> Hi Liang,
>>
>> General comment: please do not break up log strings onto multiple lines. It
>> makes it harder to grep the codebase. Checkpatch will not warn about log
>> strings going over 80 characters.
> agree, I will update in next version
>>
>>> ---
>>> lib/librte_power/Makefile | 2 +
>>> lib/librte_power/meson.build | 4 +-
>>> lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
>>> lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
>>> lib/librte_power/rte_power.c | 43 +-
>>> lib/librte_power/rte_power.h | 3 +-
>>> 6 files changed, 1039 insertions(+), 9 deletions(-)
>>> create mode 100644 lib/librte_power/power_pstate_cpufreq.c
>>> create mode 100644 lib/librte_power/power_pstate_cpufreq.h
>>
>> <snip>
>>
>>> sources = files('rte_power.c', 'power_acpi_cpufreq.c',
>>> 'power_kvm_vm.c', 'guest_channel.c',
>>> - 'rte_power_empty_poll.c')
>>> + 'rte_power_empty_poll.c',
>>> + 'power_pstate_cpufreq.c')
>>> headers = files('rte_power.h','rte_power_empty_poll.h')
>>> -deps += ['timer']
>>> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
>>> new file mode 100644
>>> index 0000000..f1dada8
>>> --- /dev/null
>>> +++ b/lib/librte_power/power_pstate_cpufreq.c
>>> @@ -0,0 +1,778 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2010-2018 Intel Corporation
>>
>> I believe it should be 2018.
> I didn't see anything wrong here.
The copyright on the file should start when it was first written. You're
created this file in 2018, not 2010 :)
>>
>>> + */
>>> +
>>> +#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 <errno.h>
>>> +
>>> +#include <rte_memcpy.h>
>>> +#include <rte_atomic.h>
>>> +
>>> +#include "power_pstate_cpufreq.h"
>>> +#include "power_common.h"
>>> +
>> <snip>
>>
>>> +
<snip>
>>> +uint32_t
>>> +power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>>> +{
>>> + struct pstate_power_info *pi;
>>> +
>>> + if (lcore_id >= RTE_MAX_LCORE) {
>>> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
>>> + return -1;
>>> + }
>>> +
>>> + pi = &lcore_power_info[lcore_id];
>>> + if (num < pi->nb_freqs) {
>>> + RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
>>> + return 0;
>>> + }
>>> + rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
>>
>> Why rte_memcpy?
> the table can be large
So? :) This isn't a hotpath, so regular memcpy should be fine. I mean,
it's OK to use rte_memcpy as well, just seems weird.
>>> +
>>> + return pi->nb_freqs;
>>> +}
>>> +
>>> +uint32_t
>>> +power_pstate_cpufreq_get_freq(unsigned int lcore_id)
>>> +{
>>> + if (lcore_id >= RTE_MAX_LCORE) {
>>> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
>>> + return RTE_POWER_INVALID_FREQ_INDEX;
>>> + }
>>> +
>>> + return lcore_power_info[lcore_id].curr_idx;
>>> +}
>>> +
>>
>> <snip>
>>
>>> + caps->turbo = !!(pi->turbo_available);
>>> +
>>> + return 0;
>>> +}
>>> diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
>>> new file mode 100644
>>> index 0000000..0fc917a
>>> --- /dev/null
>>> +++ b/lib/librte_power/power_pstate_cpufreq.h
>>> @@ -0,0 +1,218 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2010-2018 Intel Corporation
>>
>> I believe it should be just 2018.
>>
>>> + */
>>> +
>>> +#ifndef _POWER_PSTATE_CPUFREQ_H
>>> +#define _POWER_PSTATE_CPUFREQ_H
>>> +
>>> +/**
>>> + * @file
>>> + * RTE Power Management via Intel Pstate driver
>>> + */
>>> +
>>> +#include <rte_common.h>
>>> +#include <rte_byteorder.h>
>>> +#include <rte_log.h>
>>> +#include <rte_string_fns.h>
>>> +#include "rte_power.h"
>>> +
>>> +#ifdef __cplusplus
>>> +extern "C" {
>>> +#endif
>>
>> I don't think this is necessary. These extern declarations are only for
>> public API headers, so that C++ compilers could parse them. Since this is
>> not a public header, there's no need for extern C declaration.
> just keep as same as other headers
Well, other headers have it wrong then, if they too specify this extern
declaration. This is only needed for public-facing headers. Don't copy
bad/unnecessary code from other files - consistency is important, but
not *that* important :)
>>
>>> +
>>> +/**
>>> + * Initialize power management for a specific lcore. It will check and set the
>>> + * governor to performance for the lcore, get the available frequencies, and
>>> + * prepare to set new lcore frequency.
>>> + *
>>> + * @param lcore_id
>>> + * lcore id.
>>> + *
>>> + * @return
>>> + * - 0 on success.
>>> + * - Negative on error.
>>> + */
>>
<snip>
>>
>>> RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
>>> "gracefully\n");
>>> diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
>>> index d70bc0b..c5e8f6b 100644
>>> --- a/lib/librte_power/rte_power.h
>>> +++ b/lib/librte_power/rte_power.h
>>> @@ -20,7 +20,8 @@ extern "C" {
>>> #endif
>>> /* Power Management Environment State */
>>> -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
>>> +enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
>>> + PM_ENV_PSTATE_CPUFREQ};
>>
>> I don't like this approach. While it can be argued that application needs to
>> be explicitly aware of whether it's using native (ACPI or pstate) vs. KVM
>> power management, there's no real reason for an application to differentiate
>> between ACPI and pstate modes.
>>
>> Changing this would of course require a deprecation notice, so for now, can
>> we hide all of this behind ACPI mode, and auto-detect whether we want ACPI
>> or pstate mode? IMO it would be better for the user application to use a
>> somewhat misnamed ACPI option for both ACPI and pstate modes, than to
>> needlessly care about whether one or the other is in use.
>>
>> What do you think?
> acpi has significant difference with hwp. sysfs/xxxx/setspeed sysfs/xxxx/scaling_driver is different
> I would like to make application aware the driver type.
Yes, however please correct me if i'm wrong - aren't all of these
changes abstracted away by the power library API? However different they
are internally, does the *application* really need to differentiate
between the two? Are there differences *to the user* when using power
API with pstate vs. ACPI? If not, then why do we need another env type
when we can handle the differences internally?
>>
>>> /**
>>> * Set the default power management implementation. If this is not called prior
>>>
>>
>> --
>> Thanks,
>> Anatoly
>
13 Dec 11:16, Burakov, Anatoly wrote:
> On 13-Dec-18 10:58 AM, Liang, Ma wrote:
> > On 10 Dec 16:08, Burakov, Anatoly wrote:
> > <snip>
> > > Hi Liang,
> > >
> > > General comment: please do not break up log strings onto multiple lines. It
> > > makes it harder to grep the codebase. Checkpatch will not warn about log
> > > strings going over 80 characters.
> > agree, I will update in next version
> > >
> > > > ---
> > > > lib/librte_power/Makefile | 2 +
> > > > lib/librte_power/meson.build | 4 +-
> > > > lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
> > > > lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
> > > > lib/librte_power/rte_power.c | 43 +-
> > > > lib/librte_power/rte_power.h | 3 +-
> > > > 6 files changed, 1039 insertions(+), 9 deletions(-)
> > > > create mode 100644 lib/librte_power/power_pstate_cpufreq.c
> > > > create mode 100644 lib/librte_power/power_pstate_cpufreq.h
> > >
> > > <snip>
> > >
> > > > sources = files('rte_power.c', 'power_acpi_cpufreq.c',
> > > > 'power_kvm_vm.c', 'guest_channel.c',
> > > > - 'rte_power_empty_poll.c')
> > > > + 'rte_power_empty_poll.c',
> > > > + 'power_pstate_cpufreq.c')
> > > > headers = files('rte_power.h','rte_power_empty_poll.h')
> > > > -deps += ['timer']
> > > > diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> > > > new file mode 100644
> > > > index 0000000..f1dada8
> > > > --- /dev/null
> > > > +++ b/lib/librte_power/power_pstate_cpufreq.c
> > > > @@ -0,0 +1,778 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(c) 2010-2018 Intel Corporation
> > >
> > > I believe it should be 2018.
> > I didn't see anything wrong here.
>
> The copyright on the file should start when it was first written. You're
> created this file in 2018, not 2010 :)
>
> > >
> > > > + */
> > > > +
> > > > +#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 <errno.h>
> > > > +
> > > > +#include <rte_memcpy.h>
> > > > +#include <rte_atomic.h>
> > > > +
> > > > +#include "power_pstate_cpufreq.h"
> > > > +#include "power_common.h"
> > > > +
> > > <snip>
> > >
> > > > +
>
> <snip>
>
> > > > +uint32_t
> > > > +power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
> > > > +{
> > > > + struct pstate_power_info *pi;
> > > > +
> > > > + if (lcore_id >= RTE_MAX_LCORE) {
> > > > + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> > > > + return -1;
> > > > + }
> > > > +
> > > > + pi = &lcore_power_info[lcore_id];
> > > > + if (num < pi->nb_freqs) {
> > > > + RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
> > > > + return 0;
> > > > + }
> > > > + rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
> > >
> > > Why rte_memcpy?
> > the table can be large
>
> So? :) This isn't a hotpath, so regular memcpy should be fine. I mean, it's
> OK to use rte_memcpy as well, just seems weird.
>
> > > > +
> > > > + return pi->nb_freqs;
> > > > +}
> > > > +
> > > > +uint32_t
> > > > +power_pstate_cpufreq_get_freq(unsigned int lcore_id)
> > > > +{
> > > > + if (lcore_id >= RTE_MAX_LCORE) {
> > > > + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
> > > > + return RTE_POWER_INVALID_FREQ_INDEX;
> > > > + }
> > > > +
> > > > + return lcore_power_info[lcore_id].curr_idx;
> > > > +}
> > > > +
> > >
> > > <snip>
> > >
> > > > + caps->turbo = !!(pi->turbo_available);
> > > > +
> > > > + return 0;
> > > > +}
> > > > diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
> > > > new file mode 100644
> > > > index 0000000..0fc917a
> > > > --- /dev/null
> > > > +++ b/lib/librte_power/power_pstate_cpufreq.h
> > > > @@ -0,0 +1,218 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(c) 2010-2018 Intel Corporation
> > >
> > > I believe it should be just 2018.
> > >
> > > > + */
> > > > +
> > > > +#ifndef _POWER_PSTATE_CPUFREQ_H
> > > > +#define _POWER_PSTATE_CPUFREQ_H
> > > > +
> > > > +/**
> > > > + * @file
> > > > + * RTE Power Management via Intel Pstate driver
> > > > + */
> > > > +
> > > > +#include <rte_common.h>
> > > > +#include <rte_byteorder.h>
> > > > +#include <rte_log.h>
> > > > +#include <rte_string_fns.h>
> > > > +#include "rte_power.h"
> > > > +
> > > > +#ifdef __cplusplus
> > > > +extern "C" {
> > > > +#endif
> > >
> > > I don't think this is necessary. These extern declarations are only for
> > > public API headers, so that C++ compilers could parse them. Since this is
> > > not a public header, there's no need for extern C declaration.
> > just keep as same as other headers
>
> Well, other headers have it wrong then, if they too specify this extern
> declaration. This is only needed for public-facing headers. Don't copy
> bad/unnecessary code from other files - consistency is important, but not
> *that* important :)
>
> > >
> > > > +
> > > > +/**
> > > > + * Initialize power management for a specific lcore. It will check and set the
> > > > + * governor to performance for the lcore, get the available frequencies, and
> > > > + * prepare to set new lcore frequency.
> > > > + *
> > > > + * @param lcore_id
> > > > + * lcore id.
> > > > + *
> > > > + * @return
> > > > + * - 0 on success.
> > > > + * - Negative on error.
> > > > + */
> > >
>
> <snip>
>
> > >
> > > > RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
> > > > "gracefully\n");
> > > > diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
> > > > index d70bc0b..c5e8f6b 100644
> > > > --- a/lib/librte_power/rte_power.h
> > > > +++ b/lib/librte_power/rte_power.h
> > > > @@ -20,7 +20,8 @@ extern "C" {
> > > > #endif
> > > > /* Power Management Environment State */
> > > > -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
> > > > +enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
> > > > + PM_ENV_PSTATE_CPUFREQ};
> > >
> > > I don't like this approach. While it can be argued that application needs to
> > > be explicitly aware of whether it's using native (ACPI or pstate) vs. KVM
> > > power management, there's no real reason for an application to differentiate
> > > between ACPI and pstate modes.
> > >
> > > Changing this would of course require a deprecation notice, so for now, can
> > > we hide all of this behind ACPI mode, and auto-detect whether we want ACPI
> > > or pstate mode? IMO it would be better for the user application to use a
> > > somewhat misnamed ACPI option for both ACPI and pstate modes, than to
> > > needlessly care about whether one or the other is in use.
> > >
> > > What do you think?
> > acpi has significant difference with hwp. sysfs/xxxx/setspeed sysfs/xxxx/scaling_driver is different
> > I would like to make application aware the driver type.
>
> Yes, however please correct me if i'm wrong - aren't all of these changes
> abstracted away by the power library API? However different they are
> internally, does the *application* really need to differentiate between the
> two? Are there differences *to the user* when using power API with pstate
> vs. ACPI? If not, then why do we need another env type when we can handle
> the differences internally?
>
Currently, rte_power_init will try all three subsystem one by one to figure out the right driver.
however, the rte_power_set_env as a interface is keep here for flexibility I thought.
also that's the original design for the library. if we find better way, we can implementa that
in the future. but that's other patch then.;-)
> > >
> > > > /**
> > > > * Set the default power management implementation. If this is not called prior
> > > >
> > >
> > > --
> > > Thanks,
> > > Anatoly
> >
>
>
> --
> Thanks,
> Anatoly
On 13-Dec-18 1:46 PM, Liang, Ma wrote:
> 13 Dec 11:16, Burakov, Anatoly wrote:
>> On 13-Dec-18 10:58 AM, Liang, Ma wrote:
>>> On 10 Dec 16:08, Burakov, Anatoly wrote:
>>> <snip>
>>>> Hi Liang,
>>>>
>>>> General comment: please do not break up log strings onto multiple lines. It
>>>> makes it harder to grep the codebase. Checkpatch will not warn about log
>>>> strings going over 80 characters.
>>> agree, I will update in next version
>>>>
>>>>> ---
>>>>> lib/librte_power/Makefile | 2 +
>>>>> lib/librte_power/meson.build | 4 +-
>>>>> lib/librte_power/power_pstate_cpufreq.c | 778 ++++++++++++++++++++++++++++++++
>>>>> lib/librte_power/power_pstate_cpufreq.h | 218 +++++++++
>>>>> lib/librte_power/rte_power.c | 43 +-
>>>>> lib/librte_power/rte_power.h | 3 +-
>>>>> 6 files changed, 1039 insertions(+), 9 deletions(-)
>>>>> create mode 100644 lib/librte_power/power_pstate_cpufreq.c
>>>>> create mode 100644 lib/librte_power/power_pstate_cpufreq.h
>>>>
>>>> <snip>
>>>>
>>>>> sources = files('rte_power.c', 'power_acpi_cpufreq.c',
>>>>> 'power_kvm_vm.c', 'guest_channel.c',
>>>>> - 'rte_power_empty_poll.c')
>>>>> + 'rte_power_empty_poll.c',
>>>>> + 'power_pstate_cpufreq.c')
>>>>> headers = files('rte_power.h','rte_power_empty_poll.h')
>>>>> -deps += ['timer']
>>>>> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
>>>>> new file mode 100644
>>>>> index 0000000..f1dada8
>>>>> --- /dev/null
>>>>> +++ b/lib/librte_power/power_pstate_cpufreq.c
>>>>> @@ -0,0 +1,778 @@
>>>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>>>> + * Copyright(c) 2010-2018 Intel Corporation
>>>>
>>>> I believe it should be 2018.
>>> I didn't see anything wrong here.
>>
>> The copyright on the file should start when it was first written. You're
>> created this file in 2018, not 2010 :)
>>
>>>>
>>>>> + */
>>>>> +
>>>>> +#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 <errno.h>
>>>>> +
>>>>> +#include <rte_memcpy.h>
>>>>> +#include <rte_atomic.h>
>>>>> +
>>>>> +#include "power_pstate_cpufreq.h"
>>>>> +#include "power_common.h"
>>>>> +
>>>> <snip>
>>>>
>>>>> +
>>
>> <snip>
>>
>>>>> +uint32_t
>>>>> +power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
>>>>> +{
>>>>> + struct pstate_power_info *pi;
>>>>> +
>>>>> + if (lcore_id >= RTE_MAX_LCORE) {
>>>>> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
>>>>> + return -1;
>>>>> + }
>>>>> +
>>>>> + pi = &lcore_power_info[lcore_id];
>>>>> + if (num < pi->nb_freqs) {
>>>>> + RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
>>>>> + return 0;
>>>>> + }
>>>>> + rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
>>>>
>>>> Why rte_memcpy?
>>> the table can be large
>>
>> So? :) This isn't a hotpath, so regular memcpy should be fine. I mean, it's
>> OK to use rte_memcpy as well, just seems weird.
>>
>>>>> +
>>>>> + return pi->nb_freqs;
>>>>> +}
>>>>> +
>>>>> +uint32_t
>>>>> +power_pstate_cpufreq_get_freq(unsigned int lcore_id)
>>>>> +{
>>>>> + if (lcore_id >= RTE_MAX_LCORE) {
>>>>> + RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
>>>>> + return RTE_POWER_INVALID_FREQ_INDEX;
>>>>> + }
>>>>> +
>>>>> + return lcore_power_info[lcore_id].curr_idx;
>>>>> +}
>>>>> +
>>>>
>>>> <snip>
>>>>
>>>>> + caps->turbo = !!(pi->turbo_available);
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> diff --git a/lib/librte_power/power_pstate_cpufreq.h b/lib/librte_power/power_pstate_cpufreq.h
>>>>> new file mode 100644
>>>>> index 0000000..0fc917a
>>>>> --- /dev/null
>>>>> +++ b/lib/librte_power/power_pstate_cpufreq.h
>>>>> @@ -0,0 +1,218 @@
>>>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>>>> + * Copyright(c) 2010-2018 Intel Corporation
>>>>
>>>> I believe it should be just 2018.
>>>>
>>>>> + */
>>>>> +
>>>>> +#ifndef _POWER_PSTATE_CPUFREQ_H
>>>>> +#define _POWER_PSTATE_CPUFREQ_H
>>>>> +
>>>>> +/**
>>>>> + * @file
>>>>> + * RTE Power Management via Intel Pstate driver
>>>>> + */
>>>>> +
>>>>> +#include <rte_common.h>
>>>>> +#include <rte_byteorder.h>
>>>>> +#include <rte_log.h>
>>>>> +#include <rte_string_fns.h>
>>>>> +#include "rte_power.h"
>>>>> +
>>>>> +#ifdef __cplusplus
>>>>> +extern "C" {
>>>>> +#endif
>>>>
>>>> I don't think this is necessary. These extern declarations are only for
>>>> public API headers, so that C++ compilers could parse them. Since this is
>>>> not a public header, there's no need for extern C declaration.
>>> just keep as same as other headers
>>
>> Well, other headers have it wrong then, if they too specify this extern
>> declaration. This is only needed for public-facing headers. Don't copy
>> bad/unnecessary code from other files - consistency is important, but not
>> *that* important :)
>>
>>>>
>>>>> +
>>>>> +/**
>>>>> + * Initialize power management for a specific lcore. It will check and set the
>>>>> + * governor to performance for the lcore, get the available frequencies, and
>>>>> + * prepare to set new lcore frequency.
>>>>> + *
>>>>> + * @param lcore_id
>>>>> + * lcore id.
>>>>> + *
>>>>> + * @return
>>>>> + * - 0 on success.
>>>>> + * - Negative on error.
>>>>> + */
>>>>
>>
>> <snip>
>>
>>>>
>>>>> RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
>>>>> "gracefully\n");
>>>>> diff --git a/lib/librte_power/rte_power.h b/lib/librte_power/rte_power.h
>>>>> index d70bc0b..c5e8f6b 100644
>>>>> --- a/lib/librte_power/rte_power.h
>>>>> +++ b/lib/librte_power/rte_power.h
>>>>> @@ -20,7 +20,8 @@ extern "C" {
>>>>> #endif
>>>>> /* Power Management Environment State */
>>>>> -enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
>>>>> +enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
>>>>> + PM_ENV_PSTATE_CPUFREQ};
>>>>
>>>> I don't like this approach. While it can be argued that application needs to
>>>> be explicitly aware of whether it's using native (ACPI or pstate) vs. KVM
>>>> power management, there's no real reason for an application to differentiate
>>>> between ACPI and pstate modes.
>>>>
>>>> Changing this would of course require a deprecation notice, so for now, can
>>>> we hide all of this behind ACPI mode, and auto-detect whether we want ACPI
>>>> or pstate mode? IMO it would be better for the user application to use a
>>>> somewhat misnamed ACPI option for both ACPI and pstate modes, than to
>>>> needlessly care about whether one or the other is in use.
>>>>
>>>> What do you think?
>>> acpi has significant difference with hwp. sysfs/xxxx/setspeed sysfs/xxxx/scaling_driver is different
>>> I would like to make application aware the driver type.
>>
>> Yes, however please correct me if i'm wrong - aren't all of these changes
>> abstracted away by the power library API? However different they are
>> internally, does the *application* really need to differentiate between the
>> two? Are there differences *to the user* when using power API with pstate
>> vs. ACPI? If not, then why do we need another env type when we can handle
>> the differences internally?
>>
> Currently, rte_power_init will try all three subsystem one by one to figure out the right driver.
> however, the rte_power_set_env as a interface is keep here for flexibility I thought.
> also that's the original design for the library. if we find better way, we can implementa that
> in the future. but that's other patch then.;-)
OK, fair enough so.
@@ -6,6 +6,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
# library name
LIB = librte_power.a
+CFLAGS += -DALLOW_EXPERIMENTAL_API
CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3 -fno-strict-aliasing
LDLIBS += -lrte_eal -lrte_timer
@@ -17,6 +18,7 @@ LIBABIVER := 1
SRCS-$(CONFIG_RTE_LIBRTE_POWER) := rte_power.c power_acpi_cpufreq.c
SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_kvm_vm.c guest_channel.c
SRCS-$(CONFIG_RTE_LIBRTE_POWER) += rte_power_empty_poll.c
+SRCS-$(CONFIG_RTE_LIBRTE_POWER) += power_pstate_cpufreq.c
# install this header file
SYMLINK-$(CONFIG_RTE_LIBRTE_POWER)-include := rte_power.h rte_power_empty_poll.h
@@ -6,6 +6,6 @@ if host_machine.system() != 'linux'
endif
sources = files('rte_power.c', 'power_acpi_cpufreq.c',
'power_kvm_vm.c', 'guest_channel.c',
- 'rte_power_empty_poll.c')
+ 'rte_power_empty_poll.c',
+ 'power_pstate_cpufreq.c')
headers = files('rte_power.h','rte_power_empty_poll.h')
-deps += ['timer']
new file mode 100644
@@ -0,0 +1,778 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2018 Intel Corporation
+ */
+
+#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 <errno.h>
+
+#include <rte_memcpy.h>
+#include <rte_atomic.h>
+
+#include "power_pstate_cpufreq.h"
+#include "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 POWER_CONVERT_TO_DECIMAL 10
+#define BUS_FREQ 100000
+
+#define POWER_GOVERNOR_PERF "performance"
+#define POWER_SYSFILE_GOVERNOR \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
+#define POWER_SYSFILE_MAX_FREQ \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_max_freq"
+#define POWER_SYSFILE_MIN_FREQ \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_min_freq"
+#define POWER_SYSFILE_CUR_FREQ \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_cur_freq"
+#define POWER_SYSFILE_BASE_MAX_FREQ \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_max_freq"
+#define POWER_SYSFILE_BASE_MIN_FREQ \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_min_freq"
+#define POWER_MSR_PATH "/dev/cpu/%u/msr"
+
+/*
+ * MSR related
+ */
+#define PLATFORM_INFO 0x0CE
+#define NON_TURBO_MASK 0xFF00
+#define NON_TURBO_OFFSET 0x8
+
+
+enum power_state {
+ POWER_IDLE = 0,
+ POWER_ONGOING,
+ POWER_USED,
+ POWER_UNKNOWN
+};
+
+struct pstate_power_info {
+ unsigned int lcore_id; /**< Logical core id */
+ uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
+ uint32_t nb_freqs; /**< number of available freqs */
+ FILE *f_cur_min; /**< FD of scaling_min */
+ FILE *f_cur_max; /**< FD of scaling_max */
+ char governor_ori[32]; /**< Original governor name */
+ uint32_t curr_idx; /**< Freq index in freqs array */
+ uint32_t non_turbo_max_ratio; /**< Non Turbo Max ratio */
+ uint32_t sys_max_freq; /**< system wide max freq */
+ volatile uint32_t state; /**< Power in use state */
+ uint16_t turbo_available; /**< Turbo Boost available */
+ uint16_t turbo_enable; /**< Turbo Boost enable/disable */
+} __rte_cache_aligned;
+
+
+static struct pstate_power_info lcore_power_info[RTE_MAX_LCORE];
+
+/**
+ * It is to read the specific MSR.
+ */
+
+static int32_t
+power_rdmsr(int msr, uint64_t *val, unsigned int lcore_id)
+{
+ int fd;
+ int ret;
+ char fullpath[PATH_MAX];
+
+ snprintf(fullpath, sizeof(fullpath), POWER_MSR_PATH, lcore_id);
+
+ fd = open(fullpath, O_RDONLY);
+
+ if (fd < 0) {
+
+ if (errno == EACCES)
+ RTE_LOG(ERR, POWER, "No access to %s\n", fullpath);
+
+ if (errno == ENXIO)
+ RTE_LOG(ERR, POWER, "%s Not Exist!\n", fullpath);
+
+ return fd;
+ }
+
+ ret = pread(fd, val, sizeof(uint64_t), msr);
+
+ close(fd);
+
+ POWER_DEBUG_TRACE("MSR Path %s, offset 0x%X for lcore %u\n",
+ fullpath, msr, lcore_id);
+
+ POWER_DEBUG_TRACE("Ret value %d, content is 0x%lx\n", ret, *val);
+
+ return ret;
+}
+
+/**
+ * It is to fopen the sys file for the future setting the lcore frequency.
+ */
+static int
+power_init_for_setting_freq(struct pstate_power_info *pi)
+{
+ FILE *f_min, *f_max;
+ char fullpath_min[PATH_MAX];
+ char fullpath_max[PATH_MAX];
+ uint64_t max_non_turbo = 0;
+
+ snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
+ pi->lcore_id);
+
+ f_min = fopen(fullpath_min, "rw+");
+ FOPEN_OR_ERR_RET(f_min, -1);
+
+ snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
+ pi->lcore_id);
+
+ f_max = fopen(fullpath_max, "rw+");
+ FOPEN_OR_ERR_RET(f_max, -1);
+
+ pi->f_cur_min = f_min;
+ pi->f_cur_max = f_max;
+
+ /* Add MSR read to detect turbo status */
+
+ if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+ return -1;
+
+ max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
+
+ POWER_DEBUG_TRACE("no turbo perf %lu\n", max_non_turbo);
+
+ pi->non_turbo_max_ratio = max_non_turbo;
+
+ return 0;
+}
+
+static int
+set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
+{
+ uint32_t target_freq = 0;
+
+ 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)
+ return 0;
+
+
+ /* Because Intel Pstate Driver only allow user change min/max hint
+ * User need change the min/max as same value.
+ */
+ if (fseek(pi->f_cur_min, 0, SEEK_SET) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
+ "for setting frequency for lcore %u\n",
+ pi->lcore_id);
+ return -1;
+ }
+
+ if (fseek(pi->f_cur_max, 0, SEEK_SET) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
+ "for setting frequency for lcore %u\n",
+ pi->lcore_id);
+ return -1;
+ }
+
+ /* Turbo is available and enabled, first freq bucket is sys max freq */
+ if (pi->turbo_available && pi->turbo_enable && (idx == 0))
+
+ target_freq = pi->sys_max_freq;
+
+ else
+
+ target_freq = pi->freqs[idx];
+
+
+ /* Decrease freq, the min freq should be updated first */
+ if (idx > pi->curr_idx) {
+
+ if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to write new frequency for "
+ "lcore %u\n", pi->lcore_id);
+ return -1;
+ }
+
+ if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to write new frequency for "
+ "lcore %u\n", pi->lcore_id);
+ return -1;
+ }
+
+ POWER_DEBUG_TRACE("Freqency[%u] to be set for lcore %u\n",
+ target_freq, pi->lcore_id);
+
+ fflush(pi->f_cur_min);
+ fflush(pi->f_cur_max);
+
+ }
+
+ /* Increase freq, the max freq should be updated first */
+ if (idx < pi->curr_idx) {
+
+ if (fprintf(pi->f_cur_max, "%u", target_freq) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to write new frequency for "
+ "lcore %u\n", pi->lcore_id);
+ return -1;
+ }
+
+ if (fprintf(pi->f_cur_min, "%u", target_freq) < 0) {
+ RTE_LOG(ERR, POWER, "Fail to write new frequency for "
+ "lcore %u\n", pi->lcore_id);
+ return -1;
+ }
+
+ fflush(pi->f_cur_max);
+ fflush(pi->f_cur_min);
+ }
+
+ pi->curr_idx = idx;
+
+ return 1;
+}
+
+/**
+ * It is to check the current scaling governor by reading sys file, and then
+ * set it into 'performance' if it is not by writing the sys file. The original
+ * governor will be saved for rolling back.
+ */
+static int
+power_set_governor_performance(struct pstate_power_info *pi)
+{
+ FILE *f;
+ int ret = -1;
+ char buf[BUFSIZ];
+ char fullpath[PATH_MAX];
+ char *s;
+ int val;
+
+ snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
+ pi->lcore_id);
+ f = fopen(fullpath, "rw+");
+ FOPEN_OR_ERR_RET(f, ret);
+
+ s = fgets(buf, sizeof(buf), f);
+ FOPS_OR_NULL_GOTO(s, out);
+
+ /* Check if current governor is performance */
+ if (strncmp(buf, POWER_GOVERNOR_PERF,
+ sizeof(POWER_GOVERNOR_PERF)) == 0) {
+ ret = 0;
+ POWER_DEBUG_TRACE("Power management governor of lcore %u is "
+ "already performance\n", pi->lcore_id);
+ goto out;
+ }
+ /* Save the original governor */
+ snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
+
+ /* Write 'performance' to the governor */
+ val = fseek(f, 0, SEEK_SET);
+ FOPS_OR_ERR_GOTO(val, out);
+
+ val = fputs(POWER_GOVERNOR_PERF, f);
+ FOPS_OR_ERR_GOTO(val, out);
+
+ ret = 0;
+ RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
+ "set to performance successfully\n", pi->lcore_id);
+out:
+ fclose(f);
+
+ return ret;
+}
+
+/**
+ * It is to check the governor and then set the original governor back if
+ * needed by writing the sys file.
+ */
+static int
+power_set_governor_original(struct pstate_power_info *pi)
+{
+ FILE *f;
+ int ret = -1;
+ char buf[BUFSIZ];
+ char fullpath[PATH_MAX];
+ char *s;
+ int val;
+
+ snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
+ pi->lcore_id);
+ f = fopen(fullpath, "rw+");
+ FOPEN_OR_ERR_RET(f, ret);
+
+ s = fgets(buf, sizeof(buf), f);
+ FOPS_OR_NULL_GOTO(s, out);
+
+ /* Check if the governor to be set is the same as current */
+ if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
+ ret = 0;
+ POWER_DEBUG_TRACE("Power management governor of lcore %u "
+ "has already been set to %s\n",
+ pi->lcore_id, pi->governor_ori);
+ goto out;
+ }
+
+ /* Write back the original governor */
+ val = fseek(f, 0, SEEK_SET);
+ FOPS_OR_ERR_GOTO(val, out);
+
+ val = fputs(pi->governor_ori, f);
+ FOPS_OR_ERR_GOTO(val, out);
+
+ ret = 0;
+ RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
+ "has been set back to %s successfully\n",
+ pi->lcore_id, pi->governor_ori);
+out:
+ fclose(f);
+
+ return ret;
+}
+
+/**
+ * It is to get the available frequencies of the specific lcore by reading the
+ * sys file.
+ */
+static int
+power_get_available_freqs(struct pstate_power_info *pi)
+{
+ FILE *f_min, *f_max;
+ int ret = -1;
+ char *p_min, *p_max;
+ char buf_min[BUFSIZ];
+ char buf_max[BUFSIZ];
+ char fullpath_min[PATH_MAX];
+ char fullpath_max[PATH_MAX];
+ char *s_min, *s_max;
+ uint32_t sys_min_freq = 0, sys_max_freq = 0, base_max_freq = 0;
+ uint32_t i, num_freqs = 0;
+
+ snprintf(fullpath_max, sizeof(fullpath_max),
+ POWER_SYSFILE_BASE_MAX_FREQ,
+ pi->lcore_id);
+ snprintf(fullpath_min, sizeof(fullpath_min),
+ POWER_SYSFILE_BASE_MIN_FREQ,
+ pi->lcore_id);
+
+ f_min = fopen(fullpath_min, "r");
+ FOPEN_OR_ERR_RET(f_min, ret);
+
+ s_min = fgets(buf_min, sizeof(buf_min), f_min);
+ FOPS_OR_NULL_GOTO(s_min, out);
+
+ f_max = fopen(fullpath_max, "r");
+ FOPEN_OR_ERR_RET(f_max, ret);
+
+ s_max = fgets(buf_max, sizeof(buf_max), f_max);
+ FOPS_OR_NULL_GOTO(s_max, out);
+
+
+ /* Strip the line break if there is */
+ p_min = strchr(buf_min, '\n');
+ if (p_min != NULL)
+ *p_min = 0;
+
+ p_max = strchr(buf_max, '\n');
+ if (p_max != NULL)
+ *p_max = 0;
+
+ sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
+ sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+
+ if (sys_max_freq < sys_min_freq)
+ goto out;
+
+
+ pi->sys_max_freq = sys_max_freq;
+
+ base_max_freq = pi->non_turbo_max_ratio*BUS_FREQ;
+
+ POWER_DEBUG_TRACE("sys min %u, sys max %u, base_max %u\n",
+ sys_min_freq,
+ sys_max_freq,
+ base_max_freq);
+
+ if (base_max_freq < sys_max_freq)
+
+ pi->turbo_available = 1;
+ else
+ pi->turbo_available = 0;
+
+
+ /* If turbo is available then there is one extra freq bucket
+ * to store the sys max freq which value is base_max +1
+ */
+ num_freqs = (base_max_freq - sys_min_freq)/BUS_FREQ + 1
+ + pi->turbo_available;
+
+ /* Generate the freq bucket array.
+ * If turbo is available the freq bucket[0] value is base_max +1
+ * the bucket[1] is base_max, bucket[2] is base_max - BUS_FREQ
+ * and so on.
+ * If turbo is not available bucket[0] is base_max and so on
+ */
+ for (i = 0, pi->nb_freqs = 0; i < num_freqs; i++) {
+
+ if ((i == 0) && pi->turbo_available)
+ pi->freqs[pi->nb_freqs++] = base_max_freq + 1;
+ else
+ pi->freqs[pi->nb_freqs++] =
+ base_max_freq - (i - pi->turbo_available)*BUS_FREQ;
+ }
+
+ ret = 0;
+
+ POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are available\n",
+ num_freqs, pi->lcore_id);
+
+ fclose(f_min);
+ fclose(f_max);
+
+
+out:
+ return ret;
+}
+
+int
+power_pstate_cpufreq_init(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Lcore id %u can not exceeds %u\n",
+ lcore_id, RTE_MAX_LCORE - 1U);
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ if (rte_atomic32_cmpset(&(pi->state), POWER_IDLE, POWER_ONGOING)
+ == 0) {
+ RTE_LOG(INFO, POWER, "Power management of lcore %u is "
+ "in use\n", lcore_id);
+ return -1;
+ }
+
+ pi->lcore_id = lcore_id;
+ /* Check and set the governor */
+ if (power_set_governor_performance(pi) < 0) {
+ RTE_LOG(ERR, POWER, "Cannot set governor of lcore %u to "
+ "performance\n", lcore_id);
+ goto fail;
+ }
+ /* Init for setting lcore frequency */
+ if (power_init_for_setting_freq(pi) < 0) {
+ RTE_LOG(ERR, POWER, "Cannot init for setting frequency for "
+ "lcore %u\n", lcore_id);
+ goto fail;
+ }
+
+ /* Get the available frequencies */
+ if (power_get_available_freqs(pi) < 0) {
+ RTE_LOG(ERR, POWER, "Cannot get available frequencies of "
+ "lcore %u\n", lcore_id);
+ goto fail;
+ }
+
+
+ /* Set freq to max by default */
+ if (power_pstate_cpufreq_freq_max(lcore_id) < 0) {
+ RTE_LOG(ERR, POWER, "Cannot set frequency of lcore %u "
+ "to max\n", lcore_id);
+ goto fail;
+ }
+
+ RTE_LOG(INFO, POWER, "Initialized successfully for lcore %u "
+ "power management\n", lcore_id);
+ rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_USED);
+
+ return 0;
+
+fail:
+ rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
+
+ return -1;
+}
+
+int
+power_pstate_cpufreq_exit(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Lcore id %u can not exceeds %u\n",
+ lcore_id, RTE_MAX_LCORE - 1U);
+ return -1;
+ }
+ pi = &lcore_power_info[lcore_id];
+
+ if (rte_atomic32_cmpset(&(pi->state), POWER_USED, POWER_ONGOING)
+ == 0) {
+ RTE_LOG(INFO, POWER, "Power management of lcore %u is "
+ "not used\n", lcore_id);
+ return -1;
+ }
+
+ /* Close FD of setting freq */
+ fclose(pi->f_cur_min);
+ fclose(pi->f_cur_max);
+ pi->f_cur_min = NULL;
+ pi->f_cur_max = NULL;
+
+ /* Set the governor back to the original */
+ if (power_set_governor_original(pi) < 0) {
+ RTE_LOG(ERR, POWER, "Cannot set the governor of %u back "
+ "to the original\n", lcore_id);
+ goto fail;
+ }
+
+ RTE_LOG(INFO, POWER, "Power management of lcore %u has exited from "
+ "'performance' mode and been set back to the "
+ "original\n", lcore_id);
+ rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_IDLE);
+
+ return 0;
+
+fail:
+ rte_atomic32_cmpset(&(pi->state), POWER_ONGOING, POWER_UNKNOWN);
+
+ return -1;
+}
+
+
+uint32_t
+power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs, uint32_t num)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ if (num < pi->nb_freqs) {
+ RTE_LOG(ERR, POWER, "Buffer size is not enough\n");
+ return 0;
+ }
+ rte_memcpy(freqs, pi->freqs, pi->nb_freqs * sizeof(uint32_t));
+
+ return pi->nb_freqs;
+}
+
+uint32_t
+power_pstate_cpufreq_get_freq(unsigned int lcore_id)
+{
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return RTE_POWER_INVALID_FREQ_INDEX;
+ }
+
+ return lcore_power_info[lcore_id].curr_idx;
+}
+
+
+int
+power_pstate_cpufreq_set_freq(unsigned int lcore_id, uint32_t index)
+{
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ return set_freq_internal(&(lcore_power_info[lcore_id]), index);
+}
+
+int
+power_pstate_cpufreq_freq_up(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ if (pi->curr_idx == 0)
+ return 0;
+
+ /* Frequencies in the array are from high to low. */
+ return set_freq_internal(pi, pi->curr_idx - 1);
+}
+
+int
+power_pstate_cpufreq_freq_down(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ if (pi->curr_idx + 1 == pi->nb_freqs)
+ return 0;
+
+ /* Frequencies in the array are from high to low. */
+ return set_freq_internal(pi, pi->curr_idx + 1);
+}
+
+int
+power_pstate_cpufreq_freq_max(unsigned int lcore_id)
+{
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ /* Frequencies in the array are from high to low. */
+ if (lcore_power_info[lcore_id].turbo_available) {
+ if (lcore_power_info[lcore_id].turbo_enable)
+ /* Set to Turbo */
+ return set_freq_internal(
+ &lcore_power_info[lcore_id], 0);
+ else
+ /* Set to max non-turbo */
+ return set_freq_internal(
+ &lcore_power_info[lcore_id], 1);
+ } else
+ return set_freq_internal(&lcore_power_info[lcore_id], 0);
+}
+
+
+int
+power_pstate_cpufreq_freq_min(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+
+ /* Frequencies in the array are from high to low. */
+ return set_freq_internal(pi, pi->nb_freqs - 1);
+}
+
+
+int
+power_pstate_turbo_status(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+
+ return pi->turbo_enable;
+}
+
+int
+power_pstate_enable_turbo(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+
+ if (pi->turbo_available)
+ pi->turbo_enable = 1;
+ else {
+ pi->turbo_enable = 0;
+ RTE_LOG(ERR, POWER,
+ "Failed to enable turbo on lcore %u\n",
+ lcore_id);
+ return -1;
+ }
+
+ return 0;
+}
+
+
+int
+power_pstate_disable_turbo(unsigned int lcore_id)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+
+ pi->turbo_enable = 0;
+
+
+ return 0;
+}
+
+
+int power_pstate_get_capabilities(unsigned int lcore_id,
+ struct rte_power_core_capabilities *caps)
+{
+ struct pstate_power_info *pi;
+
+ if (lcore_id >= RTE_MAX_LCORE) {
+ RTE_LOG(ERR, POWER, "Invalid lcore ID\n");
+ return -1;
+ }
+ if (caps == NULL) {
+ RTE_LOG(ERR, POWER, "Invalid argument\n");
+ return -1;
+ }
+
+ pi = &lcore_power_info[lcore_id];
+ caps->capabilities = 0;
+ caps->turbo = !!(pi->turbo_available);
+
+ return 0;
+}
new file mode 100644
@@ -0,0 +1,218 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2010-2018 Intel Corporation
+ */
+
+#ifndef _POWER_PSTATE_CPUFREQ_H
+#define _POWER_PSTATE_CPUFREQ_H
+
+/**
+ * @file
+ * RTE Power Management via Intel Pstate driver
+ */
+
+#include <rte_common.h>
+#include <rte_byteorder.h>
+#include <rte_log.h>
+#include <rte_string_fns.h>
+#include "rte_power.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * Initialize power management for a specific lcore. It will check and set the
+ * governor to performance for the lcore, get the available frequencies, and
+ * prepare to set new lcore frequency.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 0 on success.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_init(unsigned int lcore_id);
+
+/**
+ * Exit power management on a specific lcore. It will set the governor to which
+ * is before initialized.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 0 on success.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_exit(unsigned int lcore_id);
+
+/**
+ * Get the available frequencies of a specific lcore. The return value will be
+ * the minimal one of the total number of available frequencies and the number
+ * of buffer. The index of available frequencies used in other interfaces
+ * should be in the range of 0 to this return value.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ * @param freqs
+ * The buffer array to save the frequencies.
+ * @param num
+ * The number of frequencies to get.
+ *
+ * @return
+ * The number of available frequencies.
+ */
+uint32_t power_pstate_cpufreq_freqs(unsigned int lcore_id, uint32_t *freqs,
+ uint32_t num);
+
+/**
+ * Return the current index of available frequencies of a specific lcore. It
+ * will return 'RTE_POWER_INVALID_FREQ_INDEX = (~0)' if error.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * The current index of available frequencies.
+ */
+uint32_t power_pstate_cpufreq_get_freq(unsigned int lcore_id);
+
+/**
+ * Set the new frequency for a specific lcore by indicating the index of
+ * available frequencies.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ * @param index
+ * The index of available frequencies.
+ *
+ * @return
+ * - 1 on success with frequency changed.
+ * - 0 on success without frequency changed.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_set_freq(unsigned int lcore_id, uint32_t index);
+
+/**
+ * Scale up the frequency of a specific lcore according to the available
+ * frequencies.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 1 on success with frequency changed.
+ * - 0 on success without frequency changed.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_freq_up(unsigned int lcore_id);
+
+/**
+ * Scale down the frequency of a specific lcore according to the available
+ * frequencies.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 1 on success with frequency changed.
+ * - 0 on success without frequency changed.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_freq_down(unsigned int lcore_id);
+
+/**
+ * Scale up the frequency of a specific lcore to the highest according to the
+ * available frequencies.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 1 on success with frequency changed.
+ * - 0 on success without frequency changed.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_freq_max(unsigned int lcore_id);
+
+/**
+ * Scale down the frequency of a specific lcore to the lowest according to the
+ * available frequencies.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 1 on success with frequency changed.
+ * - 0 on success without frequency changed.
+ * - Negative on error.
+ */
+int power_pstate_cpufreq_freq_min(unsigned int lcore_id);
+
+/**
+ * Get the turbo status of a specific lcore.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 1 Turbo Boost is enabled on this lcore.
+ * - 0 Turbo Boost is disabled on this lcore.
+ * - Negative on error.
+ */
+int power_pstate_turbo_status(unsigned int lcore_id);
+
+/**
+ * Enable Turbo Boost on a specific lcore.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 0 Turbo Boost is enabled successfully on this lcore.
+ * - Negative on error.
+ */
+int power_pstate_enable_turbo(unsigned int lcore_id);
+
+/**
+ * Disable Turbo Boost on a specific lcore.
+ * It should be protected outside of this function for threadsafe.
+ *
+ * @param lcore_id
+ * lcore id.
+ *
+ * @return
+ * - 0 Turbo Boost disabled successfully on this lcore.
+ * - Negative on error.
+ */
+int power_pstate_disable_turbo(unsigned int lcore_id);
+
+/**
+ * Returns power capabilities for a specific lcore.
+ *
+ * @param lcore_id
+ * lcore id.
+ * @param caps
+ * pointer to rte_power_core_capabilities object.
+ *
+ * @return
+ * - 0 on success.
+ * - Negative on error.
+ */
+int power_pstate_get_capabilities(unsigned int lcore_id,
+ struct rte_power_core_capabilities *caps);
+#ifdef __cplusplus
+}
+#endif
+
+#endif
@@ -7,6 +7,7 @@
#include "rte_power.h"
#include "power_acpi_cpufreq.h"
#include "power_kvm_vm.h"
+#include "power_pstate_cpufreq.h"
#include "power_common.h"
enum power_management_env global_default_env = PM_ENV_NOT_SET;
@@ -29,6 +30,8 @@ rte_power_get_capabilities_t rte_power_get_capabilities;
int
rte_power_set_env(enum power_management_env env)
{
+
+
if (rte_atomic32_cmpset(&global_env_cfg_status, 0, 1) == 0) {
return 0;
}
@@ -56,6 +59,19 @@ rte_power_set_env(enum power_management_env env)
rte_power_freq_enable_turbo = power_kvm_vm_enable_turbo;
rte_power_freq_disable_turbo = power_kvm_vm_disable_turbo;
rte_power_get_capabilities = power_kvm_vm_get_capabilities;
+ } else if (env == PM_ENV_PSTATE_CPUFREQ) {
+ rte_power_freqs = power_pstate_cpufreq_freqs;
+ rte_power_get_freq = power_pstate_cpufreq_get_freq;
+ rte_power_set_freq = power_pstate_cpufreq_set_freq;
+ rte_power_freq_up = power_pstate_cpufreq_freq_up;
+ rte_power_freq_down = power_pstate_cpufreq_freq_down;
+ rte_power_freq_min = power_pstate_cpufreq_freq_min;
+ rte_power_freq_max = power_pstate_cpufreq_freq_max;
+ rte_power_turbo_status = power_pstate_turbo_status;
+ rte_power_freq_enable_turbo = power_pstate_enable_turbo;
+ rte_power_freq_disable_turbo = power_pstate_disable_turbo;
+ rte_power_get_capabilities = power_pstate_get_capabilities;
+
} else {
RTE_LOG(ERR, POWER, "Invalid Power Management Environment(%d) set\n",
env);
@@ -84,12 +100,15 @@ rte_power_init(unsigned int lcore_id)
{
int ret = -1;
- if (global_default_env == PM_ENV_ACPI_CPUFREQ) {
+ if (global_default_env == PM_ENV_ACPI_CPUFREQ)
return power_acpi_cpufreq_init(lcore_id);
- }
- if (global_default_env == PM_ENV_KVM_VM) {
+
+ if (global_default_env == PM_ENV_KVM_VM)
return power_kvm_vm_init(lcore_id);
- }
+
+ if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
+ return power_pstate_cpufreq_init(lcore_id);
+
/* Auto detect Environment */
RTE_LOG(INFO, POWER, "Attempting to initialise ACPI cpufreq power "
"management...\n");
@@ -99,13 +118,23 @@ rte_power_init(unsigned int lcore_id)
goto out;
}
- RTE_LOG(INFO, POWER, "Attempting to initialise VM power management...\n");
+ RTE_LOG(INFO, POWER, "Attempting to initialise PSTAT power "
+ "management...\n");
+ ret = power_pstate_cpufreq_init(lcore_id);
+ if (ret == 0) {
+ rte_power_set_env(PM_ENV_PSTATE_CPUFREQ);
+ goto out;
+ }
+
+ RTE_LOG(INFO, POWER, "Attempting to initialise VM power "
+ "management...\n");
ret = power_kvm_vm_init(lcore_id);
if (ret == 0) {
rte_power_set_env(PM_ENV_KVM_VM);
goto out;
}
- RTE_LOG(ERR, POWER, "Unable to set Power Management Environment for lcore "
+ RTE_LOG(ERR, POWER, "Unable to set Power Management "
+ "Environment for lcore "
"%u\n", lcore_id);
out:
return ret;
@@ -118,6 +147,8 @@ rte_power_exit(unsigned int lcore_id)
return power_acpi_cpufreq_exit(lcore_id);
if (global_default_env == PM_ENV_KVM_VM)
return power_kvm_vm_exit(lcore_id);
+ if (global_default_env == PM_ENV_PSTATE_CPUFREQ)
+ return power_pstate_cpufreq_exit(lcore_id);
RTE_LOG(ERR, POWER, "Environment has not been set, unable to exit "
"gracefully\n");
@@ -20,7 +20,8 @@ extern "C" {
#endif
/* Power Management Environment State */
-enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM};
+enum power_management_env {PM_ENV_NOT_SET, PM_ENV_ACPI_CPUFREQ, PM_ENV_KVM_VM,
+ PM_ENV_PSTATE_CPUFREQ};
/**
* Set the default power management implementation. If this is not called prior