> -----Original Message----- > From: David Hunt <[email protected]> > Sent: Tuesday, April 20, 2021 8:39 PM > To: Richael Zhuang <[email protected]>; [email protected] > Cc: nd <[email protected]>; [email protected]; [email protected]; Pablo de > Lara <[email protected]> > 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: [email protected] > > Cc: [email protected] > > > > Signed-off-by: Richael Zhuang <[email protected]> > > --- > > 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
