| Message ID | 20200713075647.70036-1-latha@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options | expand |
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
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); /* 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")); + } + free(offline_cpus_str); + bitmask_free(offline_cpus); + 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); + 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")); + } + free(offline_cpus_str); + bitmask_free(offline_cpus); + return EXIT_SUCCESS; }