Messages in this thread | | | Subject | Re: [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options | From | Shuah Khan <> | Date | Thu, 16 Jul 2020 11:10:23 -0600 |
| |
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
| |