[dpdk-dev] [PATCH 1/2] power: switching to unbuffered stdio for /sys file access

[dpdk-dev] [PATCH 1/2] power: switching to unbuffered stdio for /sys file access

[Radoslaw Biernacki]

This patch fixes the bug caused by improper use of buffered
stdio file access for switching the CPU frequency and
governor. Each write operation when using buffered stdio
must be flushed out and the return code from fflush() must
be verified. In buffered mode, write() syscall return value
is is not returned by fwrite()/fputs()/fprintf().
Since with buffered approatch, fflush() need to be done
every time it is better to use unbuffered mode or not use
stdio at all (instead use plain open/write functions). To
minimize amount of changes this fix use first approach.

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
---
 lib/librte_power/rte_power_acpi_cpufreq.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index 01ac5ac..8bf5685 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -143,12 +143,13 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
 				"for setting frequency for lcore %u\n", pi->lcore_id);
 		return -1;
 	}
+	/* we use unbuffered mode so following will fail if kernel will refuse
+	 * freq setting */
 	if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) {
 		RTE_LOG(ERR, POWER, "Fail to write new frequency for "
 				"lcore %u\n", pi->lcore_id);
 		return -1;
 	}
-	fflush(pi->f);
 	pi->curr_idx = idx;
 
 	return 1;
@@ -174,6 +175,11 @@ power_set_governor_userspace(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
+	if (setvbuf(f, NULL, _IONBF, 0)) {
+		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
+		goto out;
+	}
+
 	s = fgets(buf, sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
@@ -292,6 +298,11 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, -1);
 
+	if (setvbuf(f, NULL, _IONBF, 0)) {
+		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
+		goto out;
+	}
+
 	s = fgets(buf, sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
@@ -389,6 +400,11 @@ power_set_governor_original(struct rte_power_info *pi)
 	f = fopen(fullpath, "rw+");
 	FOPEN_OR_ERR_RET(f, ret);
 
+	if (setvbuf(f, NULL, _IONBF, 0)) {
+		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
+		goto out;
+	}
+
 	s = fgets(buf, sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
 
-- 
2.7.4

[dpdk-dev] [PATCH 2/2] power: check if userspace governor is available

[Radoslaw Biernacki]

Since for new Intel CPU's kernel use intel_pstate driver,
which does not offer userspace governor, it is vise to check
the userspace governor availability before trying to perform
governor switch. The outcome from this patch is direct
information for user about the root cause of failure during
the rte_power_acpi_cpufreq_init(). This patch also add the
/sys file name to error message as on some platforms some
files expected by this rte_power are not available. This is
also useful for pinning down the root cause of the problem.

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
---
 lib/librte_power/rte_power_acpi_cpufreq.c | 35 ++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 8 deletions(-)

diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index 8bf5685..342eb22 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -55,9 +55,9 @@
 #define POWER_DEBUG_TRACE(fmt, args...)
 #endif
 
-#define FOPEN_OR_ERR_RET(f, retval) do { \
+#define FOPEN_OR_ERR_RET(f, fullpath, retval) do { \
 		if ((f) == NULL) { \
-			RTE_LOG(ERR, POWER, "File not openned\n"); \
+			RTE_LOG(ERR, POWER, "File %s not openned\n", fullpath); \
 			return retval; \
 		} \
 } while (0)
@@ -80,6 +80,8 @@
 #define POWER_CONVERT_TO_DECIMAL 10
 
 #define POWER_GOVERNOR_USERSPACE "userspace"
+#define POWER_SYSFILE_AVAIL_GOVERNOR   \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_governors"
 #define POWER_SYSFILE_GOVERNOR   \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
 #define POWER_SYSFILE_AVAIL_FREQ \
@@ -170,10 +172,28 @@ power_set_governor_userspace(struct rte_power_info *pi)
 	char *s;
 	int val;
 
+	/* check if userspace governor is available */
+
+	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_GOVERNOR,
+			pi->lcore_id);
+	f = fopen(fullpath, "r");
+	FOPEN_OR_ERR_RET(f, fullpath, ret);
+
+	s = fgets(buf, sizeof(buf), f);
+	FOPS_OR_NULL_GOTO(s, out);
+
+	if (!strstr(buf, POWER_GOVERNOR_USERSPACE)) {
+		RTE_LOG(ERR, POWER, "Userspace governor is not available\n");
+		goto out;
+	}
+	fclose(f);
+
+	/* store current governor and set userspace governor */
+
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	FOPEN_OR_ERR_RET(f, fullpath, ret);
 
 	if (setvbuf(f, NULL, _IONBF, 0)) {
 		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
@@ -184,8 +204,7 @@ power_set_governor_userspace(struct rte_power_info *pi)
 	FOPS_OR_NULL_GOTO(s, out);
 
 	/* Check if current governor is userspace */
-	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
-			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
+	if (!strcmp(buf, POWER_GOVERNOR_USERSPACE)) {
 		ret = 0;
 		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
 				"already userspace\n", pi->lcore_id);
@@ -228,7 +247,7 @@ power_get_available_freqs(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
 			pi->lcore_id);
 	f = fopen(fullpath, "r");
-	FOPEN_OR_ERR_RET(f, ret);
+	FOPEN_OR_ERR_RET(f, fullpath, ret);
 
 	s = fgets(buf, sizeof(buf), f);
 	FOPS_OR_NULL_GOTO(s, out);
@@ -296,7 +315,7 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, -1);
+	FOPEN_OR_ERR_RET(f, fullpath, -1);
 
 	if (setvbuf(f, NULL, _IONBF, 0)) {
 		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
@@ -398,7 +417,7 @@ power_set_governor_original(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	FOPEN_OR_ERR_RET(f, fullpath, ret);
 
 	if (setvbuf(f, NULL, _IONBF, 0)) {
 		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
-- 
2.7.4

Re: [dpdk-dev] [PATCH 1/2] power: switching to unbuffered stdio for /sys file access

[Hunt, David]

Hi Radoslaw,

Thanks for the patch. Some comments below.


On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote:
> This patch fixes the bug caused by improper use of buffered
> stdio file access for switching the CPU frequency and
> governor. Each write operation when using buffered stdio
> must be flushed out and the return code from fflush() must
> be verified. In buffered mode, write() syscall return value
> is is not returned by fwrite()/fputs()/fprintf().
> Since with buffered approatch, fflush() need to be done
> every time it is better to use unbuffered mode or not use
> stdio at all (instead use plain open/write functions). To
> minimize amount of changes this fix use first approach.
>
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> ---
>   lib/librte_power/rte_power_acpi_cpufreq.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
> index 01ac5ac..8bf5685 100644
> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
> @@ -143,12 +143,13 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>   				"for setting frequency for lcore %u\n", pi->lcore_id);
>   		return -1;
>   	}
> +	/* we use unbuffered mode so following will fail if kernel will refuse
> +	 * freq setting */
>   	if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) {
>   		RTE_LOG(ERR, POWER, "Fail to write new frequency for "
>   				"lcore %u\n", pi->lcore_id);
>   		return -1;
>   	}
> -	fflush(pi->f);
>   	pi->curr_idx = idx;
>   

One suggestion to to avoid switching to unbuffered mode would be to move 
the check from the fprintf to the fflush. That would eliminate the need 
for the changes below, keeping the changes to a minimum.
  But I also agree that open/write would be a better option, should you 
prefer to go that route.

>   	return 1;
> @@ -174,6 +175,11 @@ power_set_governor_userspace(struct rte_power_info *pi)
>   	f = fopen(fullpath, "rw+");
>   	FOPEN_OR_ERR_RET(f, ret);
>   
> +	if (setvbuf(f, NULL, _IONBF, 0)) {
> +		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
> +		goto out;
> +	}
> +
>   	s = fgets(buf, sizeof(buf), f);
>   	FOPS_OR_NULL_GOTO(s, out);
>   
> @@ -292,6 +298,11 @@ power_init_for_setting_freq(struct rte_power_info *pi)
>   	f = fopen(fullpath, "rw+");
>   	FOPEN_OR_ERR_RET(f, -1);
>   
> +	if (setvbuf(f, NULL, _IONBF, 0)) {
> +		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
> +		goto out;
> +	}
> +
>   	s = fgets(buf, sizeof(buf), f);
>   	FOPS_OR_NULL_GOTO(s, out);
>   
> @@ -389,6 +400,11 @@ power_set_governor_original(struct rte_power_info *pi)
>   	f = fopen(fullpath, "rw+");
>   	FOPEN_OR_ERR_RET(f, ret);
>   
> +	if (setvbuf(f, NULL, _IONBF, 0)) {
> +		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
> +		goto out;
> +	}
> +
>   	s = fgets(buf, sizeof(buf), f);
>   	FOPS_OR_NULL_GOTO(s, out);
>   

Regards,
Dave

Re: [dpdk-dev] [PATCH 1/2] power: switching to unbuffered stdio for /sys file access

[Hunt, David]

Hi Radoslaw,


On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote:
> This patch fixes the bug caused by improper use of buffered
> stdio file access for switching the CPU frequency and
> governor. Each write operation when using buffered stdio
> must be flushed out and the return code from fflush() must
> be verified. In buffered mode, write() syscall return value
> is is not returned by fwrite()/fputs()/fprintf().
> Since with buffered approatch, fflush() need to be done
> every time it is better to use unbuffered mode or not use
> stdio at all (instead use plain open/write functions). To
> minimize amount of changes this fix use first approach.
>
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> ---
>   lib/librte_power/rte_power_acpi_cpufreq.c | 18 +++++++++++++++++-
>   1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
> index 01ac5ac..8bf5685 100644
> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
> @@ -143,12 +143,13 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>   				"for setting frequency for lcore %u\n", pi->lcore_id);
>   		return -1;
>   	}
> +	/* we use unbuffered mode so following will fail if kernel will refuse
> +	 * freq setting */

