diff mbox

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

Message ID 1508161628-4265-2-git-send-email-radoslaw.biernacki@linaro.org (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK

Commit Message

Radoslaw Biernacki Oct. 16, 2017, 1:47 p.m. UTC 
  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(-)

Comments

Hunt, David Oct. 18, 2017, 10:53 a.m. UTC | #1 
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.
Radoslaw Biernacki Oct. 18, 2017, 2:02 p.m. UTC | #2 
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 mbox

Patch

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");