Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <25b0ccf5-cc0e-370c-aef9-5fdfd11b2c30@linuxfoundation.org>
Date:   Mon, 21 Nov 2022 13:26:38 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH] tools/cpupower: Choose first online CPU to display
 details
Content-Language: en-US
To:     Saket Kumar Bhaskar <skb99@linux.vnet.ibm.com>,
        linux-kernel@vger.kernel.org
Cc:     trenn@suse.com, shuah@kernel.org, ray.huang@amd.com,
        linux-pm@vger.kernel.org, Shuah Khan <skhan@linuxfoundation.org>
References: <20221121053530.103600-1-skb99@linux.vnet.ibm.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <20221121053530.103600-1-skb99@linux.vnet.ibm.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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