Also, there's an issue with checkpatch on the comment here. Please make 
sure to run your patches through checkpatch. Typically a recent version 
of checkpatch should be used (4.1x).

Rgds,
Dave.


---snip---

Re: [dpdk-dev] [PATCH 2/2] power: check if userspace governor is available

[Hunt, David]

Hi Radoslaw,


On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote:
> Since for new Intel CPU's kernel use intel_pstate driver,
> which does not offer userspace governor, it is vise to check

typo here, "wise"

> the userspace governor availability before trying to perform
> governor switch. The outcome from this patch is direct
> information for user about the root cause of failure during
> the rte_power_acpi_cpufreq_init(). This patch also add the
> /sys file name to error message as on some platforms some
> files expected by this rte_power are not available. This is
> also useful for pinning down the root cause of the problem.

Good suggestion. It's a useful change.

What would you think of also adding a check of the contents of the 
"scaling_driver" file? This would indicate the reason why the userspace 
governor is not available.
If it's "intel_pstate", then we could suggest to the user that 
"intel_pstate=disable" needs to be added to the kernel boot parameters.

> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> ---
>   lib/librte_power/rte_power_acpi_cpufreq.c | 35 ++++++++++++++++++++++++-------
>   1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
> index 8bf5685..342eb22 100644
> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
> @@ -55,9 +55,9 @@
>   #define POWER_DEBUG_TRACE(fmt, args...)
>   #endif
>   
> -#define FOPEN_OR_ERR_RET(f, retval) do { \
> +#define FOPEN_OR_ERR_RET(f, fullpath, retval) do { \
>   		if ((f) == NULL) { \
> -			RTE_LOG(ERR, POWER, "File not openned\n"); \
> +			RTE_LOG(ERR, POWER, "File %s not openned\n", fullpath); \
>   			return retval; \
>   		} \
>   } while (0)

There's a checkpatch error here. I've encountered this one a few times 
in the past, and I don't believe it can be fixed with the macro in it's 
current form. Later versions of checkpatch does not like control 
statements in macros, so the only way to fix this checkpatch problem is 
to remove the macro, and put the 'if' statement back into the code 
instead of calling the macro. As well as fixing the checkpatch issue, I 
feel this would have the added advantage of making the code more readable.

Also, I would suggest changing "File %s not openned\n" to "Failed to 
open %s.\n".

> @@ -80,6 +80,8 @@
>   #define POWER_CONVERT_TO_DECIMAL 10
>   
>   #define POWER_GOVERNOR_USERSPACE "userspace"
> +#define POWER_SYSFILE_AVAIL_GOVERNOR   \
> +		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_governors"
>   #define POWER_SYSFILE_GOVERNOR   \
>   		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>   #define POWER_SYSFILE_AVAIL_FREQ \
> @@ -170,10 +172,28 @@ power_set_governor_userspace(struct rte_power_info *pi)
>   	char *s;
>   	int val;
>   
> +	/* check if userspace governor is available */
> +
> +	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_GOVERNOR,
> +			pi->lcore_id);
> +	f = fopen(fullpath, "r");
> +	FOPEN_OR_ERR_RET(f, fullpath, ret);
> +
> +	s = fgets(buf, sizeof(buf), f);
> +	FOPS_OR_NULL_GOTO(s, out);
> +
> +	if (!strstr(buf, POWER_GOVERNOR_USERSPACE)) {
> +		RTE_LOG(ERR, POWER, "Userspace governor is not available\n");
> +		goto out;
> +	}
> +	fclose(f);
> +
> +	/* store current governor and set userspace governor */
> +
>   	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
>   			pi->lcore_id);
>   	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> +	FOPEN_OR_ERR_RET(f, fullpath, ret);
>   
>   	if (setvbuf(f, NULL, _IONBF, 0)) {
>   		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
> @@ -184,8 +204,7 @@ power_set_governor_userspace(struct rte_power_info *pi)
>   	FOPS_OR_NULL_GOTO(s, out);
>   
>   	/* Check if current governor is userspace */
> -	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
> -			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
> +	if (!strcmp(buf, POWER_GOVERNOR_USERSPACE)) {
>   		ret = 0;
>   		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
>   				"already userspace\n", pi->lcore_id);
> @@ -228,7 +247,7 @@ power_get_available_freqs(struct rte_power_info *pi)
>   	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
>   			pi->lcore_id);
>   	f = fopen(fullpath, "r");
> -	FOPEN_OR_ERR_RET(f, ret);
> +	FOPEN_OR_ERR_RET(f, fullpath, ret);
>   
>   	s = fgets(buf, sizeof(buf), f);
>   	FOPS_OR_NULL_GOTO(s, out);
> @@ -296,7 +315,7 @@ power_init_for_setting_freq(struct rte_power_info *pi)
>   	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>   			pi->lcore_id);
>   	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, -1);
> +	FOPEN_OR_ERR_RET(f, fullpath, -1);
>   
>   	if (setvbuf(f, NULL, _IONBF, 0)) {
>   		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
> @@ -398,7 +417,7 @@ power_set_governor_original(struct rte_power_info *pi)
>   	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
>   			pi->lcore_id);
>   	f = fopen(fullpath, "rw+");
> -	FOPEN_OR_ERR_RET(f, ret);
> +	FOPEN_OR_ERR_RET(f, fullpath, ret);
>   
>   	if (setvbuf(f, NULL, _IONBF, 0)) {
>   		RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");

Thanks,
Dave.

Re: [dpdk-dev] [PATCH 1/2] power: switching to unbuffered stdio for /sys file access

[Radoslaw Biernacki]

Hi David,

Thank you for comments.

On 18 October 2017 at 12:33, Hunt, David <david.hunt@intel.com> wrote:

> Hi Radoslaw,
>
>
> On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote:
>
>> This patch fixes the bug caused by improper use of buffered
>> stdio file access for switching the CPU frequency and
>> governor. Each write operation when using buffered stdio
>> must be flushed out and the return code from fflush() must
>> be verified. In buffered mode, write() syscall return value
>> is is not returned by fwrite()/fputs()/fprintf().
>> Since with buffered approatch, fflush() need to be done
>> every time it is better to use unbuffered mode or not use
>> stdio at all (instead use plain open/write functions). To
>> minimize amount of changes this fix use first approach.
>>
>> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
>> ---
>>   lib/librte_power/rte_power_acpi_cpufreq.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c
>> b/lib/librte_power/rte_power_acpi_cpufreq.c
>> index 01ac5ac..8bf5685 100644
>> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
>> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
>> @@ -143,12 +143,13 @@ set_freq_internal(struct rte_power_info *pi,
>> uint32_t idx)
>>                                 "for setting frequency for lcore %u\n",
>> pi->lcore_id);
>>                 return -1;
>>         }
>> +       /* we use unbuffered mode so following will fail if kernel will
>> refuse
>> +        * freq setting */
>>
>
> Also, there's an issue with checkpatch on the comment here. Please make
> sure to run your patches through checkpatch. Typically a recent version of
> checkpatch should be used (4.1x).
>

I apologise for missing checkpatch, seems that problems with my environment
settings cause the silent output.
Will never happen again.

---snip---

For the option of using open/write: yes, as you agreed with that, let me
prepare V2 of those fixes.

Re: [dpdk-dev] [PATCH 2/2] power: check if userspace governor is available

[Radoslaw Biernacki]

