[v5,2/2] power: refactor pstate and acpi code

Message ID 20210622125853.2798-2-david.hunt@intel.com (mailing list archive)
State Superseded, archived
Headers
Series [v5,1/2] power: don't use rte prefix in internal code |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot success github build: passed
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/intel-Testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Functional fail Functional Testing issues

Commit Message

Hunt, David June 22, 2021, 12:58 p.m. UTC
  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 v2
* 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.
---
 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

Hunt, David June 22, 2021, 1:27 p.m. UTC | #1
Adding people to the CC list that were on v4 of this patch set, and 
Richael who raised some issues in v4.

On 22/6/2021 1:58 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 v2 (should read 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.


So in the process of posting v5, I picked the email id from v4 in 
patchwork, used that in my --in-reply-to, and somehow it screwed up the 
threading as it looks like I'm responding to v3. So I'm sending this 
email to make sure all the people CC'd in v4 are included in this (v5).

Anatoly is busy at the moment, so I'm addressing the issues raised in 
v4, and additionally adressing the checkpatch issues where it does not 
like the macros with control statements, so removing those, as I don't 
like them either.

Regards,
Dave.
  
Richael Zhuang June 23, 2021, 8:54 a.m. UTC | #2
Hi,
 There is a bug in lib/power/power_common.c:
+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;
Here, I mentioned in the V4 patch:  ret >=0 if success, EOF means failure.  It seems you forgot to fix this.
+
+	/* flush the output */
+	ret = fflush(f);
+	if (ret != 0)
+		return -1;
+
+	return 0;
+}

Best regards,
Richael
> -----Original Message-----
> From: David Hunt <david.hunt@intel.com>
> Sent: Tuesday, June 22, 2021 9:28 PM
> To: dev@dpdk.org
> Cc: anatoly.burakov@intel.com; stephen@networkplumber.org; Richael
> Zhuang <Richael.Zhuang@arm.com>; Reshma Pattan
> <reshma.pattan@intel.com>; nd <nd@arm.com>
> Subject: Re: [PATCH v5 2/2] power: refactor pstate and acpi code
> 
> Adding people to the CC list that were on v4 of this patch set, and Richael
> who raised some issues in v4.
> 
> On 22/6/2021 1:58 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 v2 (should read 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.
> 
> 
> So in the process of posting v5, I picked the email id from v4 in patchwork,
> used that in my --in-reply-to, and somehow it screwed up the threading as it
> looks like I'm responding to v3. So I'm sending this email to make sure all the
> people CC'd in v4 are included in this (v5).
> 
> Anatoly is busy at the moment, so I'm addressing the issues raised in v4, and
> additionally adressing the checkpatch issues where it does not like the
> macros with control statements, so removing those, as I don't like them
> either.
> 
> Regards,
> Dave.
> 
> 
>
  
Hunt, David June 23, 2021, 9 a.m. UTC | #3
On 23/6/2021 9:54 AM, Richael Zhuang wrote:
> Hi,
>   There is a bug in lib/power/power_common.c:
> +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;
> Here, I mentioned in the V4 patch:  ret >=0 if success, EOF means failure.  It seems you forgot to fix this.


Ah, OK. Will fix in v6. Hopefully I'll reply to the correct email-id 
this time. :)


> +
> +	/* flush the output */
> +	ret = fflush(f);
> +	if (ret != 0)
> +		return -1;
> +
> +	return 0;
> +}
>
> Best regards,
> Richael
>> -----Original Message-----
>> From: David Hunt <david.hunt@intel.com>
>> Sent: Tuesday, June 22, 2021 9:28 PM
>> To: dev@dpdk.org
>> Cc: anatoly.burakov@intel.com; stephen@networkplumber.org; Richael
>> Zhuang <Richael.Zhuang@arm.com>; Reshma Pattan
>> <reshma.pattan@intel.com>; nd <nd@arm.com>
>> Subject: Re: [PATCH v5 2/2] power: refactor pstate and acpi code
>>
>> Adding people to the CC list that were on v4 of this patch set, and Richael
>> who raised some issues in v4.
>>
>> On 22/6/2021 1:58 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 v2 (should read 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.
>>
>> So in the process of posting v5, I picked the email id from v4 in patchwork,
>> used that in my --in-reply-to, and somehow it screwed up the threading as it
>> looks like I'm responding to v3. So I'm sending this email to make sure all the
>> people CC'd in v4 are included in this (v5).
>>
>> Anatoly is busy at the moment, so I'm addressing the issues raised in v4, and
>> additionally adressing the checkpatch issues where it does not like the
>> macros with control statements, so removing those, as I don't like them
>> either.
>>
>> Regards,
>> Dave.
>>
>>
>>
  

Patch

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_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)
 {
diff --git a/lib/power/power_common.c b/lib/power/power_common.c
index 67e3318ec7..0295b89f10 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;
 }