| Message ID | 1508161628-4265-2-git-send-email-radoslaw.biernacki@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] power: switching to unbuffered stdio for /sys file access | expand |
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 <radoslaw.biernacki@linaro.org> > --- > 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.
Hi David, On 18 October 2017 at 12:53, Hunt, David <david.hunt@intel.com> 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 <radoslaw.biernacki@linaro.org> >> --- >> 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. >
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) @@ -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");
Since for new Intel CPU's kernel use intel_pstate driver, which does not offer userspace governor, it is vise to check 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. Signed-off-by: Radoslaw Biernacki <radoslaw.biernacki@linaro.org> --- lib/librte_power/rte_power_acpi_cpufreq.c | 35 ++++++++++++++++++++++++------- 1 file changed, 27 insertions(+), 8 deletions(-) -- 2.7.4