[dpdk-dev] [PATCH v1 2/2] test/power: fix a bug in cpufreq autotest

[dpdk-dev] [PATCH v1 2/2] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

For platforms that don't support turbo boost,rte_power_turbo_status()
returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
check_power_turbo() to continue if rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1

Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
Cc: lukaszx.krakowiak@intel.com
Cc: stable@dpdk.org

CustomizedGitHooks: yes
Change-Id: I5702d5c647e941253c556ae2be8c1edeefacb618
Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
---
 app/test/test_power_cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index c4f4541ac..7a93bc90a 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -389,7 +389,7 @@ check_power_turbo(void)
 {
        int ret;

-       if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
+       if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
                printf("Turbo not available on lcore %u, skipping test\n",
                                TEST_POWER_LCORE_ID);
                return 0;
--
2.20.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[dpdk-dev] [PATCH v1 2/2] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

For platforms that don't support turbo boost,rte_power_turbo_status()
returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
check_power_turbo() to continue if rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1

Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
Cc: lukaszx.krakowiak@intel.com
Cc: stable@dpdk.org

Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
---
 app/test/test_power_cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index c4f4541ac..7a93bc90a 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -389,7 +389,7 @@ check_power_turbo(void)
 {
        int ret;

-       if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
+       if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
                printf("Turbo not available on lcore %u, skipping test\n",
                                TEST_POWER_LCORE_ID);
                return 0;
--
2.20.1

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

[dpdk-dev] [PATCH v2 2/2] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

For platforms that don't support turbo boost,rte_power_turbo_status()
returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
check_power_turbo() to continue if rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1

Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
Cc: lukaszx.krakowiak@intel.com
Cc: stable@dpdk.org

Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
---
 app/test/test_power_cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index c4f4541ac..7a93bc90a 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -389,7 +389,7 @@ check_power_turbo(void)
 {
 	int ret;
 
-	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
+	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
 		printf("Turbo not available on lcore %u, skipping test\n",
 				TEST_POWER_LCORE_ID);
 		return 0;
-- 
2.20.1

[dpdk-dev] [PATCH v3 0/3] test/power: fix bugs in cpufreq test

[Richael Zhuang]

This series fixed some bugs in unit tests for power library.

v3:
Break patch into series for individual bug fixes.

Richael Zhuang (3):
  test/power: round cpuinfo cur freq value in cpufreq autotest
  test/power: fix a bug in cpufreq autotest
  test/power: add delay before checking cpuinfo cur freq

 app/test/test_power_cpufreq.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v3 1/3] test/power: round cpuinfo cur freq value in cpufreq autotest

[Richael Zhuang]

The value in "/sys/.../cpuinfo_cur_freq" may not be exactly the
same as what was set. For example, if "2400000" is written to
"/sys/.../cpufreq/scaling_setspeed" to set the frequency, then the
value in "/sys/.../cpuinfo_cur_freq" may be "2401222". So need to
round the value.

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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index 731c6b4dc..1f4d8bb05 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -34,6 +34,10 @@ test_power_caps(void)
 #define TEST_POWER_LCORE_INVALID ((unsigned)RTE_MAX_LCORE)
 #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
 
+/* macros used for rounding frequency to nearest 100000 */
+#define TEST_FREQ_ROUNDING_DELTA 50000
+#define TEST_ROUND_FREQ_TO_N_100000 100000
+
 #define TEST_POWER_SYSFILE_CUR_FREQ \
 	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
 
@@ -62,7 +66,17 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
 		goto fail_get_cur_freq;
 	}
 	cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
-	ret = (freqs[idx] == cur_freq ? 0 : -1);
+
+	/* convert the frequency to nearest 100000 value
+	 * Ex: if cur_freq=1396789 then freq_conv=1400000
+	 * Ex: if cur_freq=800030 then freq_conv=800000
+	 */
+	unsigned int freq_conv = 0;
+	freq_conv = (cur_freq + TEST_FREQ_ROUNDING_DELTA)
+				/ TEST_ROUND_FREQ_TO_N_100000;
+	freq_conv = freq_conv * TEST_ROUND_FREQ_TO_N_100000;
+
+	ret = (freqs[idx] == freq_conv ? 0 : -1);
 
 fail_get_cur_freq:
 	fclose(f);
-- 
2.20.1

[dpdk-dev] [PATCH v4 0/2] test/power: fix bugs in cpufreq test

[Richael Zhuang]

This series fixed some bugs in unit tests for power library.

v4:
abort the patch about checking -ENOTSUP in turbo test. 

Richael Zhuang (2):
  test/power: round cpuinfo cur freq value in cpufreq autotest
  test/power: add delay before checking cpuinfo cur freq

 app/test/test_power_cpufreq.c | 41 +++++++++++++++++++++++++++++++----
 1 file changed, 37 insertions(+), 4 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v4 1/2] test/power: round cpuinfo cur freq value in cpufreq autotest

