[v6,2/2] power: refactor pstate and acpi code
Checks
Commit Message
From: Anatoly Burakov <anatoly.burakov@intel.com>
Currently, ACPI and PSTATE modes have lots of code duplication,
confusing logic, and a bunch of other issues that can, and have, led to
various bugs and resource leaks.
This commit factors out the common parts of sysfs reading/writing for
ACPI and PSTATE drivers.
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Signed-off-by: David Hunt <david.hunt@intel.com>
---
changes in v5
* fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
* removed FOPS* and FOPEN* macros, which contained control statements.
* fixed some checkpatch warnings.
changes in v6
* fixed check of fputs return, negative on error.
---
lib/power/meson.build | 7 +
lib/power/power_acpi_cpufreq.c | 192 ++++------------
lib/power/power_common.c | 146 ++++++++++++
lib/power/power_common.h | 17 ++
lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
5 files changed, 335 insertions(+), 401 deletions(-)
Comments
On 23-Jun-21 1:03 PM, David Hunt wrote:
> From: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Currently, ACPI and PSTATE modes have lots of code duplication,
> confusing logic, and a bunch of other issues that can, and have, led to
> various bugs and resource leaks.
>
> This commit factors out the common parts of sysfs reading/writing for
> ACPI and PSTATE drivers.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ---
> changes in v5
> * fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
> * removed FOPS* and FOPEN* macros, which contained control statements.
> * fixed some checkpatch warnings.
> changes in v6
> * fixed check of fputs return, negative on error.
> ---
> lib/power/meson.build | 7 +
> lib/power/power_acpi_cpufreq.c | 192 ++++------------
> lib/power/power_common.c | 146 ++++++++++++
> lib/power/power_common.h | 17 ++
> lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
> 5 files changed, 335 insertions(+), 401 deletions(-)
>
> diff --git a/lib/power/meson.build b/lib/power/meson.build
> index c1097d32f1..74c5f3a294 100644
> --- a/lib/power/meson.build
> +++ b/lib/power/meson.build
> @@ -5,6 +5,13 @@ if not is_linux
> build = false
> reason = 'only supported on Linux'
> endif
> +
> +# we do some snprintf magic so silence format-nonliteral
> +flag_nonliteral = '-Wno-format-nonliteral'
> +if cc.has_argument(flag_nonliteral)
> + cflags += flag_nonliteral
> +endif
> +
I'll do a more thorough review later, but i suspect that we can drop
this, if we fix the functions to have a GCC printf format attribute:
https://gcc.gnu.org/onlinedocs/gcc-3.2/gcc/Function-Attributes.html
See "format" section. I'm not sure this is supported by other compilers
though, so maybe it's better to keep it.
Hi,
It looks good to me. I verified on arm platforms and didn't find any problem currently.
BR,
Richael
> -----Original Message-----
> From: David Hunt <david.hunt@intel.com>
> Sent: Wednesday, June 23, 2021 8:04 PM
> To: dev@dpdk.org
> Cc: david.hunt@intel.com; anatoly.burakov@intel.com; Richael Zhuang
> <Richael.Zhuang@arm.com>; stephen@networkplumber.org;
> reshma.pattan@intel.com; nd <nd@arm.com>
> Subject: [PATCH v6 2/2] power: refactor pstate and acpi code
>
> From: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Currently, ACPI and PSTATE modes have lots of code duplication, confusing
> logic, and a bunch of other issues that can, and have, led to various bugs and
> resource leaks.
>
> This commit factors out the common parts of sysfs reading/writing for ACPI
> and PSTATE drivers.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ---
> changes in v5
> * fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
> * removed FOPS* and FOPEN* macros, which contained control statements.
> * fixed some checkpatch warnings.
> changes in v6
> * fixed check of fputs return, negative on error.
> ---
> lib/power/meson.build | 7 +
> lib/power/power_acpi_cpufreq.c | 192 ++++------------
> lib/power/power_common.c | 146 ++++++++++++
> lib/power/power_common.h | 17 ++
> lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
> 5 files changed, 335 insertions(+), 401 deletions(-)
>
> diff --git a/lib/power/meson.build b/lib/power/meson.build index
> c1097d32f1..74c5f3a294 100644
> --- a/lib/power/meson.build
> +++ b/lib/power/meson.build
> @@ -5,6 +5,13 @@ if not is_linux
> build = false
> reason = 'only supported on Linux'
> endif
> +
> +# we do some snprintf magic so silence format-nonliteral
> +flag_nonliteral = '-Wno-format-nonliteral'
> +if cc.has_argument(flag_nonliteral)
> + cflags += flag_nonliteral
> +endif
> +
> sources = files(
> 'guest_channel.c',
> 'power_acpi_cpufreq.c',
> diff --git a/lib/power/power_acpi_cpufreq.c
> b/lib/power/power_acpi_cpufreq.c index 1b8c69cc8b..9ca8d8a8f2 100644
> --- a/lib/power/power_acpi_cpufreq.c
> +++ b/lib/power/power_acpi_cpufreq.c
> @@ -19,41 +19,10 @@
> #include "power_acpi_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 opened\n"); \
> - return retval; \
> - } \
> -} while (0)
> -
> -#define FOPS_OR_NULL_GOTO(ret, label) do { \
> - if ((ret) == NULL) { \
> - RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
> - goto label; \
> - } \
> -} while (0)
> -
> -#define FOPS_OR_ERR_GOTO(ret, label) do { \
> - if ((ret) < 0) { \
> - RTE_LOG(ERR, POWER, "File operations failed\n"); \
> - goto label; \
> - } \
> -} while (0)
> -
> #define STR_SIZE 1024
> #define POWER_CONVERT_TO_DECIMAL 10
>
> #define POWER_GOVERNOR_USERSPACE "userspace"
> -#define POWER_SYSFILE_GOVERNOR \
> - "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
> #define POWER_SYSFILE_AVAIL_FREQ \
>
> "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequen
> cies"
> #define POWER_SYSFILE_SETSPEED \
> @@ -135,53 +104,18 @@ set_freq_internal(struct acpi_power_info *pi,
> uint32_t idx) static int power_set_governor_userspace(struct
> acpi_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);
> - /* Strip off terminating '\n' */
> - strtok(buf, "\n");
> -
> - /* Save the original governor */
> - rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
> -
> - /* Check if current governor is userspace */
> - if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
> - sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
> - ret = 0;
> - POWER_DEBUG_TRACE("Power management governor of
> lcore %u is "
> - "already userspace\n", pi->lcore_id);
> - goto out;
> - }
> -
> - /* Write 'userspace' to the governor */
> - val = fseek(f, 0, SEEK_SET);
> - FOPS_OR_ERR_GOTO(val, out);
> -
> - val = fputs(POWER_GOVERNOR_USERSPACE, f);
> - FOPS_OR_ERR_GOTO(val, out);
> -
> - /* We need to flush to see if the fputs succeeds */
> - val = fflush(f);
> - FOPS_OR_ERR_GOTO(val, out);
> -
> - ret = 0;
> - RTE_LOG(INFO, POWER, "Power management governor of lcore %u
> has been "
> - "set to user space successfully\n", pi->lcore_id);
> -out:
> - fclose(f);
> + return power_set_governor(pi->lcore_id,
> POWER_GOVERNOR_USERSPACE,
> + pi->governor_ori, sizeof(pi->governor_ori)); }
>
> - 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 acpi_power_info *pi) {
> + return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
> }
>
> /**
> @@ -195,22 +129,22 @@ power_get_available_freqs(struct
> acpi_power_info *pi)
> int ret = -1, i, count;
> char *p;
> char buf[BUFSIZ];
> - char fullpath[PATH_MAX];
> char *freqs[RTE_MAX_LCORE_FREQS];
> - char *s;
> -
> - snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
> - pi->lcore_id);
> - f = fopen(fullpath, "r");
> - FOPEN_OR_ERR_RET(f, ret);
>
> - s = fgets(buf, sizeof(buf), f);
> - FOPS_OR_NULL_GOTO(s, out);
> + open_core_sysfs_file(POWER_SYSFILE_AVAIL_FREQ, pi->lcore_id, "r",
> + &f);
> + if (f == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_AVAIL_FREQ);
> + goto out;
> + }
>
> - /* Strip the line break if there is */
> - p = strchr(buf, '\n');
> - if (p != NULL)
> - *p = 0;
> + ret = read_core_sysfs_s(f, buf, sizeof(buf));
> + if ((ret) < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_AVAIL_FREQ);
> + goto out;
> + }
>
> /* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
> count = rte_strsplit(buf, sizeof(buf), freqs, @@ -250,7 +184,8 @@
> power_get_available_freqs(struct acpi_power_info *pi)
> POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are
> available\n",
> count, pi->lcore_id);
> out:
> - fclose(f);
> + if (f != NULL)
> + fclose(f);
>
> return ret;
> }
> @@ -262,18 +197,24 @@ static int
> power_init_for_setting_freq(struct acpi_power_info *pi) {
> FILE *f;
> - char fullpath[PATH_MAX];
> char buf[BUFSIZ];
> uint32_t i, freq;
> - char *s;
> + int ret;
>
> - snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
> - pi->lcore_id);
> - f = fopen(fullpath, "rw+");
> - FOPEN_OR_ERR_RET(f, -1);
> + open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "rw+",
> + &f);
> + if (f == NULL) {
> + RTE_LOG(ERR, POWER, "Failed to open %s\n",
> + POWER_SYSFILE_SETSPEED);
> + goto err;
> + }
>
> - s = fgets(buf, sizeof(buf), f);
> - FOPS_OR_NULL_GOTO(s, out);
> + ret = read_core_sysfs_s(f, buf, sizeof(buf));
> + if ((ret) < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_SETSPEED);
> + goto err;
> + }
>
> freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
> for (i = 0; i < pi->nb_freqs; i++) {
> @@ -284,8 +225,9 @@ power_init_for_setting_freq(struct acpi_power_info
> *pi)
> }
> }
>
> -out:
> - fclose(f);
> +err:
> + if (f != NULL)
> + fclose(f);
>
> return -1;
> }
> @@ -369,54 +311,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
> return -1;
> }
>
> -/**
> - * 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 acpi_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;
> -}
> -
> int
> power_acpi_cpufreq_exit(unsigned int lcore_id) { diff --git
> a/lib/power/power_common.c b/lib/power/power_common.c index
> 67e3318ec7..4deb343dae 100644
> --- a/lib/power/power_common.c
> +++ b/lib/power/power_common.c
> @@ -3,13 +3,20 @@
> */
>
> #include <limits.h>
> +#include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
>
> +#include <rte_log.h>
> +#include <rte_string_fns.h>
> +
> #include "power_common.h"
>
> #define POWER_SYSFILE_SCALING_DRIVER \
> "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
> +#define POWER_SYSFILE_GOVERNOR \
> + "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
> +#define POWER_CONVERT_TO_DECIMAL 10
>
> int
> cpufreq_check_scaling_driver(const char *driver_name) @@ -58,3 +65,142
> @@ cpufreq_check_scaling_driver(const char *driver_name)
> */
> return 1;
> }
> +
> +int
> +open_core_sysfs_file(const char *template, unsigned int core, const char
> *mode,
> + FILE **f)
> +{
> + char fullpath[PATH_MAX];
> + FILE *tmpf;
> +
> + /* silenced -Wformat-nonliteral here */
> + snprintf(fullpath, sizeof(fullpath), template, core);
> + tmpf = fopen(fullpath, mode);
> + *f = tmpf;
> + if (tmpf == NULL)
> + return -1;
> +
> + return 0;
> +}
> +
> +int
> +read_core_sysfs_u32(FILE *f, uint32_t *val) {
> + char buf[BUFSIZ];
> + uint32_t fval;
> + char *s;
> +
> + s = fgets(buf, sizeof(buf), f);
> + if (s == NULL)
> + return -1;
> +
> + /* fgets puts null terminator in, but do this just in case */
> + buf[BUFSIZ - 1] = '\0';
> +
> + /* strip off any terminating newlines */
> + *strchrnul(buf, '\n') = '\0';
> +
> + fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
> +
> + /* write the value */
> + *val = fval;
> +
> + return 0;
> +}
> +
> +int
> +read_core_sysfs_s(FILE *f, char *buf, unsigned int len) {
> + char *s;
> +
> + s = fgets(buf, len, f);
> + if (s == NULL)
> + return -1;
> +
> + /* fgets puts null terminator in, but do this just in case */
> + buf[len - 1] = '\0';
> +
> + /* strip off any terminating newlines */
> + *strchrnul(buf, '\n') = '\0';
> +
> + return 0;
> +}
> +
> +int
> +write_core_sysfs_s(FILE *f, const char *str) {
> + int ret;
> +
> + ret = fseek(f, 0, SEEK_SET);
> + if (ret != 0)
> + return -1;
> +
> + ret = fputs(str, f);
> + if (ret < 0)
> + return -1;
> +
> + /* flush the output */
> + ret = fflush(f);
> + if (ret != 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/**
> + * 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.
> + */
> +int
> +power_set_governor(unsigned int lcore_id, const char *new_governor,
> + char *orig_governor, size_t orig_governor_len) {
> + FILE *f_governor = NULL;
> + int ret = -1;
> + char buf[BUFSIZ];
> +
> + open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, lcore_id, "rw+",
> + &f_governor);
> + if (f_governor == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_GOVERNOR);
> + goto out;
> + }
> +
> + ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_GOVERNOR);
> + goto out;
> + }
> +
> + /* Save the original governor, if it was provided */
> + if (orig_governor)
> + rte_strscpy(orig_governor, buf, orig_governor_len);
> +
> + /* Check if current governor is already what we want */
> + if (strcmp(buf, new_governor) == 0) {
> + ret = 0;
> + POWER_DEBUG_TRACE("Power management governor of
> lcore %u is "
> + "already %s\n", lcore_id, new_governor);
> + goto out;
> + }
> +
> + /* Write the new governor */
> + ret = write_core_sysfs_s(f_governor, new_governor);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to write %s\n",
> + POWER_SYSFILE_GOVERNOR);
> + goto out;
> + }
> +
> + ret = 0;
> + RTE_LOG(INFO, POWER, "Power management governor of lcore %u
> has been "
> + "set to '%s' successfully\n", lcore_id, new_governor);
> +out:
> + if (f_governor != NULL)
> + fclose(f_governor);
> +
> + return ret;
> +}
> diff --git a/lib/power/power_common.h b/lib/power/power_common.h
> index fab3ca995a..ef9f5fa855 100644
> --- a/lib/power/power_common.h
> +++ b/lib/power/power_common.h
> @@ -5,9 +5,26 @@
> #ifndef _POWER_COMMON_H_
> #define _POWER_COMMON_H_
>
> +#include <inttypes.h>
> +
> #define RTE_POWER_INVALID_FREQ_INDEX (~0)
>
> +
> +#ifdef RTE_LIBRTE_POWER_DEBUG
> +#define POWER_DEBUG_TRACE(fmt, args...) \
> + RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args) #else
> #define
> +POWER_DEBUG_TRACE(fmt, args...) #endif
> +
> /* check if scaling driver matches one we want */ int
> cpufreq_check_scaling_driver(const char *driver);
> +int power_set_governor(unsigned int lcore_id, const char *new_governor,
> + char *orig_governor, size_t orig_governor_len); int
> +open_core_sysfs_file(const char *template, unsigned int core,
> + const char *mode, FILE **f);
> +int read_core_sysfs_u32(FILE *f, uint32_t *val); int
> +read_core_sysfs_s(FILE *f, char *buf, unsigned int len); int
> +write_core_sysfs_s(FILE *f, const char *str);
>
> #endif /* _POWER_COMMON_H_ */
> diff --git a/lib/power/power_pstate_cpufreq.c
> b/lib/power/power_pstate_cpufreq.c
> index 2cfc54acf3..ed10bd193b 100644
> --- a/lib/power/power_pstate_cpufreq.c
> +++ b/lib/power/power_pstate_cpufreq.c
> @@ -21,46 +21,13 @@
> #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 opened\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)
> -
> /* macros used for rounding frequency to nearest 100000 */ #define
> FREQ_ROUNDING_DELTA 50000 #define ROUND_FREQ_TO_N_100000
> 100000
>
> -#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 \
> @@ -154,91 +121,78 @@ out: close(fd);
> static int
> power_init_for_setting_freq(struct pstate_power_info *pi) {
> - FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
> - char fullpath_min[PATH_MAX];
> - char fullpath_max[PATH_MAX];
> - char fullpath_base[PATH_MAX];
> - char fullpath_base_max[PATH_MAX];
> - char buf_base[BUFSIZ];
> - char *s_base;
> - char *s_base_max;
> - uint32_t base_ratio = 0;
> - uint32_t base_max_ratio = 0;
> - uint64_t max_non_turbo = 0;
> - int ret_val = 0;
> -
> - snprintf(fullpath_base_max,
> - sizeof(fullpath_base_max),
> - POWER_SYSFILE_BASE_MAX_FREQ,
> - pi->lcore_id);
> - f_base_max = fopen(fullpath_base_max, "r");
> - FOPEN_OR_ERR_RET(f_base_max, -1);
> - if (f_base_max != NULL) {
> - s_base_max = fgets(buf_base, sizeof(buf_base),
> f_base_max);
> -
> - /* close the file unconditionally */
> - fclose(f_base_max);
> - f_base_max = NULL;
> -
> - FOPS_OR_NULL_GOTO(s_base_max, out);
> -
> - buf_base[BUFSIZ-1] = '\0';
> - if (strlen(buf_base))
> - /* Strip off terminating '\n' */
> - strtok(buf_base, "\n");
> -
> - base_max_ratio =
> - strtoul(buf_base, NULL,
> POWER_CONVERT_TO_DECIMAL)
> - / BUS_FREQ;
> + FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max =
> NULL;
> + uint32_t base_ratio, base_max_ratio;
> + uint64_t max_non_turbo;
> + int ret;
> +
> + /* open all files we expect to have open */
> + open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi-
> >lcore_id, "r",
> + &f_base_max);
> + if (f_base_max == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_BASE_MAX_FREQ);
> + goto err;
> + }
> +
> + open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id,
> "rw+",
> + &f_min);
> + if (f_min == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_MIN_FREQ);
> + goto err;
> + }
> +
> + open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id,
> "rw+",
> + &f_max);
> + if (f_max == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_MAX_FREQ);
> + goto err;
> + }
> +
> + open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
> + &f_base);
> + /* base ratio file may not exist in some kernels, so no error check */
> +
> + /* read base max ratio */
> + ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_BASE_MAX_FREQ);
> + goto err;
> + }
> +
> + /* base ratio may not exist */
> + if (f_base != NULL) {
> + ret = read_core_sysfs_u32(f_base, &base_ratio);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_BASE_FREQ);
> + goto err;
> + }
> + } else {
> + base_ratio = 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);
> + /* Add MSR read to detect turbo status */
> + if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) <
> 0)
> + goto err;
> + /* no errors after this point */
>
> - snprintf(fullpath_max, sizeof(fullpath_max),
> POWER_SYSFILE_MAX_FREQ,
> - pi->lcore_id);
> - f_max = fopen(fullpath_max, "rw+");
> - if (f_max == NULL)
> - fclose(f_min);
> - FOPEN_OR_ERR_RET(f_max, -1);
> + /* convert ratios to bins */
> + base_max_ratio /= BUS_FREQ;
> + base_ratio /= BUS_FREQ;
>
> + /* assign file handles */
> pi->f_cur_min = f_min;
> pi->f_cur_max = f_max;
>
> - snprintf(fullpath_base, sizeof(fullpath_base),
> POWER_SYSFILE_BASE_FREQ,
> - pi->lcore_id);
> -
> - f_base = fopen(fullpath_base, "r");
> - if (f_base == NULL) {
> - /* No sysfs base_frequency, that's OK, continue without */
> - base_ratio = 0;
> - } else {
> - s_base = fgets(buf_base, sizeof(buf_base), f_base);
> - FOPS_OR_NULL_GOTO(s_base, out);
> -
> - buf_base[BUFSIZ-1] = '\0';
> - if (strlen(buf_base))
> - /* Strip off terminating '\n' */
> - strtok(buf_base, "\n");
> -
> - base_ratio = strtoul(buf_base, NULL,
> POWER_CONVERT_TO_DECIMAL)
> - / BUS_FREQ;
> - }
> -
> - /* Add MSR read to detect turbo status */
> -
> - if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) <
> 0) {
> - ret_val = -1;
> - goto out;
> - }
> -
> max_non_turbo =
> (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
>
> POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n",
> max_non_turbo);
>
> - pi->non_turbo_max_ratio = max_non_turbo;
> + pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
>
> /*
> * If base_frequency is reported as greater than the maximum @@ -
> 264,7 +218,20 @@ power_init_for_setting_freq(struct pstate_power_info
> *pi)
> out:
> if (f_base != NULL)
> fclose(f_base);
> - return ret_val;
> + fclose(f_base_max);
> + /* f_min and f_max are stored, no need to close */
> + return 0;
> +
> +err:
> + if (f_base != NULL)
> + fclose(f_base);
> + if (f_base_max != NULL)
> + fclose(f_base_max);
> + if (f_min != NULL)
> + fclose(f_min);
> + if (f_max != NULL)
> + fclose(f_max);
> + return -1;
> }
>
> static int
> @@ -369,53 +336,8 @@ set_freq_internal(struct pstate_power_info *pi,
> uint32_t idx) 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);
> - /* Strip off terminating '\n' */
> - strtok(buf, "\n");
> -
> - /* Save the original governor */
> - rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
> -
> - /* 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;
> - }
> -
> - /* 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);
> -
> - /* We need to flush to see if the fputs succeeds */
> - val = fflush(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;
> + return power_set_governor(pi->lcore_id, POWER_GOVERNOR_PERF,
> + pi->governor_ori, sizeof(pi->governor_ori));
> }
>
> /**
> @@ -425,45 +347,7 @@ power_set_governor_performance(struct
> pstate_power_info *pi) 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;
> + return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
> }
>
> /**
> @@ -473,51 +357,42 @@ power_set_governor_original(struct
> pstate_power_info *pi) static int power_get_available_freqs(struct
> pstate_power_info *pi) {
> - FILE *f_min, *f_max;
> + FILE *f_min = NULL, *f_max = NULL;
> 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);
> -
> - f_max = fopen(fullpath_max, "r");
> - if (f_max == NULL)
> - fclose(f_min);
> -
> - FOPEN_OR_ERR_RET(f_max, ret);
> -
> - s_min = fgets(buf_min, sizeof(buf_min), f_min);
> - FOPS_OR_NULL_GOTO(s_min, out);
> -
> - s_max = fgets(buf_max, sizeof(buf_max), f_max);
> - FOPS_OR_NULL_GOTO(s_max, out);
> -
> + /* open all files */
> + open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi-
> >lcore_id, "r",
> + &f_max);
> + if (f_max == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_BASE_MAX_FREQ);
> + goto out;
> + }
>
> - /* Strip the line break if there is */
> - p_min = strchr(buf_min, '\n');
> - if (p_min != NULL)
> - *p_min = 0;
> + open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id,
> "r",
> + &f_min);
> + if (f_min == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_BASE_MIN_FREQ);
> + goto out;
> + }
>
> - p_max = strchr(buf_max, '\n');
> - if (p_max != NULL)
> - *p_max = 0;
> + /* read base ratios */
> + ret = read_core_sysfs_u32(f_max, &sys_max_freq);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_BASE_MAX_FREQ);
> + goto out;
> + }
>
> - sys_min_freq = strtoul(buf_min, &p_min,
> POWER_CONVERT_TO_DECIMAL);
> - sys_max_freq = strtoul(buf_max, &p_max,
> POWER_CONVERT_TO_DECIMAL);
> + ret = read_core_sysfs_u32(f_min, &sys_min_freq);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_BASE_MIN_FREQ);
> + goto out;
> + }
>
> if (sys_max_freq < sys_min_freq)
> goto out;
> @@ -576,27 +451,22 @@ power_get_cur_idx(struct pstate_power_info *pi)
> {
> FILE *f_cur;
> int ret = -1;
> - char *p_cur;
> - char buf_cur[BUFSIZ];
> - char fullpath_cur[PATH_MAX];
> - char *s_cur;
> uint32_t sys_cur_freq = 0;
> unsigned int i;
>
> - snprintf(fullpath_cur, sizeof(fullpath_cur),
> - POWER_SYSFILE_CUR_FREQ,
> - pi->lcore_id);
> - f_cur = fopen(fullpath_cur, "r");
> - FOPEN_OR_ERR_RET(f_cur, ret);
> -
> - /* initialize the cur_idx to matching current frequency freq index */
> - s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
> - FOPS_OR_NULL_GOTO(s_cur, fail);
> + open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r",
> &f_cur);
> + if (f_cur == NULL) {
> + RTE_LOG(ERR, POWER, "failed to open %s\n",
> + POWER_SYSFILE_CUR_FREQ);
> + goto fail;
> + }
>
> - p_cur = strchr(buf_cur, '\n');
> - if (p_cur != NULL)
> - *p_cur = 0;
> - sys_cur_freq = strtoul(buf_cur, &p_cur,
> POWER_CONVERT_TO_DECIMAL);
> + ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
> + if (ret < 0) {
> + RTE_LOG(ERR, POWER, "Failed to read %s\n",
> + POWER_SYSFILE_CUR_FREQ);
> + goto fail;
> + }
>
> /* convert the frequency to nearest 100000 value
> * Ex: if sys_cur_freq=1396789 then freq_conv=1400000 @@ -615,10
> +485,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
> }
> }
>
> - fclose(f_cur);
> - return 0;
> + ret = 0;
> fail:
> - fclose(f_cur);
> + if (f_cur != NULL)
> + fclose(f_cur);
> return ret;
> }
>
> --
> 2.17.1
On Wed, Jun 23, 2021 at 2:04 PM David Hunt <david.hunt@intel.com> wrote:
>
> From: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Currently, ACPI and PSTATE modes have lots of code duplication,
> confusing logic, and a bunch of other issues that can, and have, led to
> various bugs and resource leaks.
>
> This commit factors out the common parts of sysfs reading/writing for
> ACPI and PSTATE drivers.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> Signed-off-by: David Hunt <david.hunt@intel.com>
>
> ---
> changes in v5
> * fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
> * removed FOPS* and FOPEN* macros, which contained control statements.
> * fixed some checkpatch warnings.
> changes in v6
> * fixed check of fputs return, negative on error.
> ---
> lib/power/meson.build | 7 +
> lib/power/power_acpi_cpufreq.c | 192 ++++------------
> lib/power/power_common.c | 146 ++++++++++++
> lib/power/power_common.h | 17 ++
> lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
> 5 files changed, 335 insertions(+), 401 deletions(-)
>
> diff --git a/lib/power/meson.build b/lib/power/meson.build
> index c1097d32f1..74c5f3a294 100644
> --- a/lib/power/meson.build
> +++ b/lib/power/meson.build
> @@ -5,6 +5,13 @@ if not is_linux
> build = false
> reason = 'only supported on Linux'
> endif
> +
> +# we do some snprintf magic so silence format-nonliteral
> +flag_nonliteral = '-Wno-format-nonliteral'
> +if cc.has_argument(flag_nonliteral)
> + cflags += flag_nonliteral
> +endif
> +
This can be removed with __rte_format_printf tag + API change below.
> sources = files(
> 'guest_channel.c',
> 'power_acpi_cpufreq.c',
[snip]
> diff --git a/lib/power/power_common.c b/lib/power/power_common.c
> index 67e3318ec7..4deb343dae 100644
> --- a/lib/power/power_common.c
> +++ b/lib/power/power_common.c
> @@ -3,13 +3,20 @@
> */
>
> #include <limits.h>
> +#include <stdlib.h>
> #include <stdio.h>
> #include <string.h>
>
> +#include <rte_log.h>
> +#include <rte_string_fns.h>
> +
> #include "power_common.h"
>
> #define POWER_SYSFILE_SCALING_DRIVER \
> "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
> +#define POWER_SYSFILE_GOVERNOR \
> + "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
> +#define POWER_CONVERT_TO_DECIMAL 10
>
> int
> cpufreq_check_scaling_driver(const char *driver_name)
> @@ -58,3 +65,142 @@ cpufreq_check_scaling_driver(const char *driver_name)
> */
> return 1;
> }
cpufreq_check_scaling_driver can use open_core_sysfs_file, right?
> +
> +int
> +open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
> + FILE **f)
> +{
> + char fullpath[PATH_MAX];
> + FILE *tmpf;
> +
> + /* silenced -Wformat-nonliteral here */
> + snprintf(fullpath, sizeof(fullpath), template, core);
> + tmpf = fopen(fullpath, mode);
> + *f = tmpf;
> + if (tmpf == NULL)
> + return -1;
> +
> + return 0;
> +}
@@ -67,14 +67,15 @@ cpufreq_check_scaling_driver(const char *driver_name)
}
int
-open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
- FILE **f)
+open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
{
char fullpath[PATH_MAX];
+ va_list ap;
FILE *tmpf;
- /* silenced -Wformat-nonliteral here */
- snprintf(fullpath, sizeof(fullpath), template, core);
+ va_start(ap, format);
+ vsnprintf(fullpath, sizeof(fullpath), format, ap);
+ va_end(ap);
tmpf = fopen(fullpath, mode);
*f = tmpf;
if (tmpf == NULL)
With declaration in .h as:
@@ -7,6 +7,8 @@
#include <inttypes.h>
+#include <rte_common.h>
+
#define RTE_POWER_INVALID_FREQ_INDEX (~0)
@@ -21,8 +23,8 @@
int cpufreq_check_scaling_driver(const char *driver);
int power_set_governor(unsigned int lcore_id, const char *new_governor,
char *orig_governor, size_t orig_governor_len);
-int open_core_sysfs_file(const char *template, unsigned int core,
- const char *mode, FILE **f);
+int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
+ __rte_format_printf(3, 4);
int read_core_sysfs_u32(FILE *f, uint32_t *val);
int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
int write_core_sysfs_s(FILE *f, const char *str);
This leaves the possibility to use any kind of formats.
And to be honest, I did not manage to make gcc happy otherwise (even
when passing __rte_format_printf(3, 0)).
On 8/7/2021 1:49 PM, David Marchand wrote:
> On Wed, Jun 23, 2021 at 2:04 PM David Hunt <david.hunt@intel.com> wrote:
>> From: Anatoly Burakov <anatoly.burakov@intel.com>
>>
>> Currently, ACPI and PSTATE modes have lots of code duplication,
>> confusing logic, and a bunch of other issues that can, and have, led to
>> various bugs and resource leaks.
>>
>> This commit factors out the common parts of sysfs reading/writing for
>> ACPI and PSTATE drivers.
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> Signed-off-by: David Hunt <david.hunt@intel.com>
>>
>> ---
>> changes in v5
>> * fixed bugs raised by Richael Zhuang in review - open file rw+, etc.
>> * removed FOPS* and FOPEN* macros, which contained control statements.
>> * fixed some checkpatch warnings.
>> changes in v6
>> * fixed check of fputs return, negative on error.
>> ---
>> lib/power/meson.build | 7 +
>> lib/power/power_acpi_cpufreq.c | 192 ++++------------
>> lib/power/power_common.c | 146 ++++++++++++
>> lib/power/power_common.h | 17 ++
>> lib/power/power_pstate_cpufreq.c | 374 ++++++++++---------------------
>> 5 files changed, 335 insertions(+), 401 deletions(-)
>>
>> diff --git a/lib/power/meson.build b/lib/power/meson.build
>> index c1097d32f1..74c5f3a294 100644
>> --- a/lib/power/meson.build
>> +++ b/lib/power/meson.build
>> @@ -5,6 +5,13 @@ if not is_linux
>> build = false
>> reason = 'only supported on Linux'
>> endif
>> +
>> +# we do some snprintf magic so silence format-nonliteral
>> +flag_nonliteral = '-Wno-format-nonliteral'
>> +if cc.has_argument(flag_nonliteral)
>> + cflags += flag_nonliteral
>> +endif
>> +
> This can be removed with __rte_format_printf tag + API change below.
>
>
>> sources = files(
>> 'guest_channel.c',
>> 'power_acpi_cpufreq.c',
> [snip]
>
>> diff --git a/lib/power/power_common.c b/lib/power/power_common.c
>> index 67e3318ec7..4deb343dae 100644
>> --- a/lib/power/power_common.c
>> +++ b/lib/power/power_common.c
>> @@ -3,13 +3,20 @@
>> */
>>
>> #include <limits.h>
>> +#include <stdlib.h>
>> #include <stdio.h>
>> #include <string.h>
>>
>> +#include <rte_log.h>
>> +#include <rte_string_fns.h>
>> +
>> #include "power_common.h"
>>
>> #define POWER_SYSFILE_SCALING_DRIVER \
>> "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
>> +#define POWER_SYSFILE_GOVERNOR \
>> + "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>> +#define POWER_CONVERT_TO_DECIMAL 10
>>
>> int
>> cpufreq_check_scaling_driver(const char *driver_name)
>> @@ -58,3 +65,142 @@ cpufreq_check_scaling_driver(const char *driver_name)
>> */
>> return 1;
>> }
> cpufreq_check_scaling_driver can use open_core_sysfs_file, right?
>
>
>> +
>> +int
>> +open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
>> + FILE **f)
>> +{
>> + char fullpath[PATH_MAX];
>> + FILE *tmpf;
>> +
>> + /* silenced -Wformat-nonliteral here */
>> + snprintf(fullpath, sizeof(fullpath), template, core);
>> + tmpf = fopen(fullpath, mode);
>> + *f = tmpf;
>> + if (tmpf == NULL)
>> + return -1;
>> +
>> + return 0;
>> +}
>
> @@ -67,14 +67,15 @@ cpufreq_check_scaling_driver(const char *driver_name)
> }
>
> int
> -open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
> - FILE **f)
> +open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
> {
> char fullpath[PATH_MAX];
> + va_list ap;
> FILE *tmpf;
>
> - /* silenced -Wformat-nonliteral here */
> - snprintf(fullpath, sizeof(fullpath), template, core);
> + va_start(ap, format);
> + vsnprintf(fullpath, sizeof(fullpath), format, ap);
> + va_end(ap);
> tmpf = fopen(fullpath, mode);
> *f = tmpf;
> if (tmpf == NULL)
>
>
>
> With declaration in .h as:
>
> @@ -7,6 +7,8 @@
>
> #include <inttypes.h>
>
> +#include <rte_common.h>
> +
> #define RTE_POWER_INVALID_FREQ_INDEX (~0)
>
>
> @@ -21,8 +23,8 @@
> int cpufreq_check_scaling_driver(const char *driver);
> int power_set_governor(unsigned int lcore_id, const char *new_governor,
> char *orig_governor, size_t orig_governor_len);
> -int open_core_sysfs_file(const char *template, unsigned int core,
> - const char *mode, FILE **f);
> +int open_core_sysfs_file(FILE **f, const char *mode, const char *format, ...)
> + __rte_format_printf(3, 4);
> int read_core_sysfs_u32(FILE *f, uint32_t *val);
> int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
> int write_core_sysfs_s(FILE *f, const char *str);
>
>
> This leaves the possibility to use any kind of formats.
> And to be honest, I did not manage to make gcc happy otherwise (even
> when passing __rte_format_printf(3, 0)).
Thanks, David, good suggestions. I'll make those changes and respin.
Rgds,
Dave.
>
@@ -5,6 +5,13 @@ if not is_linux
build = false
reason = 'only supported on Linux'
endif
+
+# we do some snprintf magic so silence format-nonliteral
+flag_nonliteral = '-Wno-format-nonliteral'
+if cc.has_argument(flag_nonliteral)
+ cflags += flag_nonliteral
+endif
+
sources = files(
'guest_channel.c',
'power_acpi_cpufreq.c',
@@ -19,41 +19,10 @@
#include "power_acpi_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 opened\n"); \
- return retval; \
- } \
-} while (0)
-
-#define FOPS_OR_NULL_GOTO(ret, label) do { \
- if ((ret) == NULL) { \
- RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
- goto label; \
- } \
-} while (0)
-
-#define FOPS_OR_ERR_GOTO(ret, label) do { \
- if ((ret) < 0) { \
- RTE_LOG(ERR, POWER, "File operations failed\n"); \
- goto label; \
- } \
-} while (0)
-
#define STR_SIZE 1024
#define POWER_CONVERT_TO_DECIMAL 10
#define POWER_GOVERNOR_USERSPACE "userspace"
-#define POWER_SYSFILE_GOVERNOR \
- "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
#define POWER_SYSFILE_AVAIL_FREQ \
"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_frequencies"
#define POWER_SYSFILE_SETSPEED \
@@ -135,53 +104,18 @@ set_freq_internal(struct acpi_power_info *pi, uint32_t idx)
static int
power_set_governor_userspace(struct acpi_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);
- /* Strip off terminating '\n' */
- strtok(buf, "\n");
-
- /* Save the original governor */
- rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
-
- /* Check if current governor is userspace */
- if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
- sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
- ret = 0;
- POWER_DEBUG_TRACE("Power management governor of lcore %u is "
- "already userspace\n", pi->lcore_id);
- goto out;
- }
-
- /* Write 'userspace' to the governor */
- val = fseek(f, 0, SEEK_SET);
- FOPS_OR_ERR_GOTO(val, out);
-
- val = fputs(POWER_GOVERNOR_USERSPACE, f);
- FOPS_OR_ERR_GOTO(val, out);
-
- /* We need to flush to see if the fputs succeeds */
- val = fflush(f);
- FOPS_OR_ERR_GOTO(val, out);
-
- ret = 0;
- RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
- "set to user space successfully\n", pi->lcore_id);
-out:
- fclose(f);
+ return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE,
+ pi->governor_ori, sizeof(pi->governor_ori));
+}
- 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 acpi_power_info *pi)
+{
+ return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
}
/**
@@ -195,22 +129,22 @@ power_get_available_freqs(struct acpi_power_info *pi)
int ret = -1, i, count;
char *p;
char buf[BUFSIZ];
- char fullpath[PATH_MAX];
char *freqs[RTE_MAX_LCORE_FREQS];
- char *s;
-
- snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
- pi->lcore_id);
- f = fopen(fullpath, "r");
- FOPEN_OR_ERR_RET(f, ret);
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
+ open_core_sysfs_file(POWER_SYSFILE_AVAIL_FREQ, pi->lcore_id, "r",
+ &f);
+ if (f == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_AVAIL_FREQ);
+ goto out;
+ }
- /* Strip the line break if there is */
- p = strchr(buf, '\n');
- if (p != NULL)
- *p = 0;
+ ret = read_core_sysfs_s(f, buf, sizeof(buf));
+ if ((ret) < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_AVAIL_FREQ);
+ goto out;
+ }
/* Split string into at most RTE_MAX_LCORE_FREQS frequencies */
count = rte_strsplit(buf, sizeof(buf), freqs,
@@ -250,7 +184,8 @@ power_get_available_freqs(struct acpi_power_info *pi)
POWER_DEBUG_TRACE("%d frequency(s) of lcore %u are available\n",
count, pi->lcore_id);
out:
- fclose(f);
+ if (f != NULL)
+ fclose(f);
return ret;
}
@@ -262,18 +197,24 @@ static int
power_init_for_setting_freq(struct acpi_power_info *pi)
{
FILE *f;
- char fullpath[PATH_MAX];
char buf[BUFSIZ];
uint32_t i, freq;
- char *s;
+ int ret;
- snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
- pi->lcore_id);
- f = fopen(fullpath, "rw+");
- FOPEN_OR_ERR_RET(f, -1);
+ open_core_sysfs_file(POWER_SYSFILE_SETSPEED, pi->lcore_id, "rw+",
+ &f);
+ if (f == NULL) {
+ RTE_LOG(ERR, POWER, "Failed to open %s\n",
+ POWER_SYSFILE_SETSPEED);
+ goto err;
+ }
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
+ ret = read_core_sysfs_s(f, buf, sizeof(buf));
+ if ((ret) < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_SETSPEED);
+ goto err;
+ }
freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
for (i = 0; i < pi->nb_freqs; i++) {
@@ -284,8 +225,9 @@ power_init_for_setting_freq(struct acpi_power_info *pi)
}
}
-out:
- fclose(f);
+err:
+ if (f != NULL)
+ fclose(f);
return -1;
}
@@ -369,54 +311,6 @@ power_acpi_cpufreq_init(unsigned int lcore_id)
return -1;
}
-/**
- * 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 acpi_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;
-}
-
int
power_acpi_cpufreq_exit(unsigned int lcore_id)
{
@@ -3,13 +3,20 @@
*/
#include <limits.h>
+#include <stdlib.h>
#include <stdio.h>
#include <string.h>
+#include <rte_log.h>
+#include <rte_string_fns.h>
+
#include "power_common.h"
#define POWER_SYSFILE_SCALING_DRIVER \
"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
+#define POWER_SYSFILE_GOVERNOR \
+ "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
+#define POWER_CONVERT_TO_DECIMAL 10
int
cpufreq_check_scaling_driver(const char *driver_name)
@@ -58,3 +65,142 @@ cpufreq_check_scaling_driver(const char *driver_name)
*/
return 1;
}
+
+int
+open_core_sysfs_file(const char *template, unsigned int core, const char *mode,
+ FILE **f)
+{
+ char fullpath[PATH_MAX];
+ FILE *tmpf;
+
+ /* silenced -Wformat-nonliteral here */
+ snprintf(fullpath, sizeof(fullpath), template, core);
+ tmpf = fopen(fullpath, mode);
+ *f = tmpf;
+ if (tmpf == NULL)
+ return -1;
+
+ return 0;
+}
+
+int
+read_core_sysfs_u32(FILE *f, uint32_t *val)
+{
+ char buf[BUFSIZ];
+ uint32_t fval;
+ char *s;
+
+ s = fgets(buf, sizeof(buf), f);
+ if (s == NULL)
+ return -1;
+
+ /* fgets puts null terminator in, but do this just in case */
+ buf[BUFSIZ - 1] = '\0';
+
+ /* strip off any terminating newlines */
+ *strchrnul(buf, '\n') = '\0';
+
+ fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+ /* write the value */
+ *val = fval;
+
+ return 0;
+}
+
+int
+read_core_sysfs_s(FILE *f, char *buf, unsigned int len)
+{
+ char *s;
+
+ s = fgets(buf, len, f);
+ if (s == NULL)
+ return -1;
+
+ /* fgets puts null terminator in, but do this just in case */
+ buf[len - 1] = '\0';
+
+ /* strip off any terminating newlines */
+ *strchrnul(buf, '\n') = '\0';
+
+ return 0;
+}
+
+int
+write_core_sysfs_s(FILE *f, const char *str)
+{
+ int ret;
+
+ ret = fseek(f, 0, SEEK_SET);
+ if (ret != 0)
+ return -1;
+
+ ret = fputs(str, f);
+ if (ret < 0)
+ return -1;
+
+ /* flush the output */
+ ret = fflush(f);
+ if (ret != 0)
+ return -1;
+
+ return 0;
+}
+
+/**
+ * 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.
+ */
+int
+power_set_governor(unsigned int lcore_id, const char *new_governor,
+ char *orig_governor, size_t orig_governor_len)
+{
+ FILE *f_governor = NULL;
+ int ret = -1;
+ char buf[BUFSIZ];
+
+ open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, lcore_id, "rw+",
+ &f_governor);
+ if (f_governor == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_GOVERNOR);
+ goto out;
+ }
+
+ ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_GOVERNOR);
+ goto out;
+ }
+
+ /* Save the original governor, if it was provided */
+ if (orig_governor)
+ rte_strscpy(orig_governor, buf, orig_governor_len);
+
+ /* Check if current governor is already what we want */
+ if (strcmp(buf, new_governor) == 0) {
+ ret = 0;
+ POWER_DEBUG_TRACE("Power management governor of lcore %u is "
+ "already %s\n", lcore_id, new_governor);
+ goto out;
+ }
+
+ /* Write the new governor */
+ ret = write_core_sysfs_s(f_governor, new_governor);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to write %s\n",
+ POWER_SYSFILE_GOVERNOR);
+ goto out;
+ }
+
+ ret = 0;
+ RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
+ "set to '%s' successfully\n", lcore_id, new_governor);
+out:
+ if (f_governor != NULL)
+ fclose(f_governor);
+
+ return ret;
+}
@@ -5,9 +5,26 @@
#ifndef _POWER_COMMON_H_
#define _POWER_COMMON_H_
+#include <inttypes.h>
+
#define RTE_POWER_INVALID_FREQ_INDEX (~0)
+
+#ifdef RTE_LIBRTE_POWER_DEBUG
+#define POWER_DEBUG_TRACE(fmt, args...) \
+ RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args)
+#else
+#define POWER_DEBUG_TRACE(fmt, args...)
+#endif
+
/* check if scaling driver matches one we want */
int cpufreq_check_scaling_driver(const char *driver);
+int power_set_governor(unsigned int lcore_id, const char *new_governor,
+ char *orig_governor, size_t orig_governor_len);
+int open_core_sysfs_file(const char *template, unsigned int core,
+ const char *mode, FILE **f);
+int read_core_sysfs_u32(FILE *f, uint32_t *val);
+int read_core_sysfs_s(FILE *f, char *buf, unsigned int len);
+int write_core_sysfs_s(FILE *f, const char *str);
#endif /* _POWER_COMMON_H_ */
@@ -21,46 +21,13 @@
#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 opened\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)
-
/* macros used for rounding frequency to nearest 100000 */
#define FREQ_ROUNDING_DELTA 50000
#define ROUND_FREQ_TO_N_100000 100000
-#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 \
@@ -154,91 +121,78 @@ out: close(fd);
static int
power_init_for_setting_freq(struct pstate_power_info *pi)
{
- FILE *f_min, *f_max, *f_base = NULL, *f_base_max;
- char fullpath_min[PATH_MAX];
- char fullpath_max[PATH_MAX];
- char fullpath_base[PATH_MAX];
- char fullpath_base_max[PATH_MAX];
- char buf_base[BUFSIZ];
- char *s_base;
- char *s_base_max;
- uint32_t base_ratio = 0;
- uint32_t base_max_ratio = 0;
- uint64_t max_non_turbo = 0;
- int ret_val = 0;
-
- snprintf(fullpath_base_max,
- sizeof(fullpath_base_max),
- POWER_SYSFILE_BASE_MAX_FREQ,
- pi->lcore_id);
- f_base_max = fopen(fullpath_base_max, "r");
- FOPEN_OR_ERR_RET(f_base_max, -1);
- if (f_base_max != NULL) {
- s_base_max = fgets(buf_base, sizeof(buf_base), f_base_max);
-
- /* close the file unconditionally */
- fclose(f_base_max);
- f_base_max = NULL;
-
- FOPS_OR_NULL_GOTO(s_base_max, out);
-
- buf_base[BUFSIZ-1] = '\0';
- if (strlen(buf_base))
- /* Strip off terminating '\n' */
- strtok(buf_base, "\n");
-
- base_max_ratio =
- strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
- / BUS_FREQ;
+ FILE *f_base = NULL, *f_base_max = NULL, *f_min = NULL, *f_max = NULL;
+ uint32_t base_ratio, base_max_ratio;
+ uint64_t max_non_turbo;
+ int ret;
+
+ /* open all files we expect to have open */
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+ &f_base_max);
+ if (f_base_max == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_BASE_MAX_FREQ);
+ goto err;
+ }
+
+ open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+ &f_min);
+ if (f_min == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_MIN_FREQ);
+ goto err;
+ }
+
+ open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+ &f_max);
+ if (f_max == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_MAX_FREQ);
+ goto err;
+ }
+
+ open_core_sysfs_file(POWER_SYSFILE_BASE_FREQ, pi->lcore_id, "r",
+ &f_base);
+ /* base ratio file may not exist in some kernels, so no error check */
+
+ /* read base max ratio */
+ ret = read_core_sysfs_u32(f_base_max, &base_max_ratio);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_BASE_MAX_FREQ);
+ goto err;
+ }
+
+ /* base ratio may not exist */
+ if (f_base != NULL) {
+ ret = read_core_sysfs_u32(f_base, &base_ratio);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_BASE_FREQ);
+ goto err;
+ }
+ } else {
+ base_ratio = 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);
+ /* Add MSR read to detect turbo status */
+ if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0)
+ goto err;
+ /* no errors after this point */
- snprintf(fullpath_max, sizeof(fullpath_max), POWER_SYSFILE_MAX_FREQ,
- pi->lcore_id);
- f_max = fopen(fullpath_max, "rw+");
- if (f_max == NULL)
- fclose(f_min);
- FOPEN_OR_ERR_RET(f_max, -1);
+ /* convert ratios to bins */
+ base_max_ratio /= BUS_FREQ;
+ base_ratio /= BUS_FREQ;
+ /* assign file handles */
pi->f_cur_min = f_min;
pi->f_cur_max = f_max;
- snprintf(fullpath_base, sizeof(fullpath_base), POWER_SYSFILE_BASE_FREQ,
- pi->lcore_id);
-
- f_base = fopen(fullpath_base, "r");
- if (f_base == NULL) {
- /* No sysfs base_frequency, that's OK, continue without */
- base_ratio = 0;
- } else {
- s_base = fgets(buf_base, sizeof(buf_base), f_base);
- FOPS_OR_NULL_GOTO(s_base, out);
-
- buf_base[BUFSIZ-1] = '\0';
- if (strlen(buf_base))
- /* Strip off terminating '\n' */
- strtok(buf_base, "\n");
-
- base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
- / BUS_FREQ;
- }
-
- /* Add MSR read to detect turbo status */
-
- if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
- ret_val = -1;
- goto out;
- }
-
max_non_turbo = (max_non_turbo&NON_TURBO_MASK)>>NON_TURBO_OFFSET;
POWER_DEBUG_TRACE("no turbo perf %"PRIu64"\n", max_non_turbo);
- pi->non_turbo_max_ratio = max_non_turbo;
+ pi->non_turbo_max_ratio = (uint32_t)max_non_turbo;
/*
* If base_frequency is reported as greater than the maximum
@@ -264,7 +218,20 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
out:
if (f_base != NULL)
fclose(f_base);
- return ret_val;
+ fclose(f_base_max);
+ /* f_min and f_max are stored, no need to close */
+ return 0;
+
+err:
+ if (f_base != NULL)
+ fclose(f_base);
+ if (f_base_max != NULL)
+ fclose(f_base_max);
+ if (f_min != NULL)
+ fclose(f_min);
+ if (f_max != NULL)
+ fclose(f_max);
+ return -1;
}
static int
@@ -369,53 +336,8 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
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);
- /* Strip off terminating '\n' */
- strtok(buf, "\n");
-
- /* Save the original governor */
- rte_strscpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
-
- /* 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;
- }
-
- /* 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);
-
- /* We need to flush to see if the fputs succeeds */
- val = fflush(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;
+ return power_set_governor(pi->lcore_id, POWER_GOVERNOR_PERF,
+ pi->governor_ori, sizeof(pi->governor_ori));
}
/**
@@ -425,45 +347,7 @@ power_set_governor_performance(struct pstate_power_info *pi)
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;
+ return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
}
/**
@@ -473,51 +357,42 @@ power_set_governor_original(struct pstate_power_info *pi)
static int
power_get_available_freqs(struct pstate_power_info *pi)
{
- FILE *f_min, *f_max;
+ FILE *f_min = NULL, *f_max = NULL;
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);
-
- f_max = fopen(fullpath_max, "r");
- if (f_max == NULL)
- fclose(f_min);
-
- FOPEN_OR_ERR_RET(f_max, ret);
-
- s_min = fgets(buf_min, sizeof(buf_min), f_min);
- FOPS_OR_NULL_GOTO(s_min, out);
-
- s_max = fgets(buf_max, sizeof(buf_max), f_max);
- FOPS_OR_NULL_GOTO(s_max, out);
-
+ /* open all files */
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+ &f_max);
+ if (f_max == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_BASE_MAX_FREQ);
+ goto out;
+ }
- /* Strip the line break if there is */
- p_min = strchr(buf_min, '\n');
- if (p_min != NULL)
- *p_min = 0;
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+ &f_min);
+ if (f_min == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_BASE_MIN_FREQ);
+ goto out;
+ }
- p_max = strchr(buf_max, '\n');
- if (p_max != NULL)
- *p_max = 0;
+ /* read base ratios */
+ ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_BASE_MAX_FREQ);
+ goto out;
+ }
- sys_min_freq = strtoul(buf_min, &p_min, POWER_CONVERT_TO_DECIMAL);
- sys_max_freq = strtoul(buf_max, &p_max, POWER_CONVERT_TO_DECIMAL);
+ ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_BASE_MIN_FREQ);
+ goto out;
+ }
if (sys_max_freq < sys_min_freq)
goto out;
@@ -576,27 +451,22 @@ power_get_cur_idx(struct pstate_power_info *pi)
{
FILE *f_cur;
int ret = -1;
- char *p_cur;
- char buf_cur[BUFSIZ];
- char fullpath_cur[PATH_MAX];
- char *s_cur;
uint32_t sys_cur_freq = 0;
unsigned int i;
- snprintf(fullpath_cur, sizeof(fullpath_cur),
- POWER_SYSFILE_CUR_FREQ,
- pi->lcore_id);
- f_cur = fopen(fullpath_cur, "r");
- FOPEN_OR_ERR_RET(f_cur, ret);
-
- /* initialize the cur_idx to matching current frequency freq index */
- s_cur = fgets(buf_cur, sizeof(buf_cur), f_cur);
- FOPS_OR_NULL_GOTO(s_cur, fail);
+ open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+ if (f_cur == NULL) {
+ RTE_LOG(ERR, POWER, "failed to open %s\n",
+ POWER_SYSFILE_CUR_FREQ);
+ goto fail;
+ }
- p_cur = strchr(buf_cur, '\n');
- if (p_cur != NULL)
- *p_cur = 0;
- sys_cur_freq = strtoul(buf_cur, &p_cur, POWER_CONVERT_TO_DECIMAL);
+ ret = read_core_sysfs_u32(f_cur, &sys_cur_freq);
+ if (ret < 0) {
+ RTE_LOG(ERR, POWER, "Failed to read %s\n",
+ POWER_SYSFILE_CUR_FREQ);
+ goto fail;
+ }
/* convert the frequency to nearest 100000 value
* Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -615,10 +485,10 @@ power_get_cur_idx(struct pstate_power_info *pi)
}
}
- fclose(f_cur);
- return 0;
+ ret = 0;
fail:
- fclose(f_cur);
+ if (f_cur != NULL)
+ fclose(f_cur);
return ret;
}