diff mbox

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

Message ID 1508161628-4265-1-git-send-email-radoslaw.biernacki@linaro.org (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Radoslaw Biernacki Oct. 16, 2017, 1:47 p.m. UTC 
  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(-)

Comments

Hunt, David Oct. 18, 2017, 10:28 a.m. UTC | #1 
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
Hunt, David Oct. 18, 2017, 10:33 a.m. UTC | #2 
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---
Radoslaw Biernacki Oct. 18, 2017, 1:56 p.m. UTC | #3 
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.
Radoslaw Biernacki Nov. 11, 2017, 6:55 p.m. UTC | #4 
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(-)
Radoslaw Biernacki Nov. 21, 2017, 11:09 a.m. UTC | #5 
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
>
>
diff mbox

Patch

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);