[Richael Zhuang]

The value in "/sys/.../cpuinfo_cur_freq" may not be exactly the
same as what was set. For example, if "2400000" is written to
"/sys/.../cpufreq/scaling_setspeed" to set the frequency, then the
value in "/sys/.../cpuinfo_cur_freq" may be "2401222". So need to
round the value.

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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index 731c6b4dc..1f4d8bb05 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -34,6 +34,10 @@ test_power_caps(void)
 #define TEST_POWER_LCORE_INVALID ((unsigned)RTE_MAX_LCORE)
 #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
 
+/* macros used for rounding frequency to nearest 100000 */
+#define TEST_FREQ_ROUNDING_DELTA 50000
+#define TEST_ROUND_FREQ_TO_N_100000 100000
+
 #define TEST_POWER_SYSFILE_CUR_FREQ \
 	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
 
@@ -62,7 +66,17 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
 		goto fail_get_cur_freq;
 	}
 	cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
-	ret = (freqs[idx] == cur_freq ? 0 : -1);
+
+	/* convert the frequency to nearest 100000 value
+	 * Ex: if cur_freq=1396789 then freq_conv=1400000
+	 * Ex: if cur_freq=800030 then freq_conv=800000
+	 */
+	unsigned int freq_conv = 0;
+	freq_conv = (cur_freq + TEST_FREQ_ROUNDING_DELTA)
+				/ TEST_ROUND_FREQ_TO_N_100000;
+	freq_conv = freq_conv * TEST_ROUND_FREQ_TO_N_100000;
+
+	ret = (freqs[idx] == freq_conv ? 0 : -1);
 
 fail_get_cur_freq:
 	fclose(f);
-- 
2.20.1

[dpdk-dev] [PATCH v4 2/2] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

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 | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index 1f4d8bb05..25249ecf5 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"
 
@@ -48,11 +49,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) {
@@ -62,8 +65,24 @@ 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);
 
@@ -78,7 +97,7 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
 
 	ret = (freqs[idx] == freq_conv ? 0 : -1);
 
-fail_get_cur_freq:
+fail_all:
 	fclose(f);
 
 	return ret;
-- 
2.20.1

[dpdk-dev] [PATCH v5 0/2] test/power: fix bugs in cpufreq test

[Richael Zhuang]

v5:
abort the patch about checking -ENOTSUP in turbo test.

Richael Zhuang (2):
  test/power: add delay before checking cpuinfo cur freq
  test/power: round cpuinfo cur freq value in cpufreq autotest

 app/test/test_power_cpufreq.c | 41 ++++++++++++++++++++++++++++++-----
 1 file changed, 36 insertions(+), 5 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

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

Re: [dpdk-dev] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq

[David Hunt]

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.

Re: [dpdk-dev] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq

[David Hunt]

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>

Re: [dpdk-dev] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

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

[dpdk-dev] [PATCH v5 2/2] test/power: round cpuinfo cur freq value in cpufreq autotest

[Richael Zhuang]

The value in "/sys/.../cpuinfo_cur_freq" may not be exactly the
same as what was set. For example, if "2400000" is written to
"/sys/.../cpufreq/scaling_setspeed" to set the frequency, then the
value in "/sys/.../cpuinfo_cur_freq" may be "2401222". So need to
round the value.

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 | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index d47b3e0a1..f753d24ac 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -35,6 +35,10 @@ test_power_caps(void)
 #define TEST_POWER_LCORE_INVALID ((unsigned)RTE_MAX_LCORE)
 #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
 
