Re: [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options

[Shuah Khan <skhan@linuxfoundation.org>]

On 7/13/20 1:56 AM, latha@linux.vnet.ibm.com wrote:
> From: Brahadambal Srinivasan <latha@linux.vnet.ibm.com>
> 
> When a user tries to modify cpuidle or cpufreq properties on offline
> CPUs, the tool returns success (exit status 0) but also does not provide
> any warning message regarding offline cpus that may have been specified
> but left unchanged. In case of all or a few CPUs being offline, it can be
> difficult to keep track of which CPUs didn't get the new frequency or idle
> state set. Silent failures are difficult to keep track of when there are a
> huge number of CPUs on which the action is performed.
> 
> This patch adds an additional message if the user attempts to modify
> offline cpus.
> 

The idea is good. A few comments below on implementing it with
duplicated code in cmd_freq_set() and cmd_idle_set().

Please eliminate code duplication as much as possible. Handling
offline_cpus alloc/free similar to cpus_chosen will reduce some
duplication.

> Reported-by: Pavithra R. Prakash <pavrampu@in.ibm.com>
> Signed-off-by: Brahadambal Srinivasan <latha@linux.vnet.ibm.com>
> ---
>   tools/power/cpupower/utils/cpufreq-set.c | 24 ++++++++++++++++++++++--
>   tools/power/cpupower/utils/cpuidle-set.c | 21 ++++++++++++++++++++-
>   2 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/cpufreq-set.c b/tools/power/cpupower/utils/cpufreq-set.c
> index 6ed82fba5aaa..87031120582a 100644
> --- a/tools/power/cpupower/utils/cpufreq-set.c
> +++ b/tools/power/cpupower/utils/cpufreq-set.c
> @@ -195,10 +195,14 @@ int cmd_freq_set(int argc, char **argv)
>   	extern int optind, opterr, optopt;
>   	int ret = 0, cont = 1;
>   	int double_parm = 0, related = 0, policychange = 0;
> +	int str_len = 0;
>   	unsigned long freq = 0;
>   	char gov[20];
> +	char *offline_cpus_str = NULL;
>   	unsigned int cpu;
>   
> +	struct bitmask *offline_cpus = NULL;
> +
>   	struct cpufreq_policy new_pol = {
>   		.min = 0,
>   		.max = 0,
> @@ -311,14 +315,21 @@ int cmd_freq_set(int argc, char **argv)
>   		}
>   	}
>   
> +	offline_cpus = bitmask_alloc(sysconf(_SC_NPROCESSORS_CONF));
> +	str_len = sysconf(_SC_NPROCESSORS_CONF) * 5;
> +	offline_cpus_str = malloc(sizeof(char) * str_len);
>   

Allocate once when cpus_chosen is allocated.

>   	/* loop over CPUs */
>   	for (cpu = bitmask_first(cpus_chosen);
>   	     cpu <= bitmask_last(cpus_chosen); cpu++) {
>   
> -		if (!bitmask_isbitset(cpus_chosen, cpu) ||
> -		    cpupower_is_cpu_online(cpu) != 1)
> +		if (!bitmask_isbitset(cpus_chosen, cpu))
> +			continue;
> +
> +		if (cpupower_is_cpu_online(cpu) != 1) {
> +			bitmask_setbit(offline_cpus, cpu);
>   			continue;
> +		}
>   
>   		printf(_("Setting cpu: %d\n"), cpu);
>   		ret = do_one_cpu(cpu, &new_pol, freq, policychange);
> @@ -328,5 +339,14 @@ int cmd_freq_set(int argc, char **argv)
>   		}
>   	}
>   
> +	if (!bitmask_isallclear(offline_cpus)) {
> +		bitmask_displaylist(offline_cpus_str, str_len, offline_cpus);
> +		printf(_("Following CPUs are offline:\n%s\n"),
> +			 offline_cpus_str);
> +		printf(_("cpupower set operation was not performed on them\n"));
> +	}

Make the printing common for cmd_idle_set() and cmd_freq_set().
Make this generic to be able to print online and offline cpus.

> +	free(offline_cpus_str);
> +	bitmask_free(offline_cpus);

Free these from main()

> +
>   	return 0;
>   }
> diff --git a/tools/power/cpupower/utils/cpuidle-set.c b/tools/power/cpupower/utils/cpuidle-set.c
> index 569f268f4c7f..adf6543fd3d6 100644
> --- a/tools/power/cpupower/utils/cpuidle-set.c
> +++ b/tools/power/cpupower/utils/cpuidle-set.c
> @@ -27,9 +27,12 @@ int cmd_idle_set(int argc, char **argv)
>   	extern char *optarg;
>   	extern int optind, opterr, optopt;
>   	int ret = 0, cont = 1, param = 0, disabled;
> +	int str_len = 0;
>   	unsigned long long latency = 0, state_latency;
>   	unsigned int cpu = 0, idlestate = 0, idlestates = 0;
>   	char *endptr;
> +	char *offline_cpus_str = NULL;
> +	struct bitmask *offline_cpus = NULL;
>   
>   	do {
>   		ret = getopt_long(argc, argv, "d:e:ED:", info_opts, NULL);
> @@ -99,14 +102,20 @@ int cmd_idle_set(int argc, char **argv)
>   	if (bitmask_isallclear(cpus_chosen))
>   		bitmask_setall(cpus_chosen);
>   
> +	offline_cpus = bitmask_alloc(sysconf(_SC_NPROCESSORS_CONF));
> +	str_len = sysconf(_SC_NPROCESSORS_CONF) * 5;
> +	offline_cpus_str = (void *)malloc(sizeof(char) * str_len);
> +

Same comment as before.

>   	for (cpu = bitmask_first(cpus_chosen);
>   	     cpu <= bitmask_last(cpus_chosen); cpu++) {
>   
>   		if (!bitmask_isbitset(cpus_chosen, cpu))
>   			continue;
>   
> -		if (cpupower_is_cpu_online(cpu) != 1)
> +		if (cpupower_is_cpu_online(cpu) != 1) {
> +			bitmask_setbit(offline_cpus, cpu);
>   			continue;
> +		}
>   
>   		idlestates = cpuidle_state_count(cpu);
>   		if (idlestates <= 0)
> @@ -181,5 +190,15 @@ int cmd_idle_set(int argc, char **argv)
>   			break;
>   		}
>   	}
> +
> +	if (!bitmask_isallclear(offline_cpus)) {
> +		bitmask_displaylist(offline_cpus_str, str_len, offline_cpus);
> +		printf(_("Following CPUs are offline:\n%s\n"),
> +			 offline_cpus_str);
> +		printf(_("CPU idle operation was not performed on them\n"));
> +	}

Same comment as before.

> +	free(offline_cpus_str);
> +	bitmask_free(offline_cpus);
> +

Same comment as before.
>   	return EXIT_SUCCESS;
>   }
> 

thanks,
-- Shuah