diff mbox series

[v3,2/3] test/power: fix a bug in cpufreq autotest

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

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Richael Zhuang April 7, 2021, 7:46 a.m. UTC 
  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(-)

Comments

Burakov, Anatoly April 7, 2021, 9:58 a.m. UTC | #1 
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...
Richael Zhuang April 8, 2021, 2:10 a.m. UTC | #2 
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.
Burakov, Anatoly April 8, 2021, 2:55 p.m. UTC | #3 
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.
>
Richael Zhuang April 12, 2021, 2:08 a.m. UTC | #4 
> -----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
Richael Zhuang April 14, 2021, 8:29 a.m. UTC | #5 
> -----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
diff mbox series

Patch

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;