diff mbox series

[v5,1/2] test/power: add delay before checking cpuinfo cur freq

Message ID 20210415055930.3899-2-richael.zhuang@arm.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series test/power: fix bugs in cpufreq test |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Richael Zhuang April 15, 2021, 5:59 a.m. UTC 
  For some platforms the newly-set frequency may not be effective
immediately. If we didn't get the right value from cpuinfo_cur_freq
immediately, add 10ms delay each time before rechecking until
timeout.

From our test, for some arm platforms, it requires up to 700ms when
going from a minimum to a maximum frequency. And it's not the
driver/software issue.

Fixes: ed7c51a6a680 ("app/test: vm power management")
Cc: alan.carew@intel.com
Cc: stable@dpdk.org

Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
---
 app/test/test_power_cpufreq.c | 27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

Comments

Hunt, David April 20, 2021, 12:38 p.m. UTC | #1 
On 15/4/2021 6:59 AM, Richael Zhuang wrote:
> For some platforms the newly-set frequency may not be effective
> immediately. If we didn't get the right value from cpuinfo_cur_freq
> immediately, add 10ms delay each time before rechecking until
> timeout.
>
>  From our test, for some arm platforms, it requires up to 700ms when
> going from a minimum to a maximum frequency. And it's not the
> driver/software issue.
>
> Fixes: ed7c51a6a680 ("app/test: vm power management")
> Cc: alan.carew@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> ---
>   app/test/test_power_cpufreq.c | 27 ++++++++++++++++++++++-----
>   1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
> index 731c6b4dc..d47b3e0a1 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -8,6 +8,7 @@
>   #include <limits.h>
>   #include <string.h>
>   #include <inttypes.h>
> +#include <rte_cycles.h>
>   
>   #include "test.h"
>   
> @@ -44,11 +45,13 @@ static int
>   check_cur_freq(unsigned lcore_id, uint32_t idx)
>   {
>   #define TEST_POWER_CONVERT_TO_DECIMAL 10
> +#define MAX_LOOP 100
>   	FILE *f;
>   	char fullpath[PATH_MAX];
>   	char buf[BUFSIZ];
>   	uint32_t cur_freq;
>   	int ret = -1;
> +	int i;
>   
>   	if (snprintf(fullpath, sizeof(fullpath),
>   		TEST_POWER_SYSFILE_CUR_FREQ, lcore_id) < 0) {
> @@ -58,13 +61,27 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
>   	if (f == NULL) {
>   		return 0;
>   	}
> -	if (fgets(buf, sizeof(buf), f) == NULL) {
> -		goto fail_get_cur_freq;
> +	for (i = 0; i < MAX_LOOP; i++) {
> +		fflush(f);
> +		if (fgets(buf, sizeof(buf), f) == NULL)
> +			goto fail_all;
> +
> +		cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
> +		ret = (freqs[idx] == cur_freq ? 0 : -1);
> +
> +		if (ret == 0)
> +			break;
> +
> +		if (fseek(f, 0, SEEK_SET) < 0) {
> +			printf("Fail to set file position indicator to 0\n");
> +			goto fail_all;
> +		}
> +
> +		/* wait for the value to be updated */
> +		rte_delay_ms(10);
>   	}
> -	cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
> -	ret = (freqs[idx] == cur_freq ? 0 : -1);
>   
> -fail_get_cur_freq:
> +fail_all:
>   	fclose(f);
>   
>   	return ret;

Hi Richael

On your system, is the current cpu frequency found in cpuinfo_cur_freq 
or in scaling_cur_freq? On my system, which uses intel_pstate driver, 
there is no file called cpuinfo_cur_freq, but the test works when I 
change TEST_POWER_SYSFILE_CUR_FREQ to scaling_cur_freq.

I know that's unrelated to your patch above, but it migth be worth using 
a file common to all platforms, or else attempting to open one, and if 
that fails, try open the other.

Rgds,
Dave.
Hunt, David April 20, 2021, 1:15 p.m. UTC | #2 
On 20/4/2021 1:38 PM, David Hunt wrote:
>
> On 15/4/2021 6:59 AM, Richael Zhuang wrote:
>> For some platforms the newly-set frequency may not be effective
>> immediately. If we didn't get the right value from cpuinfo_cur_freq
>> immediately, add 10ms delay each time before rechecking until
>> timeout.
>>
>>  From our test, for some arm platforms, it requires up to 700ms when
>> going from a minimum to a maximum frequency. And it's not the
>> driver/software issue.
>>
>> Fixes: ed7c51a6a680 ("app/test: vm power management")
>> Cc: alan.carew@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
>> ---
>>   app/test/test_power_cpufreq.c | 27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/app/test/test_power_cpufreq.c 
>> b/app/test/test_power_cpufreq.c
>> index 731c6b4dc..d47b3e0a1 100644
>> --- a/app/test/test_power_cpufreq.c
>> +++ b/app/test/test_power_cpufreq.c
>> @@ -8,6 +8,7 @@
>>   #include <limits.h>
>>   #include <string.h>
>>   #include <inttypes.h>
>> +#include <rte_cycles.h>
>>     #include "test.h"
>>   @@ -44,11 +45,13 @@ static int
>>   check_cur_freq(unsigned lcore_id, uint32_t idx)
>>   {
>>   #define TEST_POWER_CONVERT_TO_DECIMAL 10
>> +#define MAX_LOOP 100
>>       FILE *f;
>>       char fullpath[PATH_MAX];
>>       char buf[BUFSIZ];
>>       uint32_t cur_freq;
>>       int ret = -1;
>> +    int i;
>>         if (snprintf(fullpath, sizeof(fullpath),
>>           TEST_POWER_SYSFILE_CUR_FREQ, lcore_id) < 0) {
>> @@ -58,13 +61,27 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
>>       if (f == NULL) {
>>           return 0;
>>       }
>> -    if (fgets(buf, sizeof(buf), f) == NULL) {
>> -        goto fail_get_cur_freq;
>> +    for (i = 0; i < MAX_LOOP; i++) {
>> +        fflush(f);
>> +        if (fgets(buf, sizeof(buf), f) == NULL)
>> +            goto fail_all;
>> +
>> +        cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
>> +        ret = (freqs[idx] == cur_freq ? 0 : -1);
>> +
>> +        if (ret == 0)
>> +            break;
>> +
>> +        if (fseek(f, 0, SEEK_SET) < 0) {
>> +            printf("Fail to set file position indicator to 0\n");
>> +            goto fail_all;
>> +        }
>> +
>> +        /* wait for the value to be updated */
>> +        rte_delay_ms(10);
>>       }
>> -    cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
>> -    ret = (freqs[idx] == cur_freq ? 0 : -1);
>>   -fail_get_cur_freq:
>> +fail_all:
>>       fclose(f);
>>         return ret;
>
> Hi Richael
>
> On your system, is the current cpu frequency found in cpuinfo_cur_freq 
> or in scaling_cur_freq? On my system, which uses intel_pstate driver, 
> there is no file called cpuinfo_cur_freq, but the test works when I 
> change TEST_POWER_SYSFILE_CUR_FREQ to scaling_cur_freq.
>
> I know that's unrelated to your patch above, but it migth be worth 
> using a file common to all platforms, or else attempting to open one, 
> and if that fails, try open the other.
>
> Rgds,
> Dave.
>

Hi Richael,

     I've tested on other systems, where cpuinfo_cur_freq is present, 
and this patch works fine.

And even though 1 second seems like a very long time to change 
frequency, at leaset on responsive systems it will return quickly 
because of the retry mechanism you've added, and won't cause delays in 
the test.

I'll do a separate patch for checking both cpuinfo_cur_freq and 
scaling_cur_freq.

Reviewed-by: David Hunt <david.hunt@intel.com>
Richael Zhuang April 21, 2021, 2:41 a.m. UTC | #3 
> -----Original Message-----
> From: David Hunt <david.hunt@intel.com>
> Sent: Tuesday, April 20, 2021 8:39 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; alan.carew@intel.com; stable@dpdk.org; Pablo de
> Lara <pablo.de.lara.guarch@intel.com>
> Subject: Re: [PATCH v5 1/2] test/power: add delay before checking cpuinfo
> cur freq
> 
> 
> On 15/4/2021 6:59 AM, Richael Zhuang wrote:
> > For some platforms the newly-set frequency may not be effective
> > immediately. If we didn't get the right value from cpuinfo_cur_freq
> > immediately, add 10ms delay each time before rechecking until timeout.
> >
> >  From our test, for some arm platforms, it requires up to 700ms when
> > going from a minimum to a maximum frequency. And it's not the
> > driver/software issue.
> >
> > Fixes: ed7c51a6a680 ("app/test: vm power management")
> > Cc: alan.carew@intel.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> > ---
> >   app/test/test_power_cpufreq.c | 27 ++++++++++++++++++++++-----
> >   1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/app/test/test_power_cpufreq.c
> > b/app/test/test_power_cpufreq.c index 731c6b4dc..d47b3e0a1 100644
> > --- a/app/test/test_power_cpufreq.c
> > +++ b/app/test/test_power_cpufreq.c
> > @@ -8,6 +8,7 @@
> >   #include <limits.h>
> >   #include <string.h>
> >   #include <inttypes.h>
> > +#include <rte_cycles.h>
> >
> >   #include "test.h"
> >
> > @@ -44,11 +45,13 @@ static int
> >   check_cur_freq(unsigned lcore_id, uint32_t idx)
> >   {
> >   #define TEST_POWER_CONVERT_TO_DECIMAL 10
> > +#define MAX_LOOP 100
> >   	FILE *f;
> >   	char fullpath[PATH_MAX];
> >   	char buf[BUFSIZ];
> >   	uint32_t cur_freq;
> >   	int ret = -1;
> > +	int i;
> >
> >   	if (snprintf(fullpath, sizeof(fullpath),
> >   		TEST_POWER_SYSFILE_CUR_FREQ, lcore_id) < 0) { @@ -58,13
> +61,27 @@
> > check_cur_freq(unsigned lcore_id, uint32_t idx)
> >   	if (f == NULL) {
> >   		return 0;
> >   	}
> > -	if (fgets(buf, sizeof(buf), f) == NULL) {
> > -		goto fail_get_cur_freq;
> > +	for (i = 0; i < MAX_LOOP; i++) {
> > +		fflush(f);
> > +		if (fgets(buf, sizeof(buf), f) == NULL)
> > +			goto fail_all;
> > +
> > +		cur_freq = strtoul(buf, NULL,
> TEST_POWER_CONVERT_TO_DECIMAL);
> > +		ret = (freqs[idx] == cur_freq ? 0 : -1);
> > +
> > +		if (ret == 0)
> > +			break;
> > +
> > +		if (fseek(f, 0, SEEK_SET) < 0) {
> > +			printf("Fail to set file position indicator to 0\n");
> > +			goto fail_all;
> > +		}
> > +
> > +		/* wait for the value to be updated */
> > +		rte_delay_ms(10);
> >   	}
> > -	cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
> > -	ret = (freqs[idx] == cur_freq ? 0 : -1);
> >
> > -fail_get_cur_freq:
> > +fail_all:
> >   	fclose(f);
> >
> >   	return ret;
> 
> Hi Richael
> 
> On your system, is the current cpu frequency found in cpuinfo_cur_freq or in
> scaling_cur_freq? On my system, which uses intel_pstate driver, there is no
> file called cpuinfo_cur_freq, but the test works when I change
> TEST_POWER_SYSFILE_CUR_FREQ to scaling_cur_freq.
> 
> I know that's unrelated to your patch above, but it migth be worth using a file
> common to all platforms, or else attempting to open one, and if that fails, try
> open the other.
> 
> Rgds,
> Dave.
> 
Hi David,
Thanks for review. We have cpuinfo_cur_freq on our platform. For acpi_cpufreq it's also  cpuinfo_cur_freq. For pstate, the check is skipped because there is no such file. 

Best Regards,
Richael
diff mbox series

Patch

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index 731c6b4dc..d47b3e0a1 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -8,6 +8,7 @@ 
 #include <limits.h>
 #include <string.h>
 #include <inttypes.h>
+#include <rte_cycles.h>
 
 #include "test.h"
 
@@ -44,11 +45,13 @@  static int
 check_cur_freq(unsigned lcore_id, uint32_t idx)
 {
 #define TEST_POWER_CONVERT_TO_DECIMAL 10
+#define MAX_LOOP 100
 	FILE *f;
 	char fullpath[PATH_MAX];
 	char buf[BUFSIZ];
 	uint32_t cur_freq;
 	int ret = -1;
+	int i;
 
 	if (snprintf(fullpath, sizeof(fullpath),
 		TEST_POWER_SYSFILE_CUR_FREQ, lcore_id) < 0) {
@@ -58,13 +61,27 @@  check_cur_freq(unsigned lcore_id, uint32_t idx)
 	if (f == NULL) {
 		return 0;
 	}
-	if (fgets(buf, sizeof(buf), f) == NULL) {
-		goto fail_get_cur_freq;
+	for (i = 0; i < MAX_LOOP; i++) {
+		fflush(f);
+		if (fgets(buf, sizeof(buf), f) == NULL)
+			goto fail_all;
+
+		cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
+		ret = (freqs[idx] == cur_freq ? 0 : -1);
+
+		if (ret == 0)
+			break;
+
+		if (fseek(f, 0, SEEK_SET) < 0) {
+			printf("Fail to set file position indicator to 0\n");
+			goto fail_all;
+		}
+
+		/* wait for the value to be updated */
+		rte_delay_ms(10);
 	}
-	cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
-	ret = (freqs[idx] == cur_freq ? 0 : -1);
 
-fail_get_cur_freq:
+fail_all:
 	fclose(f);
 
 	return ret;