From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933969Ab1JDXmp (ORCPT ); Tue, 4 Oct 2011 19:42:45 -0400 Received: from mail-wy0-f174.google.com ([74.125.82.174]:63332 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933838Ab1JDXmo (ORCPT ); Tue, 4 Oct 2011 19:42:44 -0400 Date: Wed, 5 Oct 2011 01:42:39 +0200 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 Subject: Re: [PATCH tip/core/rcu 53/55] rcu: Warn when srcu_read_lock() is used in an extended quiescent state Message-ID: <20111004234237.GA15836@somewhere.redhat.com> References: <20110906180015.GA2560@linux.vnet.ibm.com> <1315332049-2604-53-git-send-email-paulmck@linux.vnet.ibm.com> <20111004210325.GA14165@somewhere.redhat.com> <20111004234028.GK2223@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20111004234028.GK2223@linux.vnet.ibm.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 04, 2011 at 04:40:28PM -0700, Paul E. McKenney wrote: > On Tue, Oct 04, 2011 at 11:03:29PM +0200, Frederic Weisbecker wrote: > > On Tue, Sep 06, 2011 at 11:00:47AM -0700, Paul E. McKenney wrote: > > > Catch SRCU up to the other variants of RCU by making PROVE_RCU > > > complain if either srcu_read_lock() or srcu_read_lock_held() are > > > used from within dyntick-idle mode. > > > > > > Signed-off-by: Paul E. McKenney > > > --- > > > include/linux/srcu.h | 25 +++++++++++++++---------- > > > 1 files changed, 15 insertions(+), 10 deletions(-) > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > index 58971e8..fcbaee7 100644 > > > --- a/include/linux/srcu.h > > > +++ b/include/linux/srcu.h > > > @@ -28,6 +28,7 @@ > > > #define _LINUX_SRCU_H > > > > > > #include > > > +#include > > > > > > struct srcu_struct_array { > > > int c[2]; > > > @@ -60,18 +61,10 @@ int __init_srcu_struct(struct srcu_struct *sp, const char *name, > > > __init_srcu_struct((sp), #sp, &__srcu_key); \ > > > }) > > > > > > -# define srcu_read_acquire(sp) \ > > > - lock_acquire(&(sp)->dep_map, 0, 0, 2, 1, NULL, _THIS_IP_) > > > -# define srcu_read_release(sp) \ > > > - lock_release(&(sp)->dep_map, 1, _THIS_IP_) > > > - > > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > > > int init_srcu_struct(struct srcu_struct *sp); > > > > > > -# define srcu_read_acquire(sp) do { } while (0) > > > -# define srcu_read_release(sp) do { } while (0) > > > - > > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > > > void cleanup_srcu_struct(struct srcu_struct *sp); > > > @@ -90,11 +83,23 @@ long srcu_batches_completed(struct srcu_struct *sp); > > > * read-side critical section. In absence of CONFIG_DEBUG_LOCK_ALLOC, > > > * this assumes we are in an SRCU read-side critical section unless it can > > > * prove otherwise. > > > + * > > > + * Note that if the CPU is in an extended quiescent state, for example, > > > + * if the CPU is in dyntick-idle mode, then rcu_read_lock_held() returns > > > + * false even if the CPU did an rcu_read_lock(). The reason for this is > > > + * that RCU ignores CPUs that are in extended quiescent states, so such > > > + * a CPU is effectively never in an RCU read-side critical section > > > + * regardless of what RCU primitives it invokes. This state of affairs > > > + * is required -- RCU would otherwise need to periodically wake up > > > + * dyntick-idle CPUs, which would defeat the whole purpose of dyntick-idle > > > + * mode. > > > */ > > > static inline int srcu_read_lock_held(struct srcu_struct *sp) > > > { > > > if (debug_locks) > > > return lock_is_held(&sp->dep_map); > > > + if (rcu_check_extended_qs()) > > > + return 0; > > > > Just to warn you, While rebasing this, I'm also moving things around: > > Thank you for letting me know, should not be a problem. > > > if (!debug_lock) > > return 1; > > > > if (rcu_is_cpu_idle()) > > return 0; > > > > return lock_is_held(&sp->dep_map); > > > > Otherwise we only do the check if lock debugging is disabled, > > which is not what we want I think. > > Would it make sense to use this order? > > if (rcu_is_cpu_idle()) > return 0; > > if (!debug_lock) > return 1; > > return lock_is_held(&sp->dep_map); > > Given the new approach, rcu_is_cpu_idle() works whether or not debug_lock > is enabled. Yeah why not. I'm taking that approach. Thanks.