Hi David,
On 18 October 2017 at 12:53, Hunt, David <david.h...@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.bierna...@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.
>
