Re: [PATCH] tools/cpupower: Choose first online CPU to display details

[Shuah Khan <skhan@linuxfoundation.org>]

On 11/20/22 22:35, Saket Kumar Bhaskar wrote:
> The default output of cpupower info utils shows unexpected output
> when CPU 0 is disabled.
> 
> Considering a case where CPU 0 is disabled, output of cpupower idle-info:
> 
> Before change:
> cpupower idle-info
> CPUidle driver: pseries_idle
> CPUidle governor: menu
> analyzing CPU 0:
>   *is offline
> 
> After change:
> ./cpupower idle-info
> CPUidle driver: pseries_idle
> CPUidle governor: menu
> analyzing CPU 0:
>   *is offline
> analyzing CPU 1:
> 
> Number of idle states: 2
> Available idle states: snooze Shared Cede
> snooze:
> Flags/Description: snooze
> Latency: 0
> Usage: 919
> Duration: 68227
> Shared Cede:
> Flags/Description: Shared Cede
> Latency: 10
> Usage: 99324
> Duration: 67871518243
> 
> If -c option is not passed, CPU 0 was chosen as the default chosen CPU to
> display details. However when CPU 0 is offline, it results in showing
> unexpected output. This commit chooses the first online CPU
> instead of CPU 0, hence keeping the output more relevant in all cases.
> 
> Somewhat similar logic to set all CPUs in default case is present in
> cpufreq-set.c, cpuidle-set.c, cpupower-set.c. But here we have added
> logic to stop at first online CPU.
> 
> Signed-off-by: Saket Kumar Bhaskar <skb99@linux.vnet.ibm.com>
> ---
>   tools/power/cpupower/utils/cpufreq-info.c  | 15 ++++++++++++---
>   tools/power/cpupower/utils/cpuidle-info.c  | 15 ++++++++++++---
>   tools/power/cpupower/utils/cpupower-info.c | 16 +++++++++++++---
>   3 files changed, 37 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/power/cpupower/utils/cpufreq-info.c b/tools/power/cpupower/utils/cpufreq-info.c
> index 0646f615fe2d..4001a5934494 100644
> --- a/tools/power/cpupower/utils/cpufreq-info.c
> +++ b/tools/power/cpupower/utils/cpufreq-info.c
> @@ -508,6 +508,7 @@ int cmd_freq_info(int argc, char **argv)
>   	unsigned int cpu = 0;
>   	unsigned int human = 0;
>   	int output_param = 0;
> +	bool is_default = false;
>   
>   	do {
>   		ret = getopt_long(argc, argv, "oefwldpgrasmybnc", info_opts,
> @@ -572,9 +573,11 @@ int cmd_freq_info(int argc, char **argv)
>   
>   	ret = 0;
>   
> -	/* Default is: show output of CPU 0 only */
> -	if (bitmask_isallclear(cpus_chosen))
> -		bitmask_setbit(cpus_chosen, 0);

Why can't we just use base_cpu here instead of 0 and it will
all work without any more code - you can update the comment
to say default is base_cpu info.

thanks,
-- Shuah