All of lore.kernel.org
 help / color / mirror / Atom feed
From: Oleg Nesterov <oleg@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Fernando Luis Vazquez Cao <fernando_b1@lab.ntt.co.jp>,
	Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Arjan van de Ven <arjan@linux.intel.com>
Subject: Re: [PATCH RESEND 0/4] nohz: Fix racy sleeptime stats
Date: Wed, 21 Aug 2013 13:42:28 +0200	[thread overview]
Message-ID: <20130821114228.GA2220@redhat.com> (raw)
In-Reply-To: <20130821082801.GL3258@twins.programming.kicks-ass.net>

On 08/21, Peter Zijlstra wrote:
>
> On Tue, Aug 20, 2013 at 08:15:00PM +0200, Oleg Nesterov wrote:
> > While at it.
> >
> > I do not also understand the cpu_online() checks in fs/proc/stat.c.
> >
> > OK, I agree, if cpu is offline it should not participate in cpu
> > summary. But if it goes offline, why it should switch from
> > ->iowait_sleeptime + cpustat[CPUTIME_IDLE] as it seen by /proc/stat?
> >
> > This can be another source of "idle goes backward", no?
> >
> > IOW. Ignoring the other problems we have, perhaps something like
> > below makes sense?
>
>
> Agreed, however

OK, good,