Hi David,

On 18 October 2017 at 12:53, Hunt, David <david.hunt@intel.com> wrote:

> Hi Radoslaw,
>
>
> On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote:
>
>> Since for new Intel CPU's kernel use intel_pstate driver,
>> which does not offer userspace governor, it is vise to check
>>
>
> typo here, "wise"
>
> the userspace governor availability before trying to perform
>> governor switch. The outcome from this patch is direct
>> information for user about the root cause of failure during
>> the rte_power_acpi_cpufreq_init(). This patch also add the
>> /sys file name to error message as on some platforms some
>> files expected by this rte_power are not available. This is
>> also useful for pinning down the root cause of the problem.
>>
>
> Good suggestion. It's a useful change.
>
> What would you think of also adding a check of the contents of the
> "scaling_driver" file? This would indicate the reason why the userspace
> governor is not available.
> If it's "intel_pstate", then we could suggest to the user that
> "intel_pstate=disable" needs to be added to the kernel boot parameters.


Yes, will do. This will save time of DPDK users trying to figure out the
problem.


>
>
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
>> ---
>>   lib/librte_power/rte_power_acpi_cpufreq.c | 35
>> ++++++++++++++++++++++++-------
>>   1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c
>> b/lib/librte_power/rte_power_acpi_cpufreq.c
>> index 8bf5685..342eb22 100644
>> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
>> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
>> @@ -55,9 +55,9 @@
>>   #define POWER_DEBUG_TRACE(fmt, args...)
>>   #endif
>>   -#define FOPEN_OR_ERR_RET(f, retval) do { \
>> +#define FOPEN_OR_ERR_RET(f, fullpath, retval) do { \
>>                 if ((f) == NULL) { \
>> -                       RTE_LOG(ERR, POWER, "File not openned\n"); \
>> +                       RTE_LOG(ERR, POWER, "File %s not openned\n",
>> fullpath); \
>>                         return retval; \
>>                 } \
>>   } while (0)
>>
>
> There's a checkpatch error here. I've encountered this one a few times in
> the past, and I don't believe it can be fixed with the macro in it's
> current form. Later versions of checkpatch does not like control statements
> in macros, so the only way to fix this checkpatch problem is to remove the
> macro, and put the 'if' statement back into the code instead of calling the
> macro. As well as fixing the checkpatch issue, I feel this would have the
> added advantage of making the code more readable.
>
> Also, I would suggest changing "File %s not openned\n" to "Failed to open
> %s.\n".


Sure, will fix those as well in V2


>
>
> @@ -80,6 +80,8 @@
>>   #define POWER_CONVERT_TO_DECIMAL 10
>>     #define POWER_GOVERNOR_USERSPACE "userspace"
>> +#define POWER_SYSFILE_AVAIL_GOVERNOR   \
>> +               "/sys/devices/system/cpu/cpu%
>> u/cpufreq/scaling_available_governors"
>>   #define POWER_SYSFILE_GOVERNOR   \
>>                 "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
>>   #define POWER_SYSFILE_AVAIL_FREQ \
>> @@ -170,10 +172,28 @@ power_set_governor_userspace(struct rte_power_info
>> *pi)
>>         char *s;
>>         int val;
>>   +     /* check if userspace governor is available */
>> +
>> +       snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_GOVERNOR,
>> +                       pi->lcore_id);
>> +       f = fopen(fullpath, "r");
>> +       FOPEN_OR_ERR_RET(f, fullpath, ret);
>> +
>> +       s = fgets(buf, sizeof(buf), f);
>> +       FOPS_OR_NULL_GOTO(s, out);
>> +
>> +       if (!strstr(buf, POWER_GOVERNOR_USERSPACE)) {
>> +               RTE_LOG(ERR, POWER, "Userspace governor is not
>> available\n");
>> +               goto out;
>> +       }
>> +       fclose(f);
>> +
>> +       /* store current governor and set userspace governor */
>> +
>>         snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
>>                         pi->lcore_id);
>>         f = fopen(fullpath, "rw+");
>> -       FOPEN_OR_ERR_RET(f, ret);
>> +       FOPEN_OR_ERR_RET(f, fullpath, ret);
>>         if (setvbuf(f, NULL, _IONBF, 0)) {
>>                 RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
>> @@ -184,8 +204,7 @@ power_set_governor_userspace(struct rte_power_info
>> *pi)
>>         FOPS_OR_NULL_GOTO(s, out);
>>         /* Check if current governor is userspace */
>> -       if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
>> -                       sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
>> +       if (!strcmp(buf, POWER_GOVERNOR_USERSPACE)) {
>>                 ret = 0;
>>                 POWER_DEBUG_TRACE("Power management governor of lcore %u
>> is "
>>                                 "already userspace\n", pi->lcore_id);
>> @@ -228,7 +247,7 @@ power_get_available_freqs(struct rte_power_info *pi)
>>         snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
>>                         pi->lcore_id);
>>         f = fopen(fullpath, "r");
>> -       FOPEN_OR_ERR_RET(f, ret);
>> +       FOPEN_OR_ERR_RET(f, fullpath, ret);
>>         s = fgets(buf, sizeof(buf), f);
>>         FOPS_OR_NULL_GOTO(s, out);
>> @@ -296,7 +315,7 @@ power_init_for_setting_freq(struct rte_power_info
>> *pi)
>>         snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>>                         pi->lcore_id);
>>         f = fopen(fullpath, "rw+");
>> -       FOPEN_OR_ERR_RET(f, -1);
>> +       FOPEN_OR_ERR_RET(f, fullpath, -1);
>>         if (setvbuf(f, NULL, _IONBF, 0)) {
>>                 RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
>> @@ -398,7 +417,7 @@ power_set_governor_original(struct rte_power_info
>> *pi)
>>         snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
>>                         pi->lcore_id);
>>         f = fopen(fullpath, "rw+");
>> -       FOPEN_OR_ERR_RET(f, ret);
>> +       FOPEN_OR_ERR_RET(f, fullpath, ret);
>>         if (setvbuf(f, NULL, _IONBF, 0)) {
>>                 RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n");
>>
>
> Thanks,
> Dave.
>

[dpdk-dev] [PATCH 0/3] power: fixes for power ACPI through sysfs

[Radoslaw Biernacki]

This series of patches is fixing bug in power ACPI subsystem code as
well as improve the debugging messages and cleans the code.

V2:
 - error checking macros removed (checkpatch warning)
 - using plain open()/read()/write() system functions instead of STDIO
 - added check for intel_pstate driver together with suggestion about
   needed kernel boot parameter

Radoslaw Biernacki (3):
  power: removing code macros
  power: switching to unbuffered access for /sys files
  power: check if userspace governor is available

 lib/librte_power/rte_power_acpi_cpufreq.c | 304 ++++++++++++++++++------------
 1 file changed, 182 insertions(+), 122 deletions(-)

-- 
2.7.4

[dpdk-dev] [PATCH 1/3] power: removing code macros

[Radoslaw Biernacki]

This fix implements David Hunt suggestion to remove error checking
code macros.  Together with improved code readability it removes
checkpatch warnings.

Fixes: 445c6528b55f ("power: common interface for guest and host")
CC: stable@dpdk.org

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
---
 lib/librte_power/rte_power_acpi_cpufreq.c | 81 ++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 33 deletions(-)

diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index 01ac5ac..3d0872f 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -55,27 +55,6 @@
 #define POWER_DEBUG_TRACE(fmt, args...)
 #endif
 
-#define FOPEN_OR_ERR_RET(f, retval) do { \
-		if ((f) == NULL) { \
-			RTE_LOG(ERR, POWER, "File not openned\n"); \
-			return retval; \
-		} \
-} while (0)
-
-#define FOPS_OR_NULL_GOTO(ret, label) do { \
-		if ((ret) == NULL) { \
-			RTE_LOG(ERR, POWER, "fgets returns nothing\n"); \
-			goto label; \
-		} \
-} while (0)
-
-#define FOPS_OR_ERR_GOTO(ret, label) do { \
-		if ((ret) < 0) { \
-			RTE_LOG(ERR, POWER, "File operations failed\n"); \
-			goto label; \
-		} \
-} while (0)
-
 #define STR_SIZE     1024
 #define POWER_CONVERT_TO_DECIMAL 10
 
