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.ca...@intel.com
Cc: sta...@dpdk.org

Signed-off-by: Richael Zhuang <richael.zhu...@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.h...@intel.com>