From patchwork Thu Feb 9 04:07:04 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Frederic Weisbecker X-Patchwork-Id: 6717 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 468FE23F8D for ; Thu, 9 Feb 2012 04:07:14 +0000 (UTC) Received: from mail-iy0-f180.google.com (mail-iy0-f180.google.com [209.85.210.180]) by fiordland.canonical.com (Postfix) with ESMTP id D61E1A18059 for ; Thu, 9 Feb 2012 04:07:13 +0000 (UTC) Received: by iabz7 with SMTP id z7so2533994iab.11 for ; Wed, 08 Feb 2012 20:07:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf :dkim-signature:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=l3WPr1XNGj4rqvdBSEk0FOSQ6P3LtE8pPW1soTOXVxc=; b=Vp2BAQPmJ4vwYFFyRNepJEsMG/+TVyNrmQ4drgqXNh+c4DqcifyLlHC7A3kyzUpMV7 rhKs8cgnXpYm+QwG1aeBMUmSc7iMj9AqFHEXtZMAb3Yb/w4tRHe4CWsDX/By2aolA+Av 8dmnsyAxJ+ZJSpTyEZY6cV646SzdajBDEtbYw= Received: by 10.42.74.195 with SMTP id x3mr73287icj.41.1328760433185; Wed, 08 Feb 2012 20:07:13 -0800 (PST) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.231.12.131 with SMTP id x3cs27440ibx; Wed, 8 Feb 2012 20:07:12 -0800 (PST) Received: by 10.229.135.149 with SMTP id n21mr74869qct.85.1328760431184; Wed, 08 Feb 2012 20:07:11 -0800 (PST) Received: from mail-qw0-f43.google.com (mail-qw0-f43.google.com [209.85.216.43]) by mx.google.com with ESMTPS id eo9si1137298qab.6.2012.02.08.20.07.10 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 08 Feb 2012 20:07:11 -0800 (PST) Received-SPF: pass (google.com: domain of fweisbec@gmail.com designates 209.85.216.43 as permitted sender) client-ip=209.85.216.43; Authentication-Results: mx.google.com; spf=pass (google.com: domain of fweisbec@gmail.com designates 209.85.216.43 as permitted sender) smtp.mail=fweisbec@gmail.com; dkim=pass header.i=@gmail.com Received: by qabg1 with SMTP id g1so4803207qab.16 for ; Wed, 08 Feb 2012 20:07:10 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=l3WPr1XNGj4rqvdBSEk0FOSQ6P3LtE8pPW1soTOXVxc=; b=BJ6di5uIjwpWls5ZOPRcRO+OarGGvWBwQ/y+ys+vJGqsvbagwjOwdBYdF6Q/ynVPkl 7+660kxBPjEaVKBWsBJczP0lWD87/I+0lmv6INg/h0y8BPNKvwpx/APnv5uiCp4Pec5H dNQr7KDfKbtMHrmMsqKEpk2kSA0qJlXsUHqUg= Received: by 10.229.105.159 with SMTP id t31mr94058qco.57.1328760430576; Wed, 08 Feb 2012 20:07:10 -0800 (PST) Received: from localhost (100.20.196.77.rev.sfr.net. [77.196.20.100]) by mx.google.com with ESMTPS id hk9sm3187150qab.20.2012.02.08.20.07.06 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 08 Feb 2012 20:07:09 -0800 (PST) Date: Thu, 9 Feb 2012 05:07:04 +0100 From: Frederic Weisbecker To: "Paul E. McKenney" Cc: linux-kernel@vger.kernel.org, mingo@elte.hu, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@polymtl.ca, josh@joshtriplett.org, niv@us.ibm.com, tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, Valdis.Kletnieks@vt.edu, dhowells@redhat.com, eric.dumazet@gmail.com, darren@dvhart.com, patches@linaro.org, "Paul E. McKenney" Subject: Re: [PATCH tip/core/rcu 45/47] rcu: Allow nesting of rcu_idle_enter() and rcu_idle_exit() Message-ID: <20120209040701.GF25473@somewhere.redhat.com> References: <20120204014452.GA29811@linux.vnet.ibm.com> <1328319922-30828-1-git-send-email-paulmck@linux.vnet.ibm.com> <1328319922-30828-45-git-send-email-paulmck@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1328319922-30828-45-git-send-email-paulmck@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Gm-Message-State: ALoCoQlIMDARTtYTgU+43KEEv4IwhcJEP4/SDbpd28OakToH3Ep3sgh3fcjawSe0PVX3tLb/LPXm On Fri, Feb 03, 2012 at 05:45:20PM -0800, Paul E. McKenney wrote: > From: "Paul E. McKenney" > > Use of RCU in the idle loop is incorrect, quite a few instances of > just that have made their way into mainline, primarily event tracing. > The problem with RCU read-side critical sections on CPUs that RCU believes > to be idle is that RCU is completely ignoring the CPU, along with any > attempts and RCU read-side critical sections. > > The approaches of eliminating the offending uses and of pushing the > definition of idle down beyond the offending uses have both proved > impractical. The new approach is to encapsulate offending uses of RCU > with rcu_idle_exit() and rcu_idle_enter(), but this requires nesting > for code that is invoked both during idle and and during normal execution. > Therefore, this commit modifies rcu_idle_enter() and rcu_idle_exit() to > permit nesting. > > Signed-off-by: Paul E. McKenney > Signed-off-by: Paul E. McKenney > Reviewed-by: Josh Triplett > Acked-by: Deepthi Dharwar > --- > kernel/rcu.h | 21 ++++++++++++++++++++- > kernel/rcutiny.c | 16 ++++++++++++---- > kernel/rcutree.c | 21 ++++++++++++++------- > 3 files changed, 46 insertions(+), 12 deletions(-) > > diff --git a/kernel/rcu.h b/kernel/rcu.h > index 30876f4..8ba99cd 100644 > --- a/kernel/rcu.h > +++ b/kernel/rcu.h > @@ -33,8 +33,27 @@ > * Process-level increment to ->dynticks_nesting field. This allows for > * architectures that use half-interrupts and half-exceptions from > * process context. > + * > + * DYNTICK_TASK_NEST_MASK defines a field of width DYNTICK_TASK_NEST_WIDTH > + * that counts the number of process-based reasons why RCU cannot > + * consider the corresponding CPU to be idle, and DYNTICK_TASK_NEST_VALUE > + * is the value used to increment or decrement this field. > + * > + * The rest of the bits could in principle be used to count interrupts, > + * but this would mean that a negative-one value in the interrupt > + * field could incorrectly zero out the DYNTICK_TASK_NEST_MASK field. > + * We therefore provide a two-bit guard field defined by DYNTICK_TASK_MASK > + * that is set to DYNTICK_TASK_FLAG upon initial exit from idle. > + * The DYNTICK_TASK_EXIT_IDLE value is thus the combined value used upon > + * initial exit from idle. > */ > -#define DYNTICK_TASK_NESTING (LLONG_MAX / 2 - 1) > +#define DYNTICK_TASK_NEST_WIDTH 7 > +#define DYNTICK_TASK_NEST_VALUE ((LLONG_MAX >> DYNTICK_TASK_NEST_WIDTH) + 1) > +#define DYNTICK_TASK_NEST_MASK (LLONG_MAX - DYNTICK_TASK_NEST_VALUE + 1) > +#define DYNTICK_TASK_FLAG ((DYNTICK_TASK_NEST_VALUE / 8) * 2) > +#define DYNTICK_TASK_MASK ((DYNTICK_TASK_NEST_VALUE / 8) * 3) There is one unused bit between DYNTICK_TASK_NEST_MASK and DYNTICK_TASK_MASK, is that intentional? Also do you want to allow nesting of that kind? rcu_idle_enter(); rcu_idle_enter(); rcu_idle_exit(); rcu_idle_exit() in which case I guess that rcu_irq_enter()/rcu_irq_exit() also need to be updated. If we have this: rcu_idle_enter() rcu_idle_enter() rcu_irq_enter() rcu_irq_exit() rcu_idle_exit() rcu_idle_exit() On rcu_irq_enter(), oldval will never be 0 and we'll miss rcu_idle_exit_common(). rcu_irq_exit() has a similar problem as it won't enter rcu_idle_enter_common(). Its check on WARN_ON_ONCE(rdtp->dynticks_nesting < 0) is also wrong because after two calls of rcu_idle_enter(), the value of dynticks_nesting is negative : it's -DYNTICK_TASK_NEST_VALUE. Perhaps this change would allow that. But again that's just in case you need to support that kind of nesting. > +#define DYNTICK_TASK_EXIT_IDLE (DYNTICK_TASK_NEST_VALUE + \ > + DYNTICK_TASK_FLAG) > > /* > * debug_rcu_head_queue()/debug_rcu_head_unqueue() are used internally > diff --git a/kernel/rcutiny.c b/kernel/rcutiny.c > index 4eb34fc..c8b0e15 100644 > --- a/kernel/rcutiny.c > +++ b/kernel/rcutiny.c > @@ -53,7 +53,7 @@ static void __call_rcu(struct rcu_head *head, > > #include "rcutiny_plugin.h" > > -static long long rcu_dynticks_nesting = DYNTICK_TASK_NESTING; > +static long long rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE; > > /* Common code for rcu_idle_enter() and rcu_irq_exit(), see kernel/rcutree.c. */ > static void rcu_idle_enter_common(long long oldval) > @@ -88,7 +88,12 @@ void rcu_idle_enter(void) > > local_irq_save(flags); > oldval = rcu_dynticks_nesting; > - rcu_dynticks_nesting = 0; > + WARN_ON_ONCE((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) == 0); > + if ((rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) == > + DYNTICK_TASK_NEST_VALUE) > + rcu_dynticks_nesting = 0; > + else > + rcu_dynticks_nesting -= DYNTICK_TASK_NEST_VALUE; > rcu_idle_enter_common(oldval); > local_irq_restore(flags); > } > @@ -140,8 +145,11 @@ void rcu_idle_exit(void) > > local_irq_save(flags); > oldval = rcu_dynticks_nesting; > - WARN_ON_ONCE(oldval != 0); > - rcu_dynticks_nesting = DYNTICK_TASK_NESTING; > + WARN_ON_ONCE(rcu_dynticks_nesting < 0); > + if (rcu_dynticks_nesting & DYNTICK_TASK_NEST_MASK) > + rcu_dynticks_nesting += DYNTICK_TASK_NEST_VALUE; > + else > + rcu_dynticks_nesting = DYNTICK_TASK_EXIT_IDLE; > rcu_idle_exit_common(oldval); > local_irq_restore(flags); > } > diff --git a/kernel/rcutree.c b/kernel/rcutree.c > index df0e3c1..92b4776 100644 > --- a/kernel/rcutree.c > +++ b/kernel/rcutree.c > @@ -198,7 +198,7 @@ void rcu_note_context_switch(int cpu) > EXPORT_SYMBOL_GPL(rcu_note_context_switch); > > DEFINE_PER_CPU(struct rcu_dynticks, rcu_dynticks) = { > - .dynticks_nesting = DYNTICK_TASK_NESTING, > + .dynticks_nesting = DYNTICK_TASK_EXIT_IDLE, > .dynticks = ATOMIC_INIT(1), > }; > > @@ -394,7 +394,11 @@ void rcu_idle_enter(void) > local_irq_save(flags); > rdtp = &__get_cpu_var(rcu_dynticks); > oldval = rdtp->dynticks_nesting; > - rdtp->dynticks_nesting = 0; > + WARN_ON_ONCE((oldval & DYNTICK_TASK_NEST_MASK) == 0); > + if ((oldval & DYNTICK_TASK_NEST_MASK) == DYNTICK_TASK_NEST_VALUE) > + rdtp->dynticks_nesting = 0; > + else > + rdtp->dynticks_nesting -= DYNTICK_TASK_NEST_VALUE; > rcu_idle_enter_common(rdtp, oldval); > local_irq_restore(flags); > } > @@ -467,7 +471,7 @@ static void rcu_idle_exit_common(struct rcu_dynticks *rdtp, long long oldval) > * Exit idle mode, in other words, -enter- the mode in which RCU > * read-side critical sections can occur. > * > - * We crowbar the ->dynticks_nesting field to DYNTICK_TASK_NESTING to > + * We crowbar the ->dynticks_nesting field to DYNTICK_TASK_NEST to > * allow for the possibility of usermode upcalls messing up our count > * of interrupt nesting level during the busy period that is just > * now starting. > @@ -481,8 +485,11 @@ void rcu_idle_exit(void) > local_irq_save(flags); > rdtp = &__get_cpu_var(rcu_dynticks); > oldval = rdtp->dynticks_nesting; > - WARN_ON_ONCE(oldval != 0); > - rdtp->dynticks_nesting = DYNTICK_TASK_NESTING; > + WARN_ON_ONCE(oldval < 0); > + if (oldval & DYNTICK_TASK_NEST_MASK) > + rdtp->dynticks_nesting += DYNTICK_TASK_NEST_VALUE; > + else > + rdtp->dynticks_nesting = DYNTICK_TASK_EXIT_IDLE; > rcu_idle_exit_common(rdtp, oldval); > local_irq_restore(flags); > } > @@ -2253,7 +2260,7 @@ rcu_boot_init_percpu_data(int cpu, struct rcu_state *rsp) > rdp->qlen_lazy = 0; > rdp->qlen = 0; > rdp->dynticks = &per_cpu(rcu_dynticks, cpu); > - WARN_ON_ONCE(rdp->dynticks->dynticks_nesting != DYNTICK_TASK_NESTING); > + WARN_ON_ONCE(rdp->dynticks->dynticks_nesting != DYNTICK_TASK_EXIT_IDLE); > WARN_ON_ONCE(atomic_read(&rdp->dynticks->dynticks) != 1); > rdp->cpu = cpu; > rdp->rsp = rsp; > @@ -2281,7 +2288,7 @@ rcu_init_percpu_data(int cpu, struct rcu_state *rsp, int preemptible) > rdp->qlen_last_fqs_check = 0; > rdp->n_force_qs_snap = rsp->n_force_qs; > rdp->blimit = blimit; > - rdp->dynticks->dynticks_nesting = DYNTICK_TASK_NESTING; > + rdp->dynticks->dynticks_nesting = DYNTICK_TASK_EXIT_IDLE; > atomic_set(&rdp->dynticks->dynticks, > (atomic_read(&rdp->dynticks->dynticks) & ~0x1) + 1); > rcu_prepare_for_idle_init(cpu); > -- > 1.7.8 > diff --git a/kernel/rcutree.c b/kernel/rcutree.c index eacc10b..0b7d946 100644 --- a/kernel/rcutree.c +++ b/kernel/rcutree.c @@ -430,8 +430,8 @@ void rcu_irq_exit(void) rdtp = &__get_cpu_var(rcu_dynticks); oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting--; - WARN_ON_ONCE(rdtp->dynticks_nesting < 0); - if (rdtp->dynticks_nesting) + WARN_ON_ONCE(!oldval); + if (rdtp->dynticks_nesting & ~DYNTICK_TASK_NEST_MASK) trace_rcu_dyntick("--=", oldval, rdtp->dynticks_nesting); else rcu_idle_enter_common(rdtp, oldval); @@ -525,8 +525,8 @@ void rcu_irq_enter(void) rdtp = &__get_cpu_var(rcu_dynticks); oldval = rdtp->dynticks_nesting; rdtp->dynticks_nesting++; - WARN_ON_ONCE(rdtp->dynticks_nesting == 0); - if (oldval) + WARN_ON_ONCE(oldval == ~DYNTICK_TASK_NEST_MASK); + if (oldval & ~DYNTICK_TASK_NEST_MASK) trace_rcu_dyntick("++=", oldval, rdtp->dynticks_nesting); else rcu_idle_exit_common(rdtp, oldval);