Messages in this thread | | | Date | Fri, 30 Mar 2012 12:41:01 +0200 | From | Michal Hocko <> | 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@linutronix.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@suse.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@linutronix.de> > > > Cc: "Srivatsa S. Bhat" <srivatsa.bhat@linux.vnet.ibm.com> > > > Reported-by: Martin Schwidefsky <schwidefsky@de.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@de.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@de.ibm.com>
Looks good to me. Reviewed-by: Michal Hocko <mhocko@suse.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@vger.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
| |