[v3,2/3] test/power: fix a bug in cpufreq autotest
Checks
Commit Message
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
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...
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.
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.
>
> -----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
> -----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
@@ -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;