+/* macros used for rounding frequency to nearest 100000 */
+#define TEST_FREQ_ROUNDING_DELTA 50000
+#define TEST_ROUND_FREQ_TO_N_100000 100000
+
 #define TEST_POWER_SYSFILE_CUR_FREQ \
 	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
 
@@ -67,7 +71,17 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
 			goto fail_all;
 
 		cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
-		ret = (freqs[idx] == cur_freq ? 0 : -1);
+
+		/* convert the frequency to nearest 100000 value
+		 * Ex: if cur_freq=1396789 then freq_conv=1400000
+		 * Ex: if cur_freq=800030 then freq_conv=800000
+		 */
+		unsigned int freq_conv = 0;
+		freq_conv = (cur_freq + TEST_FREQ_ROUNDING_DELTA)
+					/ TEST_ROUND_FREQ_TO_N_100000;
+		freq_conv = freq_conv * TEST_ROUND_FREQ_TO_N_100000;
+
+		ret = (freqs[idx] == freq_conv ? 0 : -1);
 
 		if (ret == 0)
 			break;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v5 2/2] test/power: round cpuinfo cur freq value in cpufreq autotest

[David Hunt]

On 15/4/2021 6:59 AM, Richael Zhuang wrote:
> The value in "/sys/.../cpuinfo_cur_freq" may not be exactly the
> same as what was set. For example, if "2400000" is written to
> "/sys/.../cpufreq/scaling_setspeed" to set the frequency, then the
> value in "/sys/.../cpuinfo_cur_freq" may be "2401222". So need to
> round the value.
>
> 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 | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
> index d47b3e0a1..f753d24ac 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -35,6 +35,10 @@ test_power_caps(void)
>   #define TEST_POWER_LCORE_INVALID ((unsigned)RTE_MAX_LCORE)
>   #define TEST_POWER_FREQS_NUM_MAX ((unsigned)RTE_MAX_LCORE_FREQS)
>   
> +/* macros used for rounding frequency to nearest 100000 */
> +#define TEST_FREQ_ROUNDING_DELTA 50000
> +#define TEST_ROUND_FREQ_TO_N_100000 100000
> +
>   #define TEST_POWER_SYSFILE_CUR_FREQ \
>   	"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq"
>   
> @@ -67,7 +71,17 @@ check_cur_freq(unsigned lcore_id, uint32_t idx)
>   			goto fail_all;
>   
>   		cur_freq = strtoul(buf, NULL, TEST_POWER_CONVERT_TO_DECIMAL);
> -		ret = (freqs[idx] == cur_freq ? 0 : -1);
> +
> +		/* convert the frequency to nearest 100000 value
> +		 * Ex: if cur_freq=1396789 then freq_conv=1400000
> +		 * Ex: if cur_freq=800030 then freq_conv=800000
> +		 */
> +		unsigned int freq_conv = 0;
> +		freq_conv = (cur_freq + TEST_FREQ_ROUNDING_DELTA)
> +					/ TEST_ROUND_FREQ_TO_N_100000;
> +		freq_conv = freq_conv * TEST_ROUND_FREQ_TO_N_100000;
> +
> +		ret = (freqs[idx] == freq_conv ? 0 : -1);
>   
>   		if (ret == 0)
>   			break;


Hi Richael,

    I played around with this, rounds nicely. I found an unrelated issue 
around turbo mode that I need to look at but this patch looks fine.

Reviewed-by: David Hunt <david.hunt@intel.com>

Re: [dpdk-dev] [PATCH v5 0/2] test/power: fix bugs in cpufreq test

[Thomas Monjalon]

Any review please?

15/04/2021 07:59, Richael Zhuang:
> v5:
> abort the patch about checking -ENOTSUP in turbo test.
> 
> Richael Zhuang (2):
>   test/power: add delay before checking cpuinfo cur freq
>   test/power: round cpuinfo cur freq value in cpufreq autotest
> 
>  app/test/test_power_cpufreq.c | 41 ++++++++++++++++++++++++++++++-----
>  1 file changed, 36 insertions(+), 5 deletions(-)

