From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1CBF1A0548; Tue, 20 Apr 2021 15:15:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id DF64E41749; Tue, 20 Apr 2021 15:15:26 +0200 (CEST) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 22D1B41735; Tue, 20 Apr 2021 15:15:24 +0200 (CEST) IronPort-SDR: y+lFBqdLRxo1mcrfCMkwGJPd0gJwGQeISJOsjHwcSDJ8zQJT8wrVOsk3Wr8TajSSQr6RFBsJeb Yy4gCgiCwI0Q== X-IronPort-AV: E=McAfee;i="6200,9189,9960"; a="259459067" X-IronPort-AV: E=Sophos;i="5.82,237,1613462400"; d="scan'208";a="259459067" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2021 06:15:23 -0700 IronPort-SDR: NYw+/ldd+QMhcdB0wTlKf7Gvso39uu4q6GZMRwfMPpombFXi5/UtOK022tIxREtQFqoygsLUQE 769TmpbqwXWQ== X-IronPort-AV: E=Sophos;i="5.82,237,1613462400"; d="scan'208";a="426922472" Received: from dhunt5-mobl5.ger.corp.intel.com (HELO [10.252.30.174]) ([10.252.30.174]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 20 Apr 2021 06:15:21 -0700 From: David Hunt To: Richael Zhuang , dev@dpdk.org Cc: nd@arm.com, alan.carew@intel.com, stable@dpdk.org, Pablo de Lara References: <20210407074636.26891-2-richael.zhuang@arm.com> <20210415055930.3899-1-richael.zhuang@arm.com> <20210415055930.3899-2-richael.zhuang@arm.com> <86f24139-a638-ec83-ab52-fa19a28dd658@intel.com> Message-ID: <7b20f122-ddd2-11fb-68af-dc3ba1da2dbd@intel.com> Date: Tue, 20 Apr 2021 14:15:19 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.1 MIME-Version: 1.0 In-Reply-To: <86f24139-a638-ec83-ab52-fa19a28dd658@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [dpdk-dev] [PATCH v5 1/2] test/power: add delay before checking cpuinfo cur freq X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> --- >>   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 >>   #include >>   #include >> +#include >>     #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