From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 916CA2B9F; Wed, 18 Oct 2017 12:53:18 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Oct 2017 03:53:17 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,396,1503385200"; d="scan'208";a="1207113506" Received: from dhunt5-mobl1.ger.corp.intel.com (HELO [10.237.220.44]) ([10.237.220.44]) by fmsmga001.fm.intel.com with ESMTP; 18 Oct 2017 03:53:14 -0700 To: Radoslaw Biernacki , dev@dpdk.org Cc: stable@dpdk.org, alan.carew@intel.com, pablo.de.lara.guarch@intel.com References: <1508161628-4265-1-git-send-email-radoslaw.biernacki@linaro.org> <1508161628-4265-2-git-send-email-radoslaw.biernacki@linaro.org> From: "Hunt, David" Message-ID: Date: Wed, 18 Oct 2017 11:53:13 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1508161628-4265-2-git-send-email-radoslaw.biernacki@linaro.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-Language: en-US Subject: Re: [dpdk-dev] [PATCH 2/2] power: check if userspace governor is available X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Oct 2017 10:53:19 -0000 Hi Radoslaw, On 16/10/2017 2:47 PM, Radoslaw Biernacki wrote: > Since for new Intel CPU's kernel use intel_pstate driver, > which does not offer userspace governor, it is vise to check typo here, "wise" > the userspace governor availability before trying to perform > governor switch. The outcome from this patch is direct > information for user about the root cause of failure during > the rte_power_acpi_cpufreq_init(). This patch also add the > /sys file name to error message as on some platforms some > files expected by this rte_power are not available. This is > also useful for pinning down the root cause of the problem. Good suggestion. It's a useful change. What would you think of also adding a check of the contents of the "scaling_driver" file? This would indicate the reason why the userspace governor is not available. If it's "intel_pstate", then we could suggest to the user that "intel_pstate=disable" needs to be added to the kernel boot parameters. > Signed-off-by: Radoslaw Biernacki > --- > lib/librte_power/rte_power_acpi_cpufreq.c | 35 ++++++++++++++++++++++++------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/lib/librte_power/rte_power_acpi_cpufreq.c b/lib/librte_power/rte_power_acpi_cpufreq.c > index 8bf5685..342eb22 100644 > --- a/lib/librte_power/rte_power_acpi_cpufreq.c > +++ b/lib/librte_power/rte_power_acpi_cpufreq.c > @@ -55,9 +55,9 @@ > #define POWER_DEBUG_TRACE(fmt, args...) > #endif > > -#define FOPEN_OR_ERR_RET(f, retval) do { \ > +#define FOPEN_OR_ERR_RET(f, fullpath, retval) do { \ > if ((f) == NULL) { \ > - RTE_LOG(ERR, POWER, "File not openned\n"); \ > + RTE_LOG(ERR, POWER, "File %s not openned\n", fullpath); \ > return retval; \ > } \ > } while (0) There's a checkpatch error here. I've encountered this one a few times in the past, and I don't believe it can be fixed with the macro in it's current form. Later versions of checkpatch does not like control statements in macros, so the only way to fix this checkpatch problem is to remove the macro, and put the 'if' statement back into the code instead of calling the macro. As well as fixing the checkpatch issue, I feel this would have the added advantage of making the code more readable. Also, I would suggest changing "File %s not openned\n" to "Failed to open %s.\n". > @@ -80,6 +80,8 @@ > #define POWER_CONVERT_TO_DECIMAL 10 > > #define POWER_GOVERNOR_USERSPACE "userspace" > +#define POWER_SYSFILE_AVAIL_GOVERNOR \ > + "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_available_governors" > #define POWER_SYSFILE_GOVERNOR \ > "/sys/devices/system/cpu/cpu%u/cpufreq/scaling_governor" > #define POWER_SYSFILE_AVAIL_FREQ \ > @@ -170,10 +172,28 @@ power_set_governor_userspace(struct rte_power_info *pi) > char *s; > int val; > > + /* check if userspace governor is available */ > + > + snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_GOVERNOR, > + pi->lcore_id); > + f = fopen(fullpath, "r"); > + FOPEN_OR_ERR_RET(f, fullpath, ret); > + > + s = fgets(buf, sizeof(buf), f); > + FOPS_OR_NULL_GOTO(s, out); > + > + if (!strstr(buf, POWER_GOVERNOR_USERSPACE)) { > + RTE_LOG(ERR, POWER, "Userspace governor is not available\n"); > + goto out; > + } > + fclose(f); > + > + /* store current governor and set userspace governor */ > + > snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR, > pi->lcore_id); > f = fopen(fullpath, "rw+"); > - FOPEN_OR_ERR_RET(f, ret); > + FOPEN_OR_ERR_RET(f, fullpath, ret); > > if (setvbuf(f, NULL, _IONBF, 0)) { > RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); > @@ -184,8 +204,7 @@ power_set_governor_userspace(struct rte_power_info *pi) > FOPS_OR_NULL_GOTO(s, out); > > /* Check if current governor is userspace */ > - if (strncmp(buf, POWER_GOVERNOR_USERSPACE, > - sizeof(POWER_GOVERNOR_USERSPACE)) == 0) { > + if (!strcmp(buf, POWER_GOVERNOR_USERSPACE)) { > ret = 0; > POWER_DEBUG_TRACE("Power management governor of lcore %u is " > "already userspace\n", pi->lcore_id); > @@ -228,7 +247,7 @@ power_get_available_freqs(struct rte_power_info *pi) > snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_AVAIL_FREQ, > pi->lcore_id); > f = fopen(fullpath, "r"); > - FOPEN_OR_ERR_RET(f, ret); > + FOPEN_OR_ERR_RET(f, fullpath, ret); > > s = fgets(buf, sizeof(buf), f); > FOPS_OR_NULL_GOTO(s, out); > @@ -296,7 +315,7 @@ power_init_for_setting_freq(struct rte_power_info *pi) > snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_SETSPEED, > pi->lcore_id); > f = fopen(fullpath, "rw+"); > - FOPEN_OR_ERR_RET(f, -1); > + FOPEN_OR_ERR_RET(f, fullpath, -1); > > if (setvbuf(f, NULL, _IONBF, 0)) { > RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); > @@ -398,7 +417,7 @@ power_set_governor_original(struct rte_power_info *pi) > snprintf(fullpath, sizeof(fullpath), POWER_SYSFILE_GOVERNOR, > pi->lcore_id); > f = fopen(fullpath, "rw+"); > - FOPEN_OR_ERR_RET(f, ret); > + FOPEN_OR_ERR_RET(f, fullpath, ret); > > if (setvbuf(f, NULL, _IONBF, 0)) { > RTE_LOG(ERR, POWER, "Cannot set unbuffered mode\n"); Thanks, Dave.