Re: [dpdk-dev] [PATCH v5 0/2] test/power: fix bugs in cpufreq test

[Thomas Monjalon]

> Richael Zhuang (2):
>   test/power: add delay before checking cpuinfo cur freq
>   test/power: round cpuinfo cur freq value in cpufreq autotest

Applied, thanks

[dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

For platforms that don't support turbo boost,rte_power_turbo_status()
returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
check_power_turbo() to continue if rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1

Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
Cc: lukaszx.krakowiak@intel.com
Cc: stable@dpdk.org

Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
---
 app/test/test_power_cpufreq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index 1f4d8bb05..cda74bd8a 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -386,7 +386,7 @@ check_power_turbo(void)
 {
 	int ret;
 
-	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
+	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
 		printf("Turbo not available on lcore %u, skipping test\n",
 				TEST_POWER_LCORE_ID);
 		return 0;
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Burakov, Anatoly]

On 07-Apr-21 8:46 AM, Richael Zhuang wrote:
> For platforms that don't support turbo boost,rte_power_turbo_status()
> returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
> check_power_turbo() to continue if rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1
> 
> Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
> Cc: lukaszx.krakowiak@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> ---
>   app/test/test_power_cpufreq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
> index 1f4d8bb05..cda74bd8a 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -386,7 +386,7 @@ check_power_turbo(void)
>   {
>   	int ret;
>   
> -	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
> +	if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
>   		printf("Turbo not available on lcore %u, skipping test\n",
>   				TEST_POWER_LCORE_ID);
>   		return 0;
> 

If what you're really checking is -ENOTSUP, maybe check for that? 
Because otherwise it seems like you're making unwarranted assumptions 
about why the call failed...

-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

Hi,
Thanks for your comments.
My change  is to make  the check_power_turbo()  to continue only when rte_power_turbo_status(TEST_POWER_LCORE_ID) returns 1 which means turbo is available.

-----Original Message-----
From: Burakov, Anatoly <anatoly.burakov@intel.com>
Sent: Wednesday, April 7, 2021 5:59 PM
To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt <david.hunt@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

On 07-Apr-21 8:46 AM, Richael Zhuang wrote:
> For platforms that don't support turbo boost,rte_power_turbo_status()
> returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
> check_power_turbo() to continue if
> rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1
>
> Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
> Cc: lukaszx.krakowiak@intel.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> ---
>   app/test/test_power_cpufreq.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/app/test/test_power_cpufreq.c
> b/app/test/test_power_cpufreq.c index 1f4d8bb05..cda74bd8a 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -386,7 +386,7 @@ check_power_turbo(void)
>   {
>   int ret;
>
> -if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
> +if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
>   printf("Turbo not available on lcore %u, skipping test\n",
>   TEST_POWER_LCORE_ID);
>   return 0;
>

If what you're really checking is -ENOTSUP, maybe check for that?
Because otherwise it seems like you're making unwarranted assumptions about why the call failed...

--
Thanks,
Anatoly
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Burakov, Anatoly]

On 08-Apr-21 3:10 AM, Richael Zhuang wrote:
> Hi,
> Thanks for your comments.
> My change  is to make  the check_power_turbo()  to continue only when rte_power_turbo_status(TEST_POWER_LCORE_ID) returns 1 which means turbo is available.

Sure, but the code reads like if the turbo status isn't available, then 
the code just treats it as "unsupported", where in reality it could've 
been supported, but failed for some other reason.

> 
> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Wednesday, April 7, 2021 5:59 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest
> 
> On 07-Apr-21 8:46 AM, Richael Zhuang wrote:
>> For platforms that don't support turbo boost,rte_power_turbo_status()
>> returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
>> check_power_turbo() to continue if
>> rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1
>>
>> Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
>> Cc: lukaszx.krakowiak@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
>> ---
>>    app/test/test_power_cpufreq.c | 2 +-
>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/app/test/test_power_cpufreq.c
>> b/app/test/test_power_cpufreq.c index 1f4d8bb05..cda74bd8a 100644
>> --- a/app/test/test_power_cpufreq.c
>> +++ b/app/test/test_power_cpufreq.c
>> @@ -386,7 +386,7 @@ check_power_turbo(void)
>>    {
>>    int ret;
>>
>> -if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
>> +if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
>>    printf("Turbo not available on lcore %u, skipping test\n",
>>    TEST_POWER_LCORE_ID);
>>    return 0;
>>
> 
> If what you're really checking is -ENOTSUP, maybe check for that?
> Because otherwise it seems like you're making unwarranted assumptions about why the call failed...
> 
> --
> Thanks,
> Anatoly
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> 


-- 
Thanks,
Anatoly

Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Thursday, April 8, 2021 10:56 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt
> <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq
> autotest
> 
> On 08-Apr-21 3:10 AM, Richael Zhuang wrote:
> > Hi,
> > Thanks for your comments.
> > My change  is to make  the check_power_turbo()  to continue only when
> rte_power_turbo_status(TEST_POWER_LCORE_ID) returns 1 which means
> turbo is available.
> 
> Sure, but the code reads like if the turbo status isn't available, then the code
> just treats it as "unsupported", where in reality it could've been supported,
> but failed for some other reason.
> 
Sounds reasonable. I will rework it, thanks.
> >
> > -----Original Message-----
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > Sent: Wednesday, April 7, 2021 5:59 PM
> > To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> > Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt
> > <david.hunt@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in
> > cpufreq autotest
> >
> > On 07-Apr-21 8:46 AM, Richael Zhuang wrote:
> >> For platforms that don't support turbo boost,rte_power_turbo_status()
> >> returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
> >> check_power_turbo() to continue if
> >> rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1
> >>
> >> Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
> >> Cc: lukaszx.krakowiak@intel.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> >> ---
> >>    app/test/test_power_cpufreq.c | 2 +-
> >>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/app/test/test_power_cpufreq.c
> >> b/app/test/test_power_cpufreq.c index 1f4d8bb05..cda74bd8a 100644
> >> --- a/app/test/test_power_cpufreq.c
> >> +++ b/app/test/test_power_cpufreq.c
> >> @@ -386,7 +386,7 @@ check_power_turbo(void)
> >>    {
> >>    int ret;
> >>
> >> -if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
> >> +if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
> >>    printf("Turbo not available on lcore %u, skipping test\n",
> >>    TEST_POWER_LCORE_ID);
> >>    return 0;
> >>
> >
> > If what you're really checking is -ENOTSUP, maybe check for that?
> > Because otherwise it seems like you're making unwarranted assumptions
> about why the call failed...
> >
> > --
> > Thanks,
> > Anatoly
> > IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.
> >
> 
> 
> --
> Thanks,
> Anatoly

Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq autotest

[Richael Zhuang]

> -----Original Message-----
> From: Burakov, Anatoly <anatoly.burakov@intel.com>
> Sent: Thursday, April 8, 2021 10:56 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt
> <david.hunt@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in cpufreq
> autotest
> 
> On 08-Apr-21 3:10 AM, Richael Zhuang wrote:
> > Hi,
> > Thanks for your comments.
> > My change  is to make  the check_power_turbo()  to continue only when
> rte_power_turbo_status(TEST_POWER_LCORE_ID) returns 1 which means
> turbo is available.
> 
> Sure, but the code reads like if the turbo status isn't available, then the code
> just treats it as "unsupported", where in reality it could've been supported,
> but failed for some other reason.
> 
Hi Anatoly,
I'm thinking if it's better to abandon this patch.
 For KVM case, although rte_power_turbo_status() returns -ENOTSUP, the check should continue  because rte_power_freq_enable_turbo() and  rte_power_freq_disable_turbo() are  available in power_kvm_vm.c .

Originally I added this patch considering there is case that turbo is not supported on some platforms and all turbo related functions return -ENOTSUP.   

Best Regards
> >
> > -----Original Message-----
> > From: Burakov, Anatoly <anatoly.burakov@intel.com>
> > Sent: Wednesday, April 7, 2021 5:59 PM
> > To: Richael Zhuang <Richael.Zhuang@arm.com>; dev@dpdk.org
> > Cc: lukaszx.krakowiak@intel.com; stable@dpdk.org; David Hunt
> > <david.hunt@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 2/3] test/power: fix a bug in
> > cpufreq autotest
> >
> > On 07-Apr-21 8:46 AM, Richael Zhuang wrote:
> >> For platforms that don't support turbo boost,rte_power_turbo_status()
> >> returns "-ENOTSUP" (like power_kvm_vm_turbo_status()). So don't allow
> >> check_power_turbo() to continue if
> >> rte_power_turbo_status(TEST_POWER_LCORE_ID)!=1
> >>
> >> Fixes: aeaeaf5f2d62 ("test/power: add cases for turbo feature")
> >> Cc: lukaszx.krakowiak@intel.com
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Richael Zhuang <richael.zhuang@arm.com>
> >> ---
> >>    app/test/test_power_cpufreq.c | 2 +-
> >>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/app/test/test_power_cpufreq.c
> >> b/app/test/test_power_cpufreq.c index 1f4d8bb05..cda74bd8a 100644
> >> --- a/app/test/test_power_cpufreq.c
> >> +++ b/app/test/test_power_cpufreq.c
> >> @@ -386,7 +386,7 @@ check_power_turbo(void)
> >>    {
> >>    int ret;
> >>
> >> -if (rte_power_turbo_status(TEST_POWER_LCORE_ID) == 0) {
> >> +if (rte_power_turbo_status(TEST_POWER_LCORE_ID) != 1) {
> >>    printf("Turbo not available on lcore %u, skipping test\n",
> >>    TEST_POWER_LCORE_ID);
> >>    return 0;
> >>
> >
> > If what you're really checking is -ENOTSUP, maybe check for that?
> > Because otherwise it seems like you're making unwarranted assumptions
> about why the call failed...
> >
> > --
> > Thanks,
> > Anatoly
> >
> 
> 
> --
> Thanks,
> Anatoly

[dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

Sleep for 1s before checking the newly updated value from
"/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because
for some systems it may not be effective immediately.

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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
index cda74bd8a..7a93bc90a 100644
--- a/app/test/test_power_cpufreq.c
+++ b/app/test/test_power_cpufreq.c
@@ -47,6 +47,9 @@ static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
 static int
 check_cur_freq(unsigned lcore_id, uint32_t idx)
 {
+	/* wait for the value to be updated */
+	sleep(1);
+
 #define TEST_POWER_CONVERT_TO_DECIMAL 10
 	FILE *f;
 	char fullpath[PATH_MAX];
-- 
2.20.1

Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Liang Ma]

On Wed, Apr 07, 2021 at 03:46:36PM +0800, Richael Zhuang wrote:
> Sleep for 1s before checking the newly updated value from
> "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because
> for some systems it may not be effective immediately.
> 
> 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 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/app/test/test_power_cpufreq.c b/app/test/test_power_cpufreq.c
> index cda74bd8a..7a93bc90a 100644
> --- a/app/test/test_power_cpufreq.c
> +++ b/app/test/test_power_cpufreq.c
> @@ -47,6 +47,9 @@ static uint32_t freqs[TEST_POWER_FREQS_NUM_MAX];
>  static int
>  check_cur_freq(unsigned lcore_id, uint32_t idx)
>  {
> +	/* wait for the value to be updated */
> +	sleep(1);
Hi Richael, 
  1 second looks way too much for CPU frequency  swap.
  The unit should be ms in the worst case regardless the vendor, IMO. 
Regards
Liang
> +
>  #define TEST_POWER_CONVERT_TO_DECIMAL 10
>  	FILE *f;
>  	char fullpath[PATH_MAX];
> -- 
> 2.20.1
> 

Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

Hi Liang,

> -----Original Message-----
> From: Liang Ma <liangma@liangbit.com>
> Sent: Wednesday, April 7, 2021 6:15 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>
> Cc: dev@dpdk.org; alan.carew@intel.com; stable@dpdk.org; David Hunt
> <david.hunt@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before
> checking cpuinfo cur freq
>
> On Wed, Apr 07, 2021 at 03:46:36PM +0800, Richael Zhuang wrote:
> > Sleep for 1s before checking the newly updated value from
> > "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because for
> > some systems it may not be effective immediately.
> >
> > 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 | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/app/test/test_power_cpufreq.c
> > b/app/test/test_power_cpufreq.c index cda74bd8a..7a93bc90a 100644
> > --- a/app/test/test_power_cpufreq.c
> > +++ b/app/test/test_power_cpufreq.c
> > @@ -47,6 +47,9 @@ static uint32_t
> freqs[TEST_POWER_FREQS_NUM_MAX];
> > static int  check_cur_freq(unsigned lcore_id, uint32_t idx)  {
> > +/* wait for the value to be updated */
> > +sleep(1);
> Hi Richael,
>   1 second looks way too much for CPU frequency  swap.
>   The unit should be ms in the worst case regardless the vendor, IMO.
> Regards
> Liang
Thanks for review. Although I also think this time seems too long,  it needs more than 700ms when I tested on our arm platform.
> > +
> >  #define TEST_POWER_CONVERT_TO_DECIMAL 10
> >  FILE *f;
> >  char fullpath[PATH_MAX];
> > --
> > 2.20.1
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

Hi Liang,
Sorry that last email contains "confidential notice", so I resend it.
> -----Original Message-----
> From: Liang Ma <liangma@liangbit.com>
> Sent: Wednesday, April 7, 2021 6:15 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>
> Cc: dev@dpdk.org; alan.carew@intel.com; stable@dpdk.org; David Hunt
> <david.hunt@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before
> checking cpuinfo cur freq
> On Wed, Apr 07, 2021 at 03:46:36PM +0800, Richael Zhuang wrote:
> > Sleep for 1s before checking the newly updated value from
> > "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because for
> > some systems it may not be effective immediately.
> >
> > 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 | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/app/test/test_power_cpufreq.c
> > b/app/test/test_power_cpufreq.c index cda74bd8a..7a93bc90a 100644
> > --- a/app/test/test_power_cpufreq.c
> > +++ b/app/test/test_power_cpufreq.c
> > @@ -47,6 +47,9 @@ static uint32_t
> freqs[TEST_POWER_FREQS_NUM_MAX];
> > static int  check_cur_freq(unsigned lcore_id, uint32_t idx)  {
> > +/* wait for the value to be updated */
> > +sleep(1);
> Hi Richael,
>   1 second looks way too much for CPU frequency  swap.
>   The unit should be ms in the worst case regardless the vendor, IMO.
> Regards
> Liang
Thanks for review. Although I also think this time seems too long,
it needs more than 700ms when I tested on our arm platform.
> > +
> >  #define TEST_POWER_CONVERT_TO_DECIMAL 10
> >  FILE *f;
> >  char fullpath[PATH_MAX];
> > --
> > 2.20.1
> >
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Liang Ma]

On Thu, Apr 08, 2021 at 05:16:34AM +0000, Richael Zhuang wrote:
> Hi Liang,
> Sorry that last email contains "confidential notice", so I resend it.
> > -----Original Message-----
> > From: Liang Ma <liangma@liangbit.com>
> > Sent: Wednesday, April 7, 2021 6:15 PM
> > To: Richael Zhuang <Richael.Zhuang@arm.com>
> > Cc: dev@dpdk.org; alan.carew@intel.com; stable@dpdk.org; David Hunt
> > <david.hunt@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before
> > checking cpuinfo cur freq
> > On Wed, Apr 07, 2021 at 03:46:36PM +0800, Richael Zhuang wrote:
> > > Sleep for 1s before checking the newly updated value from
> > > "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because for
> > > some systems it may not be effective immediately.
> > >
> > > 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 | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/app/test/test_power_cpufreq.c
> > > b/app/test/test_power_cpufreq.c index cda74bd8a..7a93bc90a 100644
> > > --- a/app/test/test_power_cpufreq.c
> > > +++ b/app/test/test_power_cpufreq.c
> > > @@ -47,6 +47,9 @@ static uint32_t
> > freqs[TEST_POWER_FREQS_NUM_MAX];
> > > static int  check_cur_freq(unsigned lcore_id, uint32_t idx)  {
> > > +/* wait for the value to be updated */
> > > +sleep(1);
> > Hi Richael,
> >   1 second looks way too much for CPU frequency  swap.
> >   The unit should be ms in the worst case regardless the vendor, IMO.
> > Regards
> > Liang
> Thanks for review. Although I also think this time seems too long,
> it needs more than 700ms when I tested on our arm platform.
Hi Richael, 
   I will suggest you talk with arm cpufreq driver maintainer(kernel). 
   I don't think HW need 700ms to complete frequency changes. 
   cpufreq driver might do something here. intel_pstate driver set the
   threshold as 10ms, according to the kernel source code.  
Regards
Liang
> > > +
> > >  #define TEST_POWER_CONVERT_TO_DECIMAL 10
> > >  FILE *f;
> > >  char fullpath[PATH_MAX];
> > > --
> > > 2.20.1
> > >
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before checking cpuinfo cur freq

[Richael Zhuang]

> -----Original Message-----
> From: Liang Ma <liangma@liangbit.com>
> Sent: Thursday, April 8, 2021 3:55 PM
> To: Richael Zhuang <Richael.Zhuang@arm.com>
> Cc: dev@dpdk.org; alan.carew@intel.com; stable@dpdk.org; David Hunt
> <david.hunt@intel.com>; Pablo de Lara <pablo.de.lara.guarch@intel.com>;
> nd <nd@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before
> checking cpuinfo cur freq
>
> On Thu, Apr 08, 2021 at 05:16:34AM +0000, Richael Zhuang wrote:
> > Hi Liang,
> > Sorry that last email contains "confidential notice", so I resend it.
> > > -----Original Message-----
> > > From: Liang Ma <liangma@liangbit.com>
> > > Sent: Wednesday, April 7, 2021 6:15 PM
> > > To: Richael Zhuang <Richael.Zhuang@arm.com>
> > > Cc: dev@dpdk.org; alan.carew@intel.com; stable@dpdk.org; David Hunt
> > > <david.hunt@intel.com>; Pablo de Lara
> > > <pablo.de.lara.guarch@intel.com>
> > > Subject: Re: [dpdk-dev] [PATCH v3 3/3] test/power: add delay before
> > > checking cpuinfo cur freq On Wed, Apr 07, 2021 at 03:46:36PM +0800,
> > > Richael Zhuang wrote:
> > > > Sleep for 1s before checking the newly updated value from
> > > > "/sys/devices/system/cpu/cpu%u/cpufreq/cpuinfo_cur_freq", because
> > > > for some systems it may not be effective immediately.
> > > >
> > > > 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 | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/app/test/test_power_cpufreq.c
> > > > b/app/test/test_power_cpufreq.c index cda74bd8a..7a93bc90a 100644
> > > > --- a/app/test/test_power_cpufreq.c
> > > > +++ b/app/test/test_power_cpufreq.c
> > > > @@ -47,6 +47,9 @@ static uint32_t
> > > freqs[TEST_POWER_FREQS_NUM_MAX];
> > > > static int  check_cur_freq(unsigned lcore_id, uint32_t idx)  {
> > > > +/* wait for the value to be updated */ sleep(1);
> > > Hi Richael,
> > >   1 second looks way too much for CPU frequency  swap.
> > >   The unit should be ms in the worst case regardless the vendor, IMO.
> > > Regards
> > > Liang
> > Thanks for review. Although I also think this time seems too long, it
> > needs more than 700ms when I tested on our arm platform.
> Hi Richael,
>    I will suggest you talk with arm cpufreq driver maintainer(kernel).
>    I don't think HW need 700ms to complete frequency changes.
>    cpufreq driver might do something here. intel_pstate driver set the
>    threshold as 10ms, according to the kernel source code.
> Regards
> Liang
Hi Liang,
Thanks, I will check this with the driver maintainer.
Regards,
Richael
> > > > +
> > > >  #define TEST_POWER_CONVERT_TO_DECIMAL 10  FILE *f;  char
> > > > fullpath[PATH_MAX];
> > > > --
> > > > 2.20.1
> > > >

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.