* [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options
@ 2020-07-13 7:56 latha
2020-07-16 17:10 ` Shuah Khan
0 siblings, 1 reply; 2+ messages in thread
From: latha @ 2020-07-13 7:56 UTC (permalink / raw)
To: trenn, shuah; +Cc: linux-pm, linux-kernel, Brahadambal Srinivasan
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.
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);
/* 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;
}
--
2.19.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options
2020-07-13 7:56 [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options latha
@ 2020-07-16 17:10 ` Shuah Khan
0 siblings, 0 replies; 2+ messages in thread
From: Shuah Khan @ 2020-07-16 17:10 UTC (permalink / raw)
To: latha, trenn, shuah; +Cc: linux-pm, linux-kernel, Shuah Khan
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-07-16 17:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 7:56 [PATCH] cpupower: Provide offline CPU information for cpuidle-set and cpufreq-set options latha
2020-07-16 17:10 ` Shuah Khan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).