| 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: <20120330104101.GA15375@tiehlicka.suse.cz>
Date: Fri, 30 Mar 2012 12:41:01 +0200
From: Michal Hocko <mhocko@...e.cz>
To: Martin Schwidefsky <schwidefsky@...ibm.com>
Cc: Thomas Gleixner <tglx@...utronix.de>,
"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] fix idle ticks in cpu summary line of /proc/stat
On Fri 30-03-12 12:23:08, Martin Schwidefsky wrote:
> Hi Thomas,
>
> On Thu, 29 Mar 2012 12:42:22 +0200 (CEST)
> Thomas Gleixner <tglx@...utronix.de> wrote:
>
> > On Tue, 13 Mar 2012, Michal Hocko wrote:
> >
> > > OK, so the updated version of the patch looks like this. I am sorry but
> > > I had time to only compile test this...
> > > ---
> > > >From d12247f14c5f8b00ae97a87442f62e49227a759b Mon Sep 17 00:00:00 2001
> > > From: Michal Hocko <mhocko@...e.cz>
> > > Date: Mon, 12 Mar 2012 13:11:38 +0100
> > > Subject: [PATCH] nohz: fix idle ticks in cpu summary line of /proc/stat
> > >
> > > Git commit 09a1d34f8535ecf9 "nohz: Make idle/iowait counter update
> > > conditional" introduced a bug in regard to cpu hotplug. The effect is
> > > that the number of idle ticks in the cpu summary line in /proc/stat is
> > > still counting ticks for offline cpus.
> > >
> > > Reproduction is easy, just start a workload that keeps all cpus busy,
> > > switch off one or more cpus and then watch the idle field in top.
> > > On a dual-core with one cpu 100% busy and one offline cpu you will get
> > > something like this:
> > >
> > > %Cpu(s): 48.7 us, 1.3 sy, 0.0 ni, 50.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st
> > >
> > > The problem is that an offline cpu still has ts->idle_active == 1.
> > > To fix this we should make sure that the cpu is online when calling
> > > get_cpu_idle_time_us and get_cpu_iowait_time_us.
> > >
> > > Cc: Thomas Gleixner <tglx@...utronix.de>
> > > Cc: "Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>
> > > Reported-by: Martin Schwidefsky <schwidefsky@...ibm.com>
> >
> > Martin, does that solve the problem for you ?
>
> Yes, the patch does solve the immediate problem. I just tested if again
> on x86 and s390, works fine. I would like to have another change though.
> The idle_sleep calculation in the scheduler is fine for x86 but for s390
> the arch_idle_time delivers a much better precision. A patch would look
> like this:
> --
> Subject: [PATCH] use arch_idle_time for idle and iowait times if available
>
> From: Martin Schwidefsky <schwidefsky@...ibm.com>
>
> Git commit a25cac5198d4ff28 "proc: Consider NO_HZ when printing idle and
> iowait times" changes the code for /proc/stat to use get_cpu_idle_time_us
> and get_cpu_iowait_time_us if the system is running with nohz enabled.
> For architectures which define arch_idle_time (currently s390 only)
> this is a change for the worse. The result of arch_idle_time is supposed
> to be the exact sleep time of the target cpu and should be used instead
> of the value kept by the scheduler.
>
> Signed-off-by: Martin Schwidefsky <schwidefsky@...ibm.com>
Looks good to me.
Reviewed-by: Michal Hocko <mhocko@...e.cz>
> ---
> fs/proc/stat.c | 34 ++++++++++++++++++++++++++++------
> 1 file changed, 28 insertions(+), 6 deletions(-)
>
> --- a/fs/proc/stat.c
> +++ b/fs/proc/stat.c
> @@ -18,9 +18,30 @@
> #ifndef arch_irq_stat
> #define arch_irq_stat() 0
> #endif
> -#ifndef arch_idle_time
> -#define arch_idle_time(cpu) 0
> -#endif
> +
> +#ifdef arch_idle_time
> +
> +static cputime64_t get_idle_time(int cpu)
> +{
> + cputime64_t idle;
> +
> + idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
> + if (cpu_online(cpu) && !nr_iowait_cpu(cpu))
> + idle += arch_idle_time(cpu);
> + return idle;
> +}
> +
> +static cputime64_t get_iowait_time(int cpu)
> +{
> + cputime64_t iowait;
> +
> + iowait = kcpustat_cpu(cpu).cpustat[CPUTIME_IOWAIT];
> + if (cpu_online(cpu) && nr_iowait_cpu(cpu))
> + iowait += arch_idle_time(cpu);
> + return iowait;
> +}
> +
> +#else
>
> static u64 get_idle_time(int cpu)
> {
> @@ -29,11 +50,10 @@ static u64 get_idle_time(int cpu)
> if (cpu_online(cpu))
> idle_time = get_cpu_idle_time_us(cpu, NULL);
>
> - if (idle_time == -1ULL) {
> + if (idle_time == -1ULL)
> /* !NO_HZ or cpu offline so we can rely on cpustat.idle */
> idle = kcpustat_cpu(cpu).cpustat[CPUTIME_IDLE];
> - idle += arch_idle_time(cpu);
> - } else
> + else
> idle = usecs_to_cputime64(idle_time);
>
> return idle;
> @@ -55,6 +75,8 @@ static u64 get_iowait_time(int cpu)
> return iowait;
> }
>
> +#endif
> +
> static int show_stat(struct seq_file *p, void *v)
> {
> int i, j;
>
> --
> blue skies,
> Martin.
>
> "Reality continues to ruin my life." - Calvin.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9
Czech Republic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists