From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f172.google.com (mail-wr0-f172.google.com [209.85.128.172]) by dpdk.org (Postfix) with ESMTP id 4E092378B for ; Wed, 18 Oct 2017 16:02:59 +0200 (CEST) Received: by mail-wr0-f172.google.com with SMTP id q42so5156526wrb.7 for ; Wed, 18 Oct 2017 07:02:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=qQhTZSDxIAoCLiMdwyxe4ct2cnnHLW0Igp00339Vjhs=; b=Q1Q10b5a4XpK8pSXEb2Z7yJN1dikSLtImWxSxu9tN2iwr5eaStbhXlSorK09/s4kEz ycjRwvJUvVcnyuX+32V1ec8SW71/cKuNFXb4frpuhoAxtl8C0HOTUs40Ol4NjEJByT4U DE57r5hKKNBPuh4ADrjNN7So382tfmX1EDRy8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=qQhTZSDxIAoCLiMdwyxe4ct2cnnHLW0Igp00339Vjhs=; b=f9fsFl0EA2+oqF2ywuQfxw3zWeOrLF3lyfBbcj4kQfP1IsUBRYrPHxKL2mlVNAl8N/ N7WjRnYqRgbqzLW++IZXTEaDst+6ANatEEAFw4yuBijbEnLGz6HbXFU3jbrFTWmUvAbF KbSwccDKehYloQfeS/JSsJEXYY+xZfY/S95/wiDMJlz4RFHozOW16Nq9CIVCAQafStOY tCW+nLlI9Hf/IDNGStltbEBG4qv2fz73xYVlhiIVfJfcEcj2L6TWc/KoZoAqcZ1B70d7 DckeyBLpEdSkOd2mWdsPtr8Scc1PdMawdLyRF+s5nWRppt+nKUEettLcBAn9xtFPzMjq J9vA== X-Gm-Message-State: AMCzsaVN9f1GFeS5kk4oRVzR9NekpibqqzQ6aG2xh+H5DNQz/yjwKAL0 VnqFrXoiv/SyTOLt9svfoDBJ5y4ec6Nkuakz8Oe+QA== X-Google-Smtp-Source: ABhQp+RGidyqxoZaak3RN959lAlLbr5OeVkCCfDPzPOeRxGqmzYEleKAb+L2BSSEbXf6zPI/IV9TUZkWXmbMTSXsBOY= X-Received: by 10.223.166.181 with SMTP id t50mr6695554wrc.251.1508335379578; Wed, 18 Oct 2017 07:02:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.144.134 with HTTP; Wed, 18 Oct 2017 07:02:39 -0700 (PDT) In-Reply-To: References: <1508161628-4265-1-git-send-email-radoslaw.biernacki@linaro.org> <1508161628-4265-2-git-send-email-radoslaw.biernacki@linaro.org> From: Radoslaw Biernacki Date: Wed, 18 Oct 2017 16:02:39 +0200 Message-ID: To: "Hunt, David" Cc: dev@dpdk.org, stable@dpdk.org, alan.carew@intel.com, pablo.de.lara.guarch@intel.com Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 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 14:03:00 -0000 Hi David, On 18 October 2017 at 12:53, Hunt, David wrote: > 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. Yes, will do. This will save time of DPDK users trying to figure out the problem. > > > 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". Sure, will fix those as well in V2 > > > @@ -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. >