@@ -172,10 +151,16 @@ power_set_governor_userspace(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	if (!f) {
+		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
+		return ret;
+	}
 
 	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	if (!s) {
+		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+		goto out;
+	}
 
 	/* Check if current governor is userspace */
 	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
@@ -190,10 +175,16 @@ power_set_governor_userspace(struct rte_power_info *pi)
 
 	/* Write 'userspace' to the governor */
 	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
+	if (val < 0) {
+		RTE_LOG(ERR, POWER, "fseek failed\n");
+		goto out;
+	}
 
 	val = fputs(POWER_GOVERNOR_USERSPACE, f);
-	FOPS_OR_ERR_GOTO(val, out);
+	if (val < 0) {
+		RTE_LOG(ERR, POWER, "fputs failed\n");
+		goto out;
+	}
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
@@ -222,10 +213,16 @@ power_get_available_freqs(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ,
 			pi->lcore_id);
 	f = fopen(fullpath, "r");
-	FOPEN_OR_ERR_RET(f, ret);
+	if (!f) {
+		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
+		return ret;
+	}
 
 	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	if (!s) {
+		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+		goto out;
+	}
 
 	/* Strip the line break if there is */
 	p = strchr(buf, '\n');
@@ -290,10 +287,16 @@ power_init_for_setting_freq(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, -1);
+	if (!f) {
+		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
+		return -1;
+	}
 
 	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	if (!s) {
+		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+		goto out;
+	}
 
 	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
 	for (i = 0; i < pi->nb_freqs; i++) {
@@ -387,10 +390,16 @@ power_set_governor_original(struct rte_power_info *pi)
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
 			pi->lcore_id);
 	f = fopen(fullpath, "rw+");
-	FOPEN_OR_ERR_RET(f, ret);
+	if (!f) {
+		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
+		return ret;
+	}
 
 	s = fgets(buf, sizeof(buf), f);
-	FOPS_OR_NULL_GOTO(s, out);
+	if (!s) {
+		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+		goto out;
+	}
 
 	/* Check if the governor to be set is the same as current */
 	if (strncmp(buf, pi->governor_ori, sizeof(pi->governor_ori)) == 0) {
@@ -403,10 +412,16 @@ power_set_governor_original(struct rte_power_info *pi)
 
 	/* Write back the original governor */
 	val = fseek(f, 0, SEEK_SET);
-	FOPS_OR_ERR_GOTO(val, out);
+	if (val < 0) {
+		RTE_LOG(ERR, POWER, "fseek failed\n");
+		goto out;
+	}
 
 	val = fputs(pi->governor_ori, f);
-	FOPS_OR_ERR_GOTO(val, out);
+	if (val < 0) {
+		RTE_LOG(ERR, POWER, "fputs failed\n");
+		goto out;
+	}
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u "
-- 
2.7.4

[dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files

[Radoslaw Biernacki]

This patch fixes the bug caused by improper use of buffered stdio file
access for switching the CPU frequency and governor. When using
buffered stdio, each fwrite() must use fflush() and the return code
must be verified. Also fseek() is needed.  Therefore it is better to
use unbuffered mode or use plain open()/write() functions.  This fix
use second approach. This not only remove need for ffush() but also
remove need for fseek() operations.  This patch also reuse some code
around power_set_governor_userspace() and
power_set_governor_userspace() functions.

Fixes: 445c6528b55f ("power: common interface for guest and host")
CC: stable@dpdk.org

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
---
 lib/librte_power/rte_power_acpi_cpufreq.c | 211 +++++++++++++-----------------
 1 file changed, 91 insertions(+), 120 deletions(-)

diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index 3d0872f..f811bd3 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -30,7 +30,7 @@
  *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
  *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
-
+#include <assert.h>
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
@@ -47,6 +47,12 @@
 #include "rte_power_acpi_cpufreq.h"
 #include "rte_power_common.h"
 
+#define min(_x, _y) ({ \
+	typeof(_x) _min1 = (_x); \
+	typeof(_y) _min2 = (_y); \
+	(void) (&_min1 == &_min2); \
+	_min1 < _min2 ? _min1 : _min2; })
+
 #ifdef RTE_LIBRTE_POWER_DEBUG
 #define POWER_DEBUG_TRACE(fmt, args...) do { \
 		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
@@ -88,7 +94,7 @@ struct rte_power_info {
 	unsigned lcore_id;                   /**< Logical core id */
 	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
 	uint32_t nb_freqs;                   /**< number of available freqs */
-	FILE *f;                             /**< FD of scaling_setspeed */
+	int fd;                              /**< FD of scaling_setspeed */
 	char governor_ori[32];               /**< Original governor name */
 	uint32_t curr_idx;                   /**< Freq index in freqs array */
 	volatile uint32_t state;             /**< Power in use state */
@@ -105,6 +111,9 @@ static struct rte_power_info lcore_power_info[RTE_MAX_LCORE];
 static int
 set_freq_internal(struct rte_power_info *pi, uint32_t idx)
 {
+	char buf[BUFSIZ];
+	int count, ret;
+
 	if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) {
 		RTE_LOG(ERR, POWER, "Invalid frequency index %u, which "
 				"should be less than %u\n", idx, pi->nb_freqs);
@@ -117,17 +126,14 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
 
 	POWER_DEBUG_TRACE("Freqency[%u] %u to be set for lcore %u\n",
 			idx, pi->freqs[idx], pi->lcore_id);
-	if (fseek(pi->f, 0, SEEK_SET) < 0) {
-		RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
-				"for setting frequency for lcore %u\n", pi->lcore_id);
+	count = snprintf(buf, sizeof(buf), "%u", pi->freqs[idx]);
+	assert((size_t)count < sizeof(buf)-1);
+	ret = write(pi->fd, buf, count);
+	if (ret != count) {
+		RTE_LOG(ERR, POWER, "Fail to write new frequency (%s) for "
+				"lcore %u\n", buf, pi->lcore_id);
 		return -1;
 	}
-	if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) {
-		RTE_LOG(ERR, POWER, "Fail to write new frequency for "
-				"lcore %u\n", pi->lcore_id);
-		return -1;
-	}
-	fflush(pi->f);
 	pi->curr_idx = idx;
 
 	return 1;
@@ -139,90 +145,109 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
  * governor will be saved for rolling back.
  */
 static int
-power_set_governor_userspace(struct rte_power_info *pi)
+power_set_governor(unsigned int lcore_id, const char *new_gov, char *old_gov,
+		   size_t old_gov_len)
 {
-	FILE *f;
+	int fd;
+	int count, len;
 	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+");
-	if (!f) {
+		 lcore_id);
+	fd = open(fullpath, O_RDWR);
+	if (fd < 0) {
 		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
 		return ret;
 	}
 
-	s = fgets(buf, sizeof(buf), f);
-	if (!s) {
-		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+	count = read(fd, buf, sizeof(buf));
+	if (count < 0) {
+		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
 		goto out;
 	}
+	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
 
-	/* Check if current governor is userspace */
-	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
-			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
+	/* Check if current governor is as requested */
+	if (!strcmp(buf, new_gov)) {
 		ret = 0;
 		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
-				"already userspace\n", pi->lcore_id);
-		goto out;
-	}
-	/* Save the original governor */
-	snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
-
-	/* Write 'userspace' to the governor */
-	val = fseek(f, 0, SEEK_SET);
-	if (val < 0) {
-		RTE_LOG(ERR, POWER, "fseek failed\n");
+				  "already %s\n", lcore_id, new_gov);
 		goto out;
 	}
+	/* Save the old governor */
+	if (old_gov)
+		snprintf(old_gov, old_gov_len, "%s", buf);
 
-	val = fputs(POWER_GOVERNOR_USERSPACE, f);
-	if (val < 0) {
-		RTE_LOG(ERR, POWER, "fputs failed\n");
+	/* Set new governor */
+	len = strlen(new_gov);
+	count = write(fd, new_gov, len);
+	if (count != len) {
+		RTE_LOG(ERR, POWER, "Failed to set %s governor\n", new_gov);
 		goto out;
 	}
 
 	ret = 0;
 	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
-			"set to user space successfully\n", pi->lcore_id);
+		"set to user space successfully\n", lcore_id);
 out:
-	fclose(f);
+	if (close(fd))
+		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
 
 	return ret;
 }
 
 /**
+ * It is to check the current scaling governor by reading sys file, and then
+ * set it into 'userspace' if it is not by writing the sys file. The original
+ * governor will be saved for rolling back.
+ */
+static int
+power_set_governor_userspace(struct rte_power_info *pi)
+{
+	return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE,
+				  pi->governor_ori, sizeof(pi->governor_ori));
+}
+
+/**
+ * It is to check the governor and then set the original governor back if
+ * needed by writing the the sys file.
+ */
+static int
+power_set_governor_original(struct rte_power_info *pi)
+{
+	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
+}
+
+/**
  * It is to get the available frequencies of the specific lcore by reading the
  * sys file.
  */
 static int
 power_get_available_freqs(struct rte_power_info *pi)
 {
-	FILE *f;
+	int fd;
 	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");
-	if (!f) {
+		 pi->lcore_id);
+	fd = open(fullpath, O_RDONLY);
+	if (fd < 0) {
 		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
 		return ret;
 	}
 
-	s = fgets(buf, sizeof(buf), f);
-	if (!s) {
-		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+	count = read(fd, buf, sizeof(buf));
+	if (count < 0) {
+		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
 		goto out;
 	}
+	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
 
 	/* Strip the line break if there is */
 	p = strchr(buf, '\n');
@@ -267,7 +292,8 @@ power_get_available_freqs(struct rte_power_info *pi)
 	POWER_DEBUG_TRACE("%d frequencie(s) of lcore %u are available\n",
 			count, pi->lcore_id);
 out:
-	fclose(f);
+	if (close(fd))
+		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
 
 	return ret;
 }
@@ -278,37 +304,39 @@ power_get_available_freqs(struct rte_power_info *pi)
 static int
 power_init_for_setting_freq(struct rte_power_info *pi)
 {
-	FILE *f;
+	int fd;
+	int count;
+	uint32_t i, freq;
 	char fullpath[PATH_MAX];
 	char buf[BUFSIZ];
-	uint32_t i, freq;
-	char *s;
 
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
 			pi->lcore_id);
-	f = fopen(fullpath, "rw+");
-	if (!f) {
+	fd = open(fullpath, O_RDWR);
+	if (fd < 0) {
 		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
 		return -1;
 	}
 
-	s = fgets(buf, sizeof(buf), f);
-	if (!s) {
-		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
+	count = read(fd, buf, sizeof(buf));
+	if (count < 0) {
+		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
 		goto out;
 	}
+	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
 
 	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
 	for (i = 0; i < pi->nb_freqs; i++) {
 		if (freq == pi->freqs[i]) {
 			pi->curr_idx = i;
-			pi->f = f;
+			pi->fd = fd;
 			return 0;
 		}
 	}
 
 out:
-	fclose(f);
+	if (close(fd))
+		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
 
 	return -1;
 }
@@ -373,66 +401,6 @@ rte_power_acpi_cpufreq_init(unsigned lcore_id)
 	return -1;
 }
 
-/**
- * It is to check the governor and then set the original governor back if
- * needed by writing the the sys file.
- */
-static int
-power_set_governor_original(struct rte_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+");
-	if (!f) {
-		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
-		return ret;
-	}
-
-	s = fgets(buf, sizeof(buf), f);
-	if (!s) {
-		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
-		goto 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);
-	if (val < 0) {
-		RTE_LOG(ERR, POWER, "fseek failed\n");
-		goto out;
-	}
-
-	val = fputs(pi->governor_ori, f);
-	if (val < 0) {
-		RTE_LOG(ERR, POWER, "fputs failed\n");
-		goto 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
 rte_power_acpi_cpufreq_exit(unsigned lcore_id)
 {
@@ -452,8 +420,11 @@ rte_power_acpi_cpufreq_exit(unsigned lcore_id)
 	}
 
 	/* Close FD of setting freq */
-	fclose(pi->f);
-	pi->f = NULL;
+	if (close(pi->fd)) {
+		RTE_LOG(ERR, POWER, "Error while closing governor file\n");
+		return -1;
+	}
+	pi->fd = -1;
 
 	/* Set the governor back to the original */
 	if (power_set_governor_original(pi) < 0) {
-- 
2.7.4

[dpdk-dev] [PATCH 3/3] power: check if userspace governor is available

[Radoslaw Biernacki]

Since for new Intel CPU's kernel use intel_pstate driver, which does
not offer userspace governor, it is wise to check the userspace
governor availability before trying to perform governor switch. The
outcome from this patch is direct information for user about the root
cause of failure during the rte_power_acpi_cpufreq_init().

Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
---
 lib/librte_power/rte_power_acpi_cpufreq.c | 74 +++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
index f811bd3..0d9a2a5 100644
--- a/lib/librte_power/rte_power_acpi_cpufreq.c
+++ b/lib/librte_power/rte_power_acpi_cpufreq.c
@@ -65,6 +65,11 @@
 #define POWER_CONVERT_TO_DECIMAL 10
 
 #define POWER_GOVERNOR_USERSPACE "userspace"
+#define POWER_SCALING_DRIVER_INTEL_PSTATE "intel_pstate"
+#define POWER_SYSFILE_SCALING_DRIVER \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_driver"
+#define POWER_SYSFILE_AVAIL_GOVERNOR \
+		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_governors"
 #define POWER_SYSFILE_GOVERNOR   \
 		"/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor"
 #define POWER_SYSFILE_AVAIL_FREQ \
@@ -139,6 +144,70 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
 	return 1;
 }
 
+/* check /sys file for presence of given string */
+static int
+power_check_sysfile_string(const char *fullpath, const char *string)
+{
+	int fd;
+	int count;
+	int ret = -1;
+	char buf[BUFSIZ];
+
+	fd = open(fullpath, O_RDONLY);
+	if (fd < 0) {
+		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
+		return ret;
+	}
+
+	count = read(fd, buf, sizeof(buf));
+	if (count < 0) {
+		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
+		goto out;
+	}
+	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
+
+	if (strstr(buf, string))
+		ret = 0; /* substring found */
+
+out:
+	if (close(fd)) {
+		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
+		return -1;
+	}
+
+	return ret;
+}
+
+/* check if userspace governor is offered by the kernel */
+static int power_check_gov_userspace(unsigned int lcore_id)
+{
+	char fullpath[PATH_MAX];
+
+	snprintf(fullpath, sizeof(fullpath),
+		 POWER_SYSFILE_AVAIL_GOVERNOR, lcore_id);
+	if (power_check_sysfile_string(
+		fullpath, POWER_GOVERNOR_USERSPACE)) {
+
+		RTE_LOG(ERR, POWER, "Userspace governor is not available\n");
+
+		snprintf(fullpath, sizeof(fullpath),
+			 POWER_SYSFILE_SCALING_DRIVER, lcore_id);
+		if (!power_check_sysfile_string(
+			fullpath, POWER_SCALING_DRIVER_INTEL_PSTATE)) {
+
+			RTE_LOG(WARNING, POWER,
+				POWER_SCALING_DRIVER_INTEL_PSTATE "was detected"
+				" which does not offer userspace power governor"
+				". \"intel_pstate=disable\" needs to be added "
+				"to the kernel boot parameters.\n");
+		}
+
+		return -1;
+	}
+
+	return 0;
+}
+
 /**
  * It is to check the current scaling governor by reading sys file, and then
  * set it into 'userspace' if it is not by writing the sys file. The original
@@ -154,6 +223,11 @@ power_set_governor(unsigned int lcore_id, const char *new_gov, char *old_gov,
 	char buf[BUFSIZ];
 	char fullpath[PATH_MAX];
 
+	/* check if userspace governor is available */
+	if (power_check_gov_userspace(lcore_id))
+		return ret;
+
+	/* store current governor and set userspace governor */
 	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR,
 		 lcore_id);
 	fd = open(fullpath, O_RDWR);
-- 
2.7.4

Re: [dpdk-dev] [PATCH 0/3] power: fixes for power ACPI through sysfs

[Radoslaw Biernacki]

Hi David,

I forgot to add V2 to the topic (sorry for that).
But did you had time to look at V2 of this patch?


On 11 November 2017 at 19:55, Radoslaw Biernacki <
radoslaw.biernacki@linaro.org> wrote:

> This series of patches is fixing bug in power ACPI subsystem code as
> well as improve the debugging messages and cleans the code.
>
> V2:
>  - error checking macros removed (checkpatch warning)
>  - using plain open()/read()/write() system functions instead of STDIO
>  - added check for intel_pstate driver together with suggestion about
>    needed kernel boot parameter
>
> Radoslaw Biernacki (3):
>   power: removing code macros
>   power: switching to unbuffered access for /sys files
>   power: check if userspace governor is available
>
>  lib/librte_power/rte_power_acpi_cpufreq.c | 304
> ++++++++++++++++++------------
>  1 file changed, 182 insertions(+), 122 deletions(-)
>
> --
> 2.7.4
>
>

Re: [dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files

[Hunt, David]

Hi Radoslaw,


On 11/11/2017 6:55 PM, Radoslaw Biernacki wrote:
> This patch fixes the bug caused by improper use of buffered stdio file
> access for switching the CPU frequency and governor. When using
> buffered stdio, each fwrite() must use fflush() and the return code
> must be verified. Also fseek() is needed.  Therefore it is better to
> use unbuffered mode or use plain open()/write() functions.  This fix
> use second approach. This not only remove need for ffush() but also
> remove need for fseek() operations.  This patch also reuse some code
> around power_set_governor_userspace() and
> power_set_governor_userspace() functions.
>
> Fixes: 445c6528b55f ("power: common interface for guest and host")
> CC: stable@dpdk.org
>
> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
> ---
>   lib/librte_power/rte_power_acpi_cpufreq.c | 211 +++++++++++++-----------------
>   1 file changed, 91 insertions(+), 120 deletions(-)
>
> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c
> index 3d0872f..f811bd3 100644
> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
> @@ -30,7 +30,7 @@
>    *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>    *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>    */
> -
> +#include <assert.h>
>   #include <stdio.h>
>   #include <sys/types.h>
>   #include <sys/stat.h>
> @@ -47,6 +47,12 @@
>   #include "rte_power_acpi_cpufreq.h"
>   #include "rte_power_common.h"
>   
> +#define min(_x, _y) ({ \
> +	typeof(_x) _min1 = (_x); \
> +	typeof(_y) _min2 = (_y); \
> +	(void) (&_min1 == &_min2); \
> +	_min1 < _min2 ? _min1 : _min2; })
> +
>   #ifdef RTE_LIBRTE_POWER_DEBUG
>   #define POWER_DEBUG_TRACE(fmt, args...) do { \
>   		RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
> @@ -88,7 +94,7 @@ struct rte_power_info {
>   	unsigned lcore_id;                   /**< Logical core id */
>   	uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
>   	uint32_t nb_freqs;                   /**< number of available freqs */
> -	FILE *f;                             /**< FD of scaling_setspeed */
> +	int fd;                              /**< FD of scaling_setspeed */
>   	char governor_ori[32];               /**< Original governor name */
>   	uint32_t curr_idx;                   /**< Freq index in freqs array */
>   	volatile uint32_t state;             /**< Power in use state */
> @@ -105,6 +111,9 @@ static struct rte_power_info lcore_power_info[RTE_MAX_LCORE];
>   static int
>   set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>   {
> +	char buf[BUFSIZ];
> +	int count, ret;
> +
>   	if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) {
>   		RTE_LOG(ERR, POWER, "Invalid frequency index %u, which "
>   				"should be less than %u\n", idx, pi->nb_freqs);
> @@ -117,17 +126,14 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>   
>   	POWER_DEBUG_TRACE("Freqency[%u] %u to be set for lcore %u\n",
>   			idx, pi->freqs[idx], pi->lcore_id);
> -	if (fseek(pi->f, 0, SEEK_SET) < 0) {
> -		RTE_LOG(ERR, POWER, "Fail to set file position indicator to 0 "
> -				"for setting frequency for lcore %u\n", pi->lcore_id);
> +	count = snprintf(buf, sizeof(buf), "%u", pi->freqs[idx]);
> +	assert((size_t)count < sizeof(buf)-1);
> +	ret = write(pi->fd, buf, count);
> +	if (ret != count) {
> +		RTE_LOG(ERR, POWER, "Fail to write new frequency (%s) for "
> +				"lcore %u\n", buf, pi->lcore_id);
>   		return -1;
>   	}
> -	if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) {
> -		RTE_LOG(ERR, POWER, "Fail to write new frequency for "
> -				"lcore %u\n", pi->lcore_id);
> -		return -1;
> -	}
> -	fflush(pi->f);
>   	pi->curr_idx = idx;
>   
>   	return 1;
> @@ -139,90 +145,109 @@ set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>    * governor will be saved for rolling back.
>    */
>   static int
> -power_set_governor_userspace(struct rte_power_info *pi)
> +power_set_governor(unsigned int lcore_id, const char *new_gov, char *old_gov,
> +		   size_t old_gov_len)
>   {
> -	FILE *f;
> +	int fd;
> +	int count, len;
>   	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+");
> -	if (!f) {
> +		 lcore_id);
> +	fd = open(fullpath, O_RDWR);
> +	if (fd < 0) {
>   		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>   		return ret;
>   	}
>   
> -	s = fgets(buf, sizeof(buf), f);
> -	if (!s) {
> -		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
> +	count = read(fd, buf, sizeof(buf));
> +	if (count < 0) {
> +		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>   		goto out;
>   	}
> +	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>   
> -	/* Check if current governor is userspace */
> -	if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
> -			sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
> +	/* Check if current governor is as requested */
> +	if (!strcmp(buf, new_gov)) {
>   		ret = 0;
>   		POWER_DEBUG_TRACE("Power management governor of lcore %u is "
> -				"already userspace\n", pi->lcore_id);
> -		goto out;
> -	}
> -	/* Save the original governor */
> -	snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
> -
> -	/* Write 'userspace' to the governor */
> -	val = fseek(f, 0, SEEK_SET);
> -	if (val < 0) {
> -		RTE_LOG(ERR, POWER, "fseek failed\n");
> +				  "already %s\n", lcore_id, new_gov);
>   		goto out;
>   	}
> +	/* Save the old governor */
> +	if (old_gov)
> +		snprintf(old_gov, old_gov_len, "%s", buf);
>   
> -	val = fputs(POWER_GOVERNOR_USERSPACE, f);
> -	if (val < 0) {
> -		RTE_LOG(ERR, POWER, "fputs failed\n");
> +	/* Set new governor */
> +	len = strlen(new_gov);
> +	count = write(fd, new_gov, len);
> +	if (count != len) {
> +		RTE_LOG(ERR, POWER, "Failed to set %s governor\n", new_gov);
>   		goto out;
>   	}
>   
>   	ret = 0;
>   	RTE_LOG(INFO, POWER, "Power management governor of lcore %u has been "
> -			"set to user space successfully\n", pi->lcore_id);
> +		"set to user space successfully\n", lcore_id);
>   out:
> -	fclose(f);
> +	if (close(fd))
> +		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
>   
>   	return ret;
>   }
>   
>   /**
> + * It is to check the current scaling governor by reading sys file, and then
> + * set it into 'userspace' if it is not by writing the sys file. The original
> + * governor will be saved for rolling back.
> + */
> +static int
> +power_set_governor_userspace(struct rte_power_info *pi)
> +{
> +	return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE,
> +				  pi->governor_ori, sizeof(pi->governor_ori));
> +}
> +
> +/**
> + * It is to check the governor and then set the original governor back if
> + * needed by writing the the sys file.
> + */
> +static int
> +power_set_governor_original(struct rte_power_info *pi)
> +{
> +	return power_set_governor(pi->lcore_id, pi->governor_ori, NULL, 0);
> +}
> +
> +/**
>    * It is to get the available frequencies of the specific lcore by reading the
>    * sys file.
>    */
>   static int
>   power_get_available_freqs(struct rte_power_info *pi)
>   {
> -	FILE *f;
> +	int fd;
>   	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");
> -	if (!f) {
> +		 pi->lcore_id);
> +	fd = open(fullpath, O_RDONLY);
> +	if (fd < 0) {
>   		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>   		return ret;
>   	}
>   
> -	s = fgets(buf, sizeof(buf), f);
> -	if (!s) {
> -		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
> +	count = read(fd, buf, sizeof(buf));
> +	if (count < 0) {
> +		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>   		goto out;
>   	}
> +	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>   
>   	/* Strip the line break if there is */
>   	p = strchr(buf, '\n');
> @@ -267,7 +292,8 @@ power_get_available_freqs(struct rte_power_info *pi)
>   	POWER_DEBUG_TRACE("%d frequencie(s) of lcore %u are available\n",
>   			count, pi->lcore_id);
>   out:
> -	fclose(f);
> +	if (close(fd))
> +		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
>   
>   	return ret;
>   }
> @@ -278,37 +304,39 @@ power_get_available_freqs(struct rte_power_info *pi)
>   static int
>   power_init_for_setting_freq(struct rte_power_info *pi)
>   {
> -	FILE *f;
> +	int fd;
> +	int count;
> +	uint32_t i, freq;
>   	char fullpath[PATH_MAX];
>   	char buf[BUFSIZ];
> -	uint32_t i, freq;
> -	char *s;
>   
>   	snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>   			pi->lcore_id);
> -	f = fopen(fullpath, "rw+");
> -	if (!f) {
> +	fd = open(fullpath, O_RDWR);
> +	if (fd < 0) {
>   		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>   		return -1;
>   	}
>   
> -	s = fgets(buf, sizeof(buf), f);
> -	if (!s) {
> -		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
> +	count = read(fd, buf, sizeof(buf));
> +	if (count < 0) {
> +		RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>   		goto out;
>   	}
> +	buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>   
>   	freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
>   	for (i = 0; i < pi->nb_freqs; i++) {
>   		if (freq == pi->freqs[i]) {
>   			pi->curr_idx = i;
> -			pi->f = f;
> +			pi->fd = fd;
>   			return 0;
>   		}
>   	}
>   
>   out:
> -	fclose(f);
> +	if (close(fd))
> +		RTE_LOG(ERR, POWER, "Error while closing file %s\n", fullpath);
>   
>   	return -1;
>   }
> @@ -373,66 +401,6 @@ rte_power_acpi_cpufreq_init(unsigned lcore_id)
>   	return -1;
>   }
>   
> -/**
> - * It is to check the governor and then set the original governor back if
> - * needed by writing the the sys file.
> - */
> -static int
> -power_set_governor_original(struct rte_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+");
> -	if (!f) {
> -		RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
> -		return ret;
> -	}
> -
> -	s = fgets(buf, sizeof(buf), f);
> -	if (!s) {
> -		RTE_LOG(ERR, POWER, "fgets returns nothing\n");
> -		goto 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);
> -	if (val < 0) {
> -		RTE_LOG(ERR, POWER, "fseek failed\n");
> -		goto out;
> -	}
> -
> -	val = fputs(pi->governor_ori, f);
> -	if (val < 0) {
> -		RTE_LOG(ERR, POWER, "fputs failed\n");
> -		goto 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
>   rte_power_acpi_cpufreq_exit(unsigned lcore_id)
>   {
> @@ -452,8 +420,11 @@ rte_power_acpi_cpufreq_exit(unsigned lcore_id)
>   	}
>   
>   	/* Close FD of setting freq */
> -	fclose(pi->f);
> -	pi->f = NULL;
> +	if (close(pi->fd)) {
> +		RTE_LOG(ERR, POWER, "Error while closing governor file\n");
> +		return -1;
> +	}
> +	pi->fd = -1;
>   
>   	/* Set the governor back to the original */
>   	if (power_set_governor_original(pi) < 0) {

Could you re-base on top of 17.11? It fails to apply currently.

I think there is a little much going on in this patch, there are a 
couple of discreet changes that could/should be split into individual 
patches.
Your first patch in this series does this well, remove the macros.
I think this one could be split into two patches:
     1. First the switch to unbuffered file access, including the extra 
error checking.
     2. Then rework the functions, i.e. add the new "power_set_governor" 
function and have "_userspace" and "_original" call that.
Do you agree?
Regards,
Dave

Re: [dpdk-dev] [PATCH 2/3] power: switching to unbuffered access for /sys files

[Radoslaw Biernacki]

Hi David,

Sorry for log delay.
I will make V3 with your suggestions.
Thanks.

On 23 November 2017 at 15:42, Hunt, David <david.hunt@intel.com> wrote:

> Hi Radoslaw,
>
>
>
> On 11/11/2017 6:55 PM, Radoslaw Biernacki wrote:
>
>> This patch fixes the bug caused by improper use of buffered stdio file
>> access for switching the CPU frequency and governor. When using
>> buffered stdio, each fwrite() must use fflush() and the return code
>> must be verified. Also fseek() is needed.  Therefore it is better to
>> use unbuffered mode or use plain open()/write() functions.  This fix
>> use second approach. This not only remove need for ffush() but also
>> remove need for fseek() operations.  This patch also reuse some code
>> around power_set_governor_userspace() and
>> power_set_governor_userspace() functions.
>>
>> Fixes: 445c6528b55f ("power: common interface for guest and host")
>> CC: stable@dpdk.org
>>
>> Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org>
>> ---
>>   lib/librte_power/rte_power_acpi_cpufreq.c | 211
>> +++++++++++++-----------------
>>   1 file changed, 91 insertions(+), 120 deletions(-)
>>
>> diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c
>> b/lib/librte_power/rte_power_acpi_cpufreq.c
>> index 3d0872f..f811bd3 100644
>> --- a/lib/librte_power/rte_power_acpi_cpufreq.c
>> +++ b/lib/librte_power/rte_power_acpi_cpufreq.c
>> @@ -30,7 +30,7 @@
>>    *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE
>> USE
>>    *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH
>> DAMAGE.
>>    */
>> -
>> +#include <assert.h>
>>   #include <stdio.h>
>>   #include <sys/types.h>
>>   #include <sys/stat.h>
>> @@ -47,6 +47,12 @@
>>   #include "rte_power_acpi_cpufreq.h"
>>   #include "rte_power_common.h"
>>   +#define min(_x, _y) ({ \
>> +       typeof(_x) _min1 = (_x); \
>> +       typeof(_y) _min2 = (_y); \
>> +       (void) (&_min1 == &_min2); \
>> +       _min1 < _min2 ? _min1 : _min2; })
>> +
>>   #ifdef RTE_LIBRTE_POWER_DEBUG
>>   #define POWER_DEBUG_TRACE(fmt, args...) do { \
>>                 RTE_LOG(ERR, POWER, "%s: " fmt, __func__, ## args); \
>> @@ -88,7 +94,7 @@ struct rte_power_info {
>>         unsigned lcore_id;                   /**< Logical core id */
>>         uint32_t freqs[RTE_MAX_LCORE_FREQS]; /**< Frequency array */
>>         uint32_t nb_freqs;                   /**< number of available
>> freqs */
>> -       FILE *f;                             /**< FD of scaling_setspeed
>> */
>> +       int fd;                              /**< FD of scaling_setspeed
>> */
>>         char governor_ori[32];               /**< Original governor name
>> */
>>         uint32_t curr_idx;                   /**< Freq index in freqs
>> array */
>>         volatile uint32_t state;             /**< Power in use state */
>> @@ -105,6 +111,9 @@ static struct rte_power_info
>> lcore_power_info[RTE_MAX_LCORE];
>>   static int
>>   set_freq_internal(struct rte_power_info *pi, uint32_t idx)
>>   {
>> +       char buf[BUFSIZ];
>> +       int count, ret;
>> +
>>         if (idx >= RTE_MAX_LCORE_FREQS || idx >= pi->nb_freqs) {
>>                 RTE_LOG(ERR, POWER, "Invalid frequency index %u, which "
>>                                 "should be less than %u\n", idx,
>> pi->nb_freqs);
>> @@ -117,17 +126,14 @@ set_freq_internal(struct rte_power_info *pi,
>> uint32_t idx)
>>         POWER_DEBUG_TRACE("Freqency[%u] %u to be set for lcore %u\n",
>>                         idx, pi->freqs[idx], pi->lcore_id);
>> -       if (fseek(pi->f, 0, SEEK_SET) < 0) {
>> -               RTE_LOG(ERR, POWER, "Fail to set file position indicator
>> to 0 "
>> -                               "for setting frequency for lcore %u\n",
>> pi->lcore_id);
>> +       count = snprintf(buf, sizeof(buf), "%u", pi->freqs[idx]);
>> +       assert((size_t)count < sizeof(buf)-1);
>> +       ret = write(pi->fd, buf, count);
>> +       if (ret != count) {
>> +               RTE_LOG(ERR, POWER, "Fail to write new frequency (%s) for
>> "
>> +                               "lcore %u\n", buf, pi->lcore_id);
>>                 return -1;
>>         }
>> -       if (fprintf(pi->f, "%u", pi->freqs[idx]) < 0) {
>> -               RTE_LOG(ERR, POWER, "Fail to write new frequency for "
>> -                               "lcore %u\n", pi->lcore_id);
>> -               return -1;
>> -       }
>> -       fflush(pi->f);
>>         pi->curr_idx = idx;
>>         return 1;
>> @@ -139,90 +145,109 @@ set_freq_internal(struct rte_power_info *pi,
>> uint32_t idx)
>>    * governor will be saved for rolling back.
>>    */
>>   static int
>> -power_set_governor_userspace(struct rte_power_info *pi)
>> +power_set_governor(unsigned int lcore_id, const char *new_gov, char
>> *old_gov,
>> +                  size_t old_gov_len)
>>   {
>> -       FILE *f;
>> +       int fd;
>> +       int count, len;
>>         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+");
>> -       if (!f) {
>> +                lcore_id);
>> +       fd = open(fullpath, O_RDWR);
>> +       if (fd < 0) {
>>                 RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>>                 return ret;
>>         }
>>   -     s = fgets(buf, sizeof(buf), f);
>> -       if (!s) {
>> -               RTE_LOG(ERR, POWER, "fgets returns nothing\n");
>> +       count = read(fd, buf, sizeof(buf));
>> +       if (count < 0) {
>> +               RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>>                 goto out;
>>         }
>> +       buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>>   -     /* Check if current governor is userspace */
>> -       if (strncmp(buf, POWER_GOVERNOR_USERSPACE,
>> -                       sizeof(POWER_GOVERNOR_USERSPACE)) == 0) {
>> +       /* Check if current governor is as requested */
>> +       if (!strcmp(buf, new_gov)) {
>>                 ret = 0;
>>                 POWER_DEBUG_TRACE("Power management governor of lcore %u
>> is "
>> -                               "already userspace\n", pi->lcore_id);
>> -               goto out;
>> -       }
>> -       /* Save the original governor */
>> -       snprintf(pi->governor_ori, sizeof(pi->governor_ori), "%s", buf);
>> -
>> -       /* Write 'userspace' to the governor */
>> -       val = fseek(f, 0, SEEK_SET);
>> -       if (val < 0) {
>> -               RTE_LOG(ERR, POWER, "fseek failed\n");
>> +                                 "already %s\n", lcore_id, new_gov);
>>                 goto out;
>>         }
>> +       /* Save the old governor */
>> +       if (old_gov)
>> +               snprintf(old_gov, old_gov_len, "%s", buf);
>>   -     val = fputs(POWER_GOVERNOR_USERSPACE, f);
>> -       if (val < 0) {
>> -               RTE_LOG(ERR, POWER, "fputs failed\n");
>> +       /* Set new governor */
>> +       len = strlen(new_gov);
>> +       count = write(fd, new_gov, len);
>> +       if (count != len) {
>> +               RTE_LOG(ERR, POWER, "Failed to set %s governor\n",
>> new_gov);
>>                 goto out;
>>         }
>>         ret = 0;
>>         RTE_LOG(INFO, POWER, "Power management governor of lcore %u has
>> been "
>> -                       "set to user space successfully\n", pi->lcore_id);
>> +               "set to user space successfully\n", lcore_id);
>>   out:
>> -       fclose(f);
>> +       if (close(fd))
>> +               RTE_LOG(ERR, POWER, "Error while closing file %s\n",
>> fullpath);
>>         return ret;
>>   }
>>     /**
>> + * It is to check the current scaling governor by reading sys file, and
>> then
>> + * set it into 'userspace' if it is not by writing the sys file. The
>> original
>> + * governor will be saved for rolling back.
>> + */
>> +static int
>> +power_set_governor_userspace(struct rte_power_info *pi)
>> +{
>> +       return power_set_governor(pi->lcore_id, POWER_GOVERNOR_USERSPACE,
>> +                                 pi->governor_ori,
>> sizeof(pi->governor_ori));
>> +}
>> +
>> +/**
>> + * It is to check the governor and then set the original governor back if
>> + * needed by writing the the sys file.
>> + */
>> +static int
>> +power_set_governor_original(struct rte_power_info *pi)
>> +{
>> +       return power_set_governor(pi->lcore_id, pi->governor_ori, NULL,
>> 0);
>> +}
>> +
>> +/**
>>    * It is to get the available frequencies of the specific lcore by
>> reading the
>>    * sys file.
>>    */
>>   static int
>>   power_get_available_freqs(struct rte_power_info *pi)
>>   {
>> -       FILE *f;
>> +       int fd;
>>         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");
>> -       if (!f) {
>> +                pi->lcore_id);
>> +       fd = open(fullpath, O_RDONLY);
>> +       if (fd < 0) {
>>                 RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>>                 return ret;
>>         }
>>   -     s = fgets(buf, sizeof(buf), f);
>> -       if (!s) {
>> -               RTE_LOG(ERR, POWER, "fgets returns nothing\n");
>> +       count = read(fd, buf, sizeof(buf));
>> +       if (count < 0) {
>> +               RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>>                 goto out;
>>         }
>> +       buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>>         /* Strip the line break if there is */
>>         p = strchr(buf, '\n');
>> @@ -267,7 +292,8 @@ power_get_available_freqs(struct rte_power_info *pi)
>>         POWER_DEBUG_TRACE("%d frequencie(s) of lcore %u are available\n",
>>                         count, pi->lcore_id);
>>   out:
>> -       fclose(f);
>> +       if (close(fd))
>> +               RTE_LOG(ERR, POWER, "Error while closing file %s\n",
>> fullpath);
>>         return ret;
>>   }
>> @@ -278,37 +304,39 @@ power_get_available_freqs(struct rte_power_info
>> *pi)
>>   static int
>>   power_init_for_setting_freq(struct rte_power_info *pi)
>>   {
>> -       FILE *f;
>> +       int fd;
>> +       int count;
>> +       uint32_t i, freq;
>>         char fullpath[PATH_MAX];
>>         char buf[BUFSIZ];
>> -       uint32_t i, freq;
>> -       char *s;
>>         snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED,
>>                         pi->lcore_id);
>> -       f = fopen(fullpath, "rw+");
>> -       if (!f) {
>> +       fd = open(fullpath, O_RDWR);
>> +       if (fd < 0) {
>>                 RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>>                 return -1;
>>         }
>>   -     s = fgets(buf, sizeof(buf), f);
>> -       if (!s) {
>> -               RTE_LOG(ERR, POWER, "fgets returns nothing\n");
>> +       count = read(fd, buf, sizeof(buf));
>> +       if (count < 0) {
>> +               RTE_LOG(ERR, POWER, "Failed to read from %s\n", fullpath);
>>                 goto out;
>>         }
>> +       buf[min((size_t)count, sizeof(buf)-1)] = '\0';
>>         freq = strtoul(buf, NULL, POWER_CONVERT_TO_DECIMAL);
>>         for (i = 0; i < pi->nb_freqs; i++) {
>>                 if (freq == pi->freqs[i]) {
>>                         pi->curr_idx = i;
>> -                       pi->f = f;
>> +                       pi->fd = fd;
>>                         return 0;
>>                 }
>>         }
>>     out:
>> -       fclose(f);
>> +       if (close(fd))
>> +               RTE_LOG(ERR, POWER, "Error while closing file %s\n",
>> fullpath);
>>         return -1;
>>   }
>> @@ -373,66 +401,6 @@ rte_power_acpi_cpufreq_init(unsigned lcore_id)
>>         return -1;
>>   }
>>   -/**
>> - * It is to check the governor and then set the original governor back if
>> - * needed by writing the the sys file.
>> - */
>> -static int
>> -power_set_governor_original(struct rte_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+");
>> -       if (!f) {
>> -               RTE_LOG(ERR, POWER, "Failed to open %s\n", fullpath);
>> -               return ret;
>> -       }
>> -
>> -       s = fgets(buf, sizeof(buf), f);
>> -       if (!s) {
>> -               RTE_LOG(ERR, POWER, "fgets returns nothing\n");
>> -               goto 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);
>> -       if (val < 0) {
>> -               RTE_LOG(ERR, POWER, "fseek failed\n");
>> -               goto out;
>> -       }
>> -
>> -       val = fputs(pi->governor_ori, f);
>> -       if (val < 0) {
>> -               RTE_LOG(ERR, POWER, "fputs failed\n");
>> -               goto 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
>>   rte_power_acpi_cpufreq_exit(unsigned lcore_id)
>>   {
>> @@ -452,8 +420,11 @@ rte_power_acpi_cpufreq_exit(unsigned lcore_id)
>>         }
>>         /* Close FD of setting freq */
>> -       fclose(pi->f);
>> -       pi->f = NULL;
>> +       if (close(pi->fd)) {
>> +               RTE_LOG(ERR, POWER, "Error while closing governor
>> file\n");
>> +               return -1;
>> +       }
>> +       pi->fd = -1;
>>         /* Set the governor back to the original */
>>         if (power_set_governor_original(pi) < 0) {
>>
>
> Could you re-base on top of 17.11? It fails to apply currently.
>
> I think there is a little much going on in this patch, there are a couple
> of discreet changes that could/should be split into individual patches.
> Your first patch in this series does this well, remove the macros.
> I think this one could be split into two patches:
>     1. First the switch to unbuffered file access, including the extra
> error checking.
>     2. Then rework the functions, i.e. add the new "power_set_governor"
> function and have "_userspace" and "_original" call that.
> Do you agree?
> Regards,
> Dave
>
>

Re: [dpdk-dev] [dpdk-stable] [PATCH 2/3] power: switching to unbuffered access for /sys files

[Thomas Monjalon]

01/12/2017 22:01, Radoslaw Biernacki:
> Hi David,
> 
> Sorry for log delay.
> I will make V3 with your suggestions.
> Thanks.

Hi,
Any news?