Re: [dpdk-dev] [PATCH 2/2] power: check if userspace governor is available

[Radoslaw Biernacki <radoslaw.biernacki@linaro.org>]

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.
>