lkml.org 
[lkml]   [2020]   [Jul]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options
From
Date
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

\
 
 \ /
  Last update: 2020-07-16 19:11    [W:0.032 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site