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

["Hunt, David" <david.hunt@intel.com>]

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.