> > +++ x/kernel/time/tick-sched.c
> > @@ -477,7 +477,7 @@ u64 get_cpu_idle_time_us(int cpu, u64 *l
> >  		update_ts_time_stats(cpu, ts, now, last_update_time);
> >  		idle = ts->idle_sleeptime;
> >  	} else {
> > -		if (ts->idle_active && !nr_iowait_cpu(cpu)) {
> > +		if (ts->idle_active && cpu_online(cpu) && !nr_iowait_cpu(cpu)) {
> >  			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
> >
> >  			idle = ktime_add(ts->idle_sleeptime, delta);
> > @@ -518,7 +518,7 @@ u64 get_cpu_iowait_time_us(int cpu, u64
> >  		update_ts_time_stats(cpu, ts, now, last_update_time);
> >  		iowait = ts->iowait_sleeptime;
> >  	} else {
> > -		if (ts->idle_active && nr_iowait_cpu(cpu) > 0) {
> > +		if (ts->idle_active && cpu_online(cpu) && nr_iowait_cpu(cpu)) {
> >  			ktime_t delta = ktime_sub(now, ts->idle_entrytime);
> >
> >  			iowait = ktime_add(ts->iowait_sleeptime, delta);
> >
>
> That's still mighty odd, but I guess that's in part due to the whacky
> semantics. We could simply transfer any open nr_iowait to the cpu
> doing the hotplug and then we have offline cpus that have nr_iowait == 0
> and the above becomes simpler again.

This won't help get_cpu_idle_time_us().

But anyway we should fix other problems first, then think about this
change. I just wanted to verify that I didn't miss something and this
iowait_sleeptime -> CPUTIME_IDLE switch is indeed wrong.

Oleg.


      reply	other threads:[~2013-08-21 11:48 UTC|newest]

Thread overview: 74+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-16 15:42 [PATCH RESEND 0/4] nohz: Fix racy sleeptime stats Frederic Weisbecker
2013-08-16 15:42 ` [PATCH 1/4] nohz: Only update sleeptime stats locally Frederic Weisbecker
2013-08-18 16:49   ` Oleg Nesterov
2013-08-18 21:38     ` Frederic Weisbecker
2013-08-18 17:04   ` Oleg Nesterov
2013-08-19 18:05     ` Stratos Karafotis
2013-08-16 15:42 ` [PATCH 2/4] nohz: Synchronize sleep time stats with seqlock Frederic Weisbecker
2013-08-16 16:02   ` Oleg Nesterov
2013-08-16 16:20     ` Frederic Weisbecker
2013-08-16 16:26       ` Oleg Nesterov
2013-08-16 16:46         ` Frederic Weisbecker
2013-08-16 16:49           ` Oleg Nesterov
2013-08-16 17:12             ` Frederic Weisbecker
2013-08-18 16:36               ` Oleg Nesterov
2013-08-18 21:25                 ` Frederic Weisbecker
2013-08-19 10:58               ` Peter Zijlstra
2013-08-19 15:44                 ` Arjan van de Ven
2013-08-19 15:47                 ` Arjan van de Ven
2013-08-19 11:10           ` Peter Zijlstra
2013-08-19 11:15             ` Peter Zijlstra
2013-08-20  6:59             ` Fernando Luis Vázquez Cao
2013-08-20  8:44               ` Peter Zijlstra
2013-08-20 15:29                 ` Frederic Weisbecker
2013-08-20 15:33                   ` Arjan van de Ven
2013-08-20 15:35                     ` Frederic Weisbecker
2013-08-20 15:41                       ` Arjan van de Ven
2013-08-20 15:31                 ` Arjan van de Ven
2013-08-20 16:01                   ` Peter Zijlstra
2013-08-20 16:33                     ` Oleg Nesterov
2013-08-20 17:54                       ` Peter Zijlstra
2013-08-20 18:25                         ` Oleg Nesterov
2013-08-21  8:31                           ` Peter Zijlstra
2013-08-21 11:35                             ` Oleg Nesterov
2013-08-21 12:33                               ` Peter Zijlstra
2013-08-21 14:23                                 ` Peter Zijlstra
2013-08-21 16:41                                   ` Oleg Nesterov
2013-10-01 14:05                                     ` Frederic Weisbecker
2013-10-01 14:26                                       ` Frederic Weisbecker
2013-10-01 14:27                                         ` Frederic Weisbecker
2013-10-01 14:49                                         ` Frederic Weisbecker
2013-10-01 15:00                                       ` Peter Zijlstra
2013-10-01 15:21                                         ` Frederic Weisbecker
2013-10-01 15:56                                       ` Peter Zijlstra
2013-10-01 16:47                                         ` Frederic Weisbecker
2013-10-01 16:59                                           ` Peter Zijlstra
2013-10-02 12:45                                             ` Frederic Weisbecker
2013-10-02 12:50                                               ` Peter Zijlstra
2013-10-02 14:35                                               ` Arjan van de Ven
2013-10-02 16:01                                                 ` Frederic Weisbecker
2013-08-21 12:48                               ` Peter Zijlstra
2013-08-21 17:09                                 ` Oleg Nesterov
2013-08-21 18:31                                   ` Peter Zijlstra
2013-08-21 18:32                                     ` Oleg Nesterov
2013-08-20 22:18                       ` Frederic Weisbecker
2013-08-21 11:49                         ` Oleg Nesterov
2013-08-20  6:21           ` Fernando Luis Vázquez Cao
2013-08-20 21:55             ` Frederic Weisbecker
2013-08-16 16:32     ` Frederic Weisbecker
2013-08-16 16:33       ` Oleg Nesterov
2013-08-16 16:49         ` Frederic Weisbecker
2013-08-16 16:37       ` Frederic Weisbecker
2013-08-18 16:54   ` Oleg Nesterov
2013-08-18 21:40     ` Frederic Weisbecker
2013-08-16 15:42 ` [PATCH 3/4] nohz: Consolidate sleep time stats read code Frederic Weisbecker
2013-08-18 17:00   ` Oleg Nesterov
2013-08-18 21:47     ` Frederic Weisbecker
2013-08-16 15:42 ` [PATCH 4/4] nohz: Convert a few places to use local per cpu accesses Frederic Weisbecker
2013-08-16 16:00   ` Peter Zijlstra
2013-08-16 16:12     ` Frederic Weisbecker
2013-08-16 16:19     ` Oleg Nesterov
2013-08-16 16:34       ` Frederic Weisbecker
2013-08-20 18:15 ` [PATCH RESEND 0/4] nohz: Fix racy sleeptime stats Oleg Nesterov
2013-08-21  8:28   ` Peter Zijlstra
2013-08-21 11:42     ` Oleg Nesterov [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20130821114228.GA2220@redhat.com \
    --to=oleg@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=arjan@linux.intel.com \
    --cc=fernando_b1@lab.ntt.co.jp \
    --cc=fweisbec@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=penguin-kernel@I-love.SAKURA.ne.jp \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.