Re: [dpdk-stable] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq

[David Hunt <david.hunt@intel.com>]

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.carew@intel.com
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Richael Zhuang <richael.zhuang@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.hunt@intel.com>