diff mbox series

[v3,3/3] test/power: add delay before checking cpuinfo cur freq

Message ID 20210407074636.26891-4-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 success coding style OK
ci/travis-robot success travis build: passed
ci/github-robot success github build: passed
ci/Intel-compilation success Compilation OK
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

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

Comments

Liang Ma April 7, 2021, 10:14 a.m. UTC | #1 
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
>
Richael Zhuang April 8, 2021, 5:10 a.m. UTC | #2 
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.
Richael Zhuang April 8, 2021, 5:16 a.m. UTC | #3 
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.
Liang Ma April 8, 2021, 7:54 a.m. UTC | #4 
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.
Richael Zhuang April 8, 2021, 9:08 a.m. UTC | #5 
> -----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.
diff mbox series

Patch

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