| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <839adf7f-8875-47c5-0126-038e69e638ac@linuxfoundation.org>
Date: Thu, 16 Jul 2020 11:10:23 -0600
From: Shuah Khan <skhan@...uxfoundation.org>
To: latha@...ux.vnet.ibm.com, trenn@...e.com, shuah@...nel.org
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
Shuah Khan <skhan@...uxfoundation.org>
Subject: Re: [PATCH] cpupower: Provide offline CPU information for cpuidle-set
and cpufreq-set options
On 7/13/20 1:56 AM, latha@...ux.vnet.ibm.com wrote:
> From: Brahadambal Srinivasan <latha@...ux.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@...ibm.com>
> Signed-off-by: Brahadambal Srinivasan <latha@...ux.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
Powered by blists - more mailing lists