* [dpdk-dev] [PATCH 2/3] power: refactor pstate sysfs handling
2021-03-30 14:15 [dpdk-dev] [PATCH 1/3] power: refactor base frequency detection Anatoly Burakov
@ 2021-03-30 14:15 ` Anatoly Burakov
2021-03-30 14:15 ` [dpdk-dev] [PATCH 3/3] power: do not skip saving original pstate governor Anatoly Burakov
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-03-30 14:15 UTC (permalink / raw)
To: dev; +Cc: David Hunt
Make use of new sysfs handling functions in other pstate code.
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 177 +++++++++++-------------
1 file changed, 80 insertions(+), 97 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index add06720db..7ea1bf677a 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -198,6 +198,46 @@ read_core_sysfs_u32(FILE *f, uint32_t *val)
return 0;
}
+static 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 */
+ if (strlen(buf))
+ strtok(buf, "\n");
+
+ return 0;
+}
+
+static 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 fopen the sys file for the future setting the lcore frequency.
*/
@@ -399,22 +439,16 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
static int
power_set_governor_performance(struct pstate_power_info *pi)
{
- FILE *f;
+ FILE *f_governor = NULL;
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);
+ open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+ &f_governor);
+ FOPEN_OR_ERR_GOTO(f_governor, out);
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
- /* Strip off terminating '\n' */
- strtok(buf, "\n");
+ ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+ FOPS_OR_ERR_GOTO(ret, out);
/* Check if current governor is performance */
if (strncmp(buf, POWER_GOVERNOR_PERF,
@@ -428,21 +462,15 @@ power_set_governor_performance(struct pstate_power_info *pi)
strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* 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 = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
+ FOPS_OR_ERR_GOTO(ret, 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);
+ if (f_governor != NULL)
+ fclose(f_governor);
return ret;
}
@@ -454,20 +482,16 @@ power_set_governor_performance(struct pstate_power_info *pi)
static int
power_set_governor_original(struct pstate_power_info *pi)
{
- FILE *f;
+ FILE *f_governor = NULL;
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);
+ open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+ &f_governor);
+ FOPEN_OR_ERR_GOTO(f_governor, out);
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
+ ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+ FOPS_OR_ERR_GOTO(ret, out);
/* Check if the governor to be set is the same as current */
if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -479,19 +503,16 @@ power_set_governor_original(struct pstate_power_info *pi)
}
/* 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 = write_core_sysfs_s(f_governor, pi->governor_ori);
+ FOPS_OR_ERR_GOTO(ret, 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);
-
+ if (f_governor != NULL)
+ fclose(f_governor);
return ret;
}
@@ -502,51 +523,26 @@ 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);
+ /* open all files */
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+ &f_max);
+ FOPEN_OR_ERR_GOTO(f_max, out);
- f_min = fopen(fullpath_min, "r");
- FOPEN_OR_ERR_RET(f_min, ret);
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+ &f_min);
+ FOPEN_OR_ERR_GOTO(f_max, out);
- f_max = fopen(fullpath_max, "r");
- if (f_max == NULL)
- fclose(f_min);
+ /* read base ratios */
+ ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+ FOPS_OR_ERR_GOTO(ret, out);
- 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);
-
-
- /* 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);
+ ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+ FOPS_OR_ERR_GOTO(ret, out);
if (sys_max_freq < sys_min_freq)
goto out;
@@ -605,27 +601,14 @@ 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);
+ open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+ FOPEN_OR_ERR_GOTO(f_cur, fail);
- /* 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);
-
- 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);
+ FOPS_OR_ERR_GOTO(ret, fail);
/* convert the frequency to nearest 100000 value
* Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -644,10 +627,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.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH 3/3] power: do not skip saving original pstate governor
2021-03-30 14:15 [dpdk-dev] [PATCH 1/3] power: refactor base frequency detection Anatoly Burakov
2021-03-30 14:15 ` [dpdk-dev] [PATCH 2/3] power: refactor pstate sysfs handling Anatoly Burakov
@ 2021-03-30 14:15 ` Anatoly Burakov
2021-03-30 16:52 ` Burakov, Anatoly
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
` (2 subsequent siblings)
4 siblings, 1 reply; 19+ messages in thread
From: Anatoly Burakov @ 2021-03-30 14:15 UTC (permalink / raw)
To: dev; +Cc: David Hunt
Currently, when we set the pstate governor to "performance", we check if
it is already set to this value, and if it is, we skip setting it.
However, we never save this value anywhere, so that next time we come
back and request the governor to be set to its original value, the
original value is empty.
Fix it by saving the original pstate governor first. While we're at it,
replace `strlcpy` with `strscpy`.
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 7ea1bf677a..a7a44df23f 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -450,6 +450,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
FOPS_OR_ERR_GOTO(ret, out);
+ /* Save the original governor */
+ 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) {
@@ -458,8 +461,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
"already performance\n", pi->lcore_id);
goto out;
}
- /* Save the original governor */
- strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* Write 'performance' to the governor */
ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH 3/3] power: do not skip saving original pstate governor
2021-03-30 14:15 ` [dpdk-dev] [PATCH 3/3] power: do not skip saving original pstate governor Anatoly Burakov
@ 2021-03-30 16:52 ` Burakov, Anatoly
0 siblings, 0 replies; 19+ messages in thread
From: Burakov, Anatoly @ 2021-03-30 16:52 UTC (permalink / raw)
To: dev; +Cc: David Hunt
On 30-Mar-21 3:15 PM, Anatoly Burakov wrote:
> Currently, when we set the pstate governor to "performance", we check if
> it is already set to this value, and if it is, we skip setting it.
>
> However, we never save this value anywhere, so that next time we come
> back and request the governor to be set to its original value, the
> original value is empty.
>
> Fix it by saving the original pstate governor first. While we're at it,
> replace `strlcpy` with `strscpy`.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> lib/librte_power/power_pstate_cpufreq.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> index 7ea1bf677a..a7a44df23f 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -450,6 +450,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
> ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
> FOPS_OR_ERR_GOTO(ret, out);
>
> + /* Save the original governor */
> + 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) {
> @@ -458,8 +461,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
> "already performance\n", pi->lcore_id);
> goto out;
> }
> - /* Save the original governor */
> - strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
>
> /* Write 'performance' to the governor */
> ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
>
Forgot rte_ with strscpy...
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection
2021-03-30 14:15 [dpdk-dev] [PATCH 1/3] power: refactor base frequency detection Anatoly Burakov
2021-03-30 14:15 ` [dpdk-dev] [PATCH 2/3] power: refactor pstate sysfs handling Anatoly Burakov
2021-03-30 14:15 ` [dpdk-dev] [PATCH 3/3] power: do not skip saving original pstate governor Anatoly Burakov
@ 2021-03-30 14:25 ` Anatoly Burakov
2021-04-01 15:04 ` [dpdk-dev] [PATCH v2 1/2] power: fix pstate base frequency handling Anatoly Burakov
` (3 more replies)
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 2/3] power: refactor pstate sysfs handling Anatoly Burakov
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 3/3] power: do not skip saving original pstate governor Anatoly Burakov
4 siblings, 4 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-03-30 14:25 UTC (permalink / raw)
To: dev; +Cc: David Hunt
Currently, base frequency detection code is a bit of an unmaintainable
mess that has a couple of Coverity issues (dead code, and a resource
leak). Rather than just fixing the issues and leaving the rest in place,
refactor the code in a way that makes it more maintainable and less
prone to such Coverity issues in the first place.
Coverity issue: 369693
Coverity issue: 369694
Fixes: 4db9587bbf72 ("power: check sysfs base frequency")
Cc: david.hunt@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/meson.build | 7 +
lib/librte_power/power_pstate_cpufreq.c | 178 ++++++++++++++----------
2 files changed, 113 insertions(+), 72 deletions(-)
diff --git a/lib/librte_power/meson.build b/lib/librte_power/meson.build
index 9a2dcbfc7a..fd408ffd4c 100644
--- a/lib/librte_power/meson.build
+++ b/lib/librte_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('rte_power.c', 'power_acpi_cpufreq.c',
'power_kvm_vm.c', 'guest_channel.c',
'rte_power_empty_poll.c',
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 8a1fffaed5..add06720db 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -37,6 +37,13 @@
} \
} while (0)
+#define FOPEN_OR_ERR_GOTO(f, label) do { \
+ if ((f) == NULL) { \
+ RTE_LOG(ERR, POWER, "File not opened\n"); \
+ goto label; \
+ } \
+} while (0)
+
#define FOPS_OR_NULL_GOTO(ret, label) do { \
if ((ret) == NULL) { \
RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
@@ -148,93 +155,107 @@ out: close(fd);
return ret;
}
+static 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);
+ if (tmpf == NULL)
+ return -1;
+ *f = tmpf;
+
+ return 0;
+}
+
+static 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 */
+ if (strlen(buf))
+ strtok(buf, "\n");
+
+ fval = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
+
+ /* write the value */
+ *val = fval;
+
+ return 0;
+}
+
/**
* 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, *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);
- 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;
- }
-
- 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+");
- if (f_max == NULL)
- fclose(f_min);
- FOPEN_OR_ERR_RET(f_max, -1);
-
- 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");
- FOPEN_OR_ERR_RET(f_base, -1);
- if (f_base == NULL) {
- /* No sysfs base_frequency, that's OK, continue without */
- base_ratio = 0;
+ 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);
+ FOPEN_OR_ERR_GOTO(f_base_max, err);
+
+ open_core_sysfs_file(POWER_SYSFILE_MIN_FREQ, pi->lcore_id, "rw+",
+ &f_min);
+ FOPEN_OR_ERR_GOTO(f_min, err);
+
+ open_core_sysfs_file(POWER_SYSFILE_MAX_FREQ, pi->lcore_id, "rw+",
+ &f_max);
+ FOPEN_OR_ERR_GOTO(f_max, 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);
+ FOPS_OR_ERR_GOTO(ret, err);
+
+ /* base ratio may not exist */
+ if (f_base != NULL) {
+ ret = read_core_sysfs_u32(f_base, &base_ratio);
+ FOPS_OR_ERR_GOTO(ret, err);
} 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;
+ base_ratio = 0;
}
/* 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 */
- if (power_rdmsr(PLATFORM_INFO, &max_non_turbo, pi->lcore_id) < 0) {
- ret_val = -1;
- goto out;
- }
+ /* 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;
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
@@ -260,7 +281,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
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH v2 1/2] power: fix pstate base frequency handling
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
@ 2021-04-01 15:04 ` Anatoly Burakov
2021-04-01 15:05 ` [dpdk-dev] [PATCH v2 2/2] power: do not skip saving original pstate governor Anatoly Burakov
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-01 15:04 UTC (permalink / raw)
To: dev; +Cc: anatoly.burakov, david.hunt
Previous fix for base frequency handling in pstate mode introduced a
couple of issues:
- When base_frequency file does not exist, it simply bails out because
of what appears to be accidental addition of FOPEN_OR_ERR_RET. This is
incorrect, as absence of this file is not fatal and is in fact
expected on kernel versions earlier than 5.3
- When base_frequency file does exist, it gets opened, but never gets
closed, resulting in a resource leak
Both issues also manifest themselves as Coverity defects (dead code, and
a resource leak), so this fix addresses both.
Fixes: 4db9587bbf72 ("power: check sysfs base frequency")
Cc: david.hunt@intel.com
Coverity issue: 369693
Coverity issue: 369694
Bugzilla ID: 668
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 8a1fffaed5..af5ad0b506 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -172,7 +172,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
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);
FOPS_OR_NULL_GOTO(s_base_max, out);
@@ -185,6 +184,7 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
base_max_ratio =
strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
/ BUS_FREQ;
+ fclose(f_base_max);
}
snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH v2 2/2] power: do not skip saving original pstate governor
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
2021-04-01 15:04 ` [dpdk-dev] [PATCH v2 1/2] power: fix pstate base frequency handling Anatoly Burakov
@ 2021-04-01 15:05 ` Anatoly Burakov
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 1/2] power: fix pstate base frequency handling Anatoly Burakov
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 " Anatoly Burakov
3 siblings, 0 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-01 15:05 UTC (permalink / raw)
To: dev; +Cc: anatoly.burakov, david.hunt
Currently, when we set the pstate governor to "performance", we check if
it is already set to this value, and if it is, we skip setting it.
However, we never save this value anywhere, so that next time we come
back and request the governor to be set to its original value, the
original value is empty.
Fix it by saving the original pstate governor first. While we're at it,
replace `strlcpy` with `rte_strscpy`.
Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
Cc: david.hunt@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index af5ad0b506..3a5face4f0 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -382,6 +382,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
/* 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) {
@@ -390,8 +393,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
"already performance\n", pi->lcore_id);
goto out;
}
- /* Save the original governor */
- strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* Write 'performance' to the governor */
val = fseek(f, 0, SEEK_SET);
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH v3 1/2] power: fix pstate base frequency handling
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
2021-04-01 15:04 ` [dpdk-dev] [PATCH v2 1/2] power: fix pstate base frequency handling Anatoly Burakov
2021-04-01 15:05 ` [dpdk-dev] [PATCH v2 2/2] power: do not skip saving original pstate governor Anatoly Burakov
@ 2021-04-01 15:06 ` Anatoly Burakov
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 " Anatoly Burakov
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor Anatoly Burakov
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 " Anatoly Burakov
3 siblings, 2 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-01 15:06 UTC (permalink / raw)
To: dev; +Cc: anatoly.burakov, david.hunt
Previous fix for base frequency handling in pstate mode introduced a
couple of issues:
- When base_frequency file does not exist, it simply bails out because
of what appears to be accidental addition of FOPEN_OR_ERR_RET. This is
incorrect, as absence of this file is not fatal and is in fact
expected on kernel versions earlier than 5.3
- When base_frequency file does exist, it gets opened, but never gets
closed, resulting in a resource leak
Both issues also manifest themselves as Coverity defects (dead code, and
a resource leak), so this fix addresses both.
Fixes: 4db9587bbf72 ("power: check sysfs base frequency")
Cc: david.hunt@intel.com
Coverity issue: 369693
Coverity issue: 369694
Bugzilla ID: 668
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 8a1fffaed5..af5ad0b506 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -172,7 +172,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
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);
FOPS_OR_NULL_GOTO(s_base_max, out);
@@ -185,6 +184,7 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
base_max_ratio =
strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
/ BUS_FREQ;
+ fclose(f_base_max);
}
snprintf(fullpath_min, sizeof(fullpath_min), POWER_SYSFILE_MIN_FREQ,
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH v4 1/2] power: fix pstate base frequency handling
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 1/2] power: fix pstate base frequency handling Anatoly Burakov
@ 2021-04-02 9:26 ` Anatoly Burakov
2021-04-02 9:35 ` Burakov, Anatoly
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor Anatoly Burakov
1 sibling, 1 reply; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-02 9:26 UTC (permalink / raw)
To: dev; +Cc: david.hunt
Previous fix for base frequency handling in pstate mode introduced a
couple of issues:
- When base_frequency file does not exist, it simply bails out because
of what appears to be accidental addition of FOPEN_OR_ERR_RET. This is
incorrect, as absence of this file is not fatal and is in fact
expected on kernel versions earlier than 5.3
- When base_frequency file does exist, it gets opened, but never gets
closed, resulting in a resource leak
Both issues also manifest themselves as Coverity defects (dead code, and
a resource leak), so this fix addresses both.
Fixes: 4db9587bbf72 ("power: check sysfs base frequency")
Cc: david.hunt@intel.com
Coverity issue: 369693
Coverity issue: 369694
Bugzilla ID: 668
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 8a1fffaed5..c4639e4b8a 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -206,7 +206,6 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
pi->lcore_id);
f_base = fopen(fullpath_base, "r");
- FOPEN_OR_ERR_RET(f_base, -1);
if (f_base == NULL) {
/* No sysfs base_frequency, that's OK, continue without */
base_ratio = 0;
@@ -221,6 +220,7 @@ power_init_for_setting_freq(struct pstate_power_info *pi)
base_ratio = strtoul(buf_base, NULL, POWER_CONVERT_TO_DECIMAL)
/ BUS_FREQ;
+ fclose(f_base);
}
/* Add MSR read to detect turbo status */
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH v4 1/2] power: fix pstate base frequency handling
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 " Anatoly Burakov
@ 2021-04-02 9:35 ` Burakov, Anatoly
2021-04-02 11:12 ` Pattan, Reshma
0 siblings, 1 reply; 19+ messages in thread
From: Burakov, Anatoly @ 2021-04-02 9:35 UTC (permalink / raw)
To: dev; +Cc: david.hunt
On 02-Apr-21 10:26 AM, Anatoly Burakov wrote:
> Previous fix for base frequency handling in pstate mode introduced a
> couple of issues:
>
> - When base_frequency file does not exist, it simply bails out because
> of what appears to be accidental addition of FOPEN_OR_ERR_RET. This is
> incorrect, as absence of this file is not fatal and is in fact
> expected on kernel versions earlier than 5.3
> - When base_frequency file does exist, it gets opened, but never gets
> closed, resulting in a resource leak
>
> Both issues also manifest themselves as Coverity defects (dead code, and
> a resource leak), so this fix addresses both.
>
> Fixes: 4db9587bbf72 ("power: check sysfs base frequency")
> Cc: david.hunt@intel.com
>
> Coverity issue: 369693
> Coverity issue: 369694
> Bugzilla ID: 668
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
For some reason git notes got lost on format-patch.
v3:
- Split refactor from bugfixes
v4:
- v3 was erroneously "fixing" handling of base max rather than base
frequency
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH v4 1/2] power: fix pstate base frequency handling
2021-04-02 9:35 ` Burakov, Anatoly
@ 2021-04-02 11:12 ` Pattan, Reshma
0 siblings, 0 replies; 19+ messages in thread
From: Pattan, Reshma @ 2021-04-02 11:12 UTC (permalink / raw)
To: Burakov, Anatoly, dev; +Cc: Hunt, David
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
> Sent: Friday, April 2, 2021 10:36 AM
> To: dev@dpdk.org
> Cc: Hunt, David <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 1/2] power: fix pstate base frequency
> handling
>
> On 02-Apr-21 10:26 AM, Anatoly Burakov wrote:
> > Previous fix for base frequency handling in pstate mode introduced a
> > couple of issues:
> >
> > - When base_frequency file does not exist, it simply bails out because
> > of what appears to be accidental addition of FOPEN_OR_ERR_RET. This is
> > incorrect, as absence of this file is not fatal and is in fact
> > expected on kernel versions earlier than 5.3
> > - When base_frequency file does exist, it gets opened, but never gets
> > closed, resulting in a resource leak
> >
> > Both issues also manifest themselves as Coverity defects (dead code,
> > and a resource leak), so this fix addresses both.
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > ---
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 1/2] power: fix pstate base frequency handling Anatoly Burakov
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 " Anatoly Burakov
@ 2021-04-02 9:26 ` Anatoly Burakov
2021-04-02 9:34 ` Burakov, Anatoly
1 sibling, 1 reply; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-02 9:26 UTC (permalink / raw)
To: dev; +Cc: david.hunt
Currently, when we set the pstate governor to "performance", we check if
it is already set to this value, and if it is, we skip setting it.
However, we never save this value anywhere, so that next time we come
back and request the governor to be set to its original value, the
original value is empty.
Fix it by saving the original pstate governor first. While we're at it,
replace `strlcpy` with `rte_strscpy`.
Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
Cc: david.hunt@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index c4639e4b8a..1cb0e4d917 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -382,6 +382,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
/* 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) {
@@ -390,8 +393,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
"already performance\n", pi->lcore_id);
goto out;
}
- /* Save the original governor */
- strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* Write 'performance' to the governor */
val = fseek(f, 0, SEEK_SET);
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor
2021-04-02 9:26 ` [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor Anatoly Burakov
@ 2021-04-02 9:34 ` Burakov, Anatoly
2021-04-02 11:12 ` Pattan, Reshma
0 siblings, 1 reply; 19+ messages in thread
From: Burakov, Anatoly @ 2021-04-02 9:34 UTC (permalink / raw)
To: dev; +Cc: david.hunt
On 02-Apr-21 10:26 AM, Anatoly Burakov wrote:
> Currently, when we set the pstate governor to "performance", we check if
> it is already set to this value, and if it is, we skip setting it.
>
> However, we never save this value anywhere, so that next time we come
> back and request the governor to be set to its original value, the
> original value is empty.
>
> Fix it by saving the original pstate governor first. While we're at it,
> replace `strlcpy` with `rte_strscpy`.
>
> Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
> Cc: david.hunt@intel.com
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> lib/librte_power/power_pstate_cpufreq.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> index c4639e4b8a..1cb0e4d917 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
Apologies for wrong threading, i'm sending patches from a new machine so
it's been a while since i've run git commands manually...
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor
2021-04-02 9:34 ` Burakov, Anatoly
@ 2021-04-02 11:12 ` Pattan, Reshma
2021-04-06 8:38 ` Thomas Monjalon
0 siblings, 1 reply; 19+ messages in thread
From: Pattan, Reshma @ 2021-04-02 11:12 UTC (permalink / raw)
To: Burakov, Anatoly, dev; +Cc: Hunt, David
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
> Sent: Friday, April 2, 2021 10:34 AM
> To: dev@dpdk.org
> Cc: Hunt, David <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate
> governor
>
> On 02-Apr-21 10:26 AM, Anatoly Burakov wrote:
> > Currently, when we set the pstate governor to "performance", we check
> > if it is already set to this value, and if it is, we skip setting it.
> >
> > However, we never save this value anywhere, so that next time we come
> > back and request the governor to be set to its original value, the
> > original value is empty.
> >
> > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Reshma Pattan <reshma.pattan@intel.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [dpdk-dev] [PATCH v4 2/2] power: do not skip saving original pstate governor
2021-04-02 11:12 ` Pattan, Reshma
@ 2021-04-06 8:38 ` Thomas Monjalon
0 siblings, 0 replies; 19+ messages in thread
From: Thomas Monjalon @ 2021-04-06 8:38 UTC (permalink / raw)
To: Burakov, Anatoly; +Cc: dev, Hunt, David, Pattan, Reshma
02/04/2021 13:12, Pattan, Reshma:
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Burakov, Anatoly
> > On 02-Apr-21 10:26 AM, Anatoly Burakov wrote:
> > > Currently, when we set the pstate governor to "performance", we check
> > > if it is already set to this value, and if it is, we skip setting it.
> > >
> > > However, we never save this value anywhere, so that next time we come
> > > back and request the governor to be set to its original value, the
> > > original value is empty.
> > >
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>
> Acked-by: Reshma Pattan <reshma.pattan@intel.com>
Applied, thanks
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v3 2/2] power: do not skip saving original pstate governor
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
` (2 preceding siblings ...)
2021-04-01 15:06 ` [dpdk-dev] [PATCH v3 1/2] power: fix pstate base frequency handling Anatoly Burakov
@ 2021-04-01 15:06 ` Anatoly Burakov
3 siblings, 0 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-04-01 15:06 UTC (permalink / raw)
To: dev; +Cc: anatoly.burakov, david.hunt
Currently, when we set the pstate governor to "performance", we check if
it is already set to this value, and if it is, we skip setting it.
However, we never save this value anywhere, so that next time we come
back and request the governor to be set to its original value, the
original value is empty.
Fix it by saving the original pstate governor first. While we're at it,
replace `strlcpy` with `rte_strscpy`.
Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
Cc: david.hunt@intel.com
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index af5ad0b506..3a5face4f0 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -382,6 +382,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
/* 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) {
@@ -390,8 +393,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
"already performance\n", pi->lcore_id);
goto out;
}
- /* Save the original governor */
- strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* Write 'performance' to the governor */
val = fseek(f, 0, SEEK_SET);
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread
* [dpdk-dev] [PATCH v2 2/3] power: refactor pstate sysfs handling
2021-03-30 14:15 [dpdk-dev] [PATCH 1/3] power: refactor base frequency detection Anatoly Burakov
` (2 preceding siblings ...)
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 1/3] power: refactor base frequency detection Anatoly Burakov
@ 2021-03-30 14:25 ` Anatoly Burakov
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 3/3] power: do not skip saving original pstate governor Anatoly Burakov
4 siblings, 0 replies; 19+ messages in thread
From: Anatoly Burakov @ 2021-03-30 14:25 UTC (permalink / raw)
To: dev; +Cc: David Hunt
Make use of new sysfs handling functions in other pstate code.
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 177 +++++++++++-------------
1 file changed, 80 insertions(+), 97 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index add06720db..7ea1bf677a 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -198,6 +198,46 @@ read_core_sysfs_u32(FILE *f, uint32_t *val)
return 0;
}
+static 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 */
+ if (strlen(buf))
+ strtok(buf, "\n");
+
+ return 0;
+}
+
+static 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 fopen the sys file for the future setting the lcore frequency.
*/
@@ -399,22 +439,16 @@ set_freq_internal(struct pstate_power_info *pi, uint32_t idx)
static int
power_set_governor_performance(struct pstate_power_info *pi)
{
- FILE *f;
+ FILE *f_governor = NULL;
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);
+ open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+ &f_governor);
+ FOPEN_OR_ERR_GOTO(f_governor, out);
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
- /* Strip off terminating '\n' */
- strtok(buf, "\n");
+ ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+ FOPS_OR_ERR_GOTO(ret, out);
/* Check if current governor is performance */
if (strncmp(buf, POWER_GOVERNOR_PERF,
@@ -428,21 +462,15 @@ power_set_governor_performance(struct pstate_power_info *pi)
strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* 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 = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
+ FOPS_OR_ERR_GOTO(ret, 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);
+ if (f_governor != NULL)
+ fclose(f_governor);
return ret;
}
@@ -454,20 +482,16 @@ power_set_governor_performance(struct pstate_power_info *pi)
static int
power_set_governor_original(struct pstate_power_info *pi)
{
- FILE *f;
+ FILE *f_governor = NULL;
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);
+ open_core_sysfs_file(POWER_SYSFILE_GOVERNOR, pi->lcore_id, "rw+",
+ &f_governor);
+ FOPEN_OR_ERR_GOTO(f_governor, out);
- s = fgets(buf, sizeof(buf), f);
- FOPS_OR_NULL_GOTO(s, out);
+ ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
+ FOPS_OR_ERR_GOTO(ret, out);
/* Check if the governor to be set is the same as current */
if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -479,19 +503,16 @@ power_set_governor_original(struct pstate_power_info *pi)
}
/* 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 = write_core_sysfs_s(f_governor, pi->governor_ori);
+ FOPS_OR_ERR_GOTO(ret, 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);
-
+ if (f_governor != NULL)
+ fclose(f_governor);
return ret;
}
@@ -502,51 +523,26 @@ 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);
+ /* open all files */
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MAX_FREQ, pi->lcore_id, "r",
+ &f_max);
+ FOPEN_OR_ERR_GOTO(f_max, out);
- f_min = fopen(fullpath_min, "r");
- FOPEN_OR_ERR_RET(f_min, ret);
+ open_core_sysfs_file(POWER_SYSFILE_BASE_MIN_FREQ, pi->lcore_id, "r",
+ &f_min);
+ FOPEN_OR_ERR_GOTO(f_max, out);
- f_max = fopen(fullpath_max, "r");
- if (f_max == NULL)
- fclose(f_min);
+ /* read base ratios */
+ ret = read_core_sysfs_u32(f_max, &sys_max_freq);
+ FOPS_OR_ERR_GOTO(ret, out);
- 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);
-
-
- /* 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);
+ ret = read_core_sysfs_u32(f_min, &sys_min_freq);
+ FOPS_OR_ERR_GOTO(ret, out);
if (sys_max_freq < sys_min_freq)
goto out;
@@ -605,27 +601,14 @@ 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);
+ open_core_sysfs_file(POWER_SYSFILE_CUR_FREQ, pi->lcore_id, "r", &f_cur);
+ FOPEN_OR_ERR_GOTO(f_cur, fail);
- /* 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);
-
- 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);
+ FOPS_OR_ERR_GOTO(ret, fail);
/* convert the frequency to nearest 100000 value
* Ex: if sys_cur_freq=1396789 then freq_conv=1400000
@@ -644,10 +627,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.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* [dpdk-dev] [PATCH v2 3/3] power: do not skip saving original pstate governor
2021-03-30 14:15 [dpdk-dev] [PATCH 1/3] power: refactor base frequency detection Anatoly Burakov
` (3 preceding siblings ...)
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 2/3] power: refactor pstate sysfs handling Anatoly Burakov
@ 2021-03-30 14:25 ` Anatoly Burakov
2021-03-30 16:51 ` Burakov, Anatoly
4 siblings, 1 reply; 19+ messages in thread
From: Anatoly Burakov @ 2021-03-30 14:25 UTC (permalink / raw)
To: dev; +Cc: David Hunt
Currently, when we set the pstate governor to "performance", we check if
it is already set to this value, and if it is, we skip setting it.
However, we never save this value anywhere, so that next time we come
back and request the governor to be set to its original value, the
original value is empty.
Fix it by saving the original pstate governor first. While we're at it,
replace `strlcpy` with `rte_strscpy`.
Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
lib/librte_power/power_pstate_cpufreq.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
index 7ea1bf677a..db7856dadc 100644
--- a/lib/librte_power/power_pstate_cpufreq.c
+++ b/lib/librte_power/power_pstate_cpufreq.c
@@ -450,6 +450,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
FOPS_OR_ERR_GOTO(ret, out);
+ /* 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) {
@@ -458,8 +461,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
"already performance\n", pi->lcore_id);
goto out;
}
- /* Save the original governor */
- strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
/* Write 'performance' to the governor */
ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
--
2.25.1
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [dpdk-dev] [PATCH v2 3/3] power: do not skip saving original pstate governor
2021-03-30 14:25 ` [dpdk-dev] [PATCH v2 3/3] power: do not skip saving original pstate governor Anatoly Burakov
@ 2021-03-30 16:51 ` Burakov, Anatoly
0 siblings, 0 replies; 19+ messages in thread
From: Burakov, Anatoly @ 2021-03-30 16:51 UTC (permalink / raw)
To: dev; +Cc: David Hunt
On 30-Mar-21 3:25 PM, Anatoly Burakov wrote:
> Currently, when we set the pstate governor to "performance", we check if
> it is already set to this value, and if it is, we skip setting it.
>
> However, we never save this value anywhere, so that next time we come
> back and request the governor to be set to its original value, the
> original value is empty.
>
> Fix it by saving the original pstate governor first. While we're at it,
> replace `strlcpy` with `rte_strscpy`.
>
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> lib/librte_power/power_pstate_cpufreq.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_power/power_pstate_cpufreq.c b/lib/librte_power/power_pstate_cpufreq.c
> index 7ea1bf677a..db7856dadc 100644
> --- a/lib/librte_power/power_pstate_cpufreq.c
> +++ b/lib/librte_power/power_pstate_cpufreq.c
> @@ -450,6 +450,9 @@ power_set_governor_performance(struct pstate_power_info *pi)
> ret = read_core_sysfs_s(f_governor, buf, sizeof(buf));
> FOPS_OR_ERR_GOTO(ret, out);
>
> + /* 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) {
> @@ -458,8 +461,6 @@ power_set_governor_performance(struct pstate_power_info *pi)
> "already performance\n", pi->lcore_id);
> goto out;
> }
> - /* Save the original governor */
> - strlcpy(pi->governor_ori, buf, sizeof(pi->governor_ori));
>
> /* Write 'performance' to the governor */
> ret = write_core_sysfs_s(f_governor, POWER_GOVERNOR_PERF);
>
Fixes: e6c6dc0f96c8 ("power: add p-state driver compatibility")
--
Thanks,
Anatoly
^ permalink raw reply [flat|nested] 19+ messages in thread