Messages in this thread | | | Date | Wed, 5 Oct 2011 01:42:39 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH tip/core/rcu 53/55] rcu: Warn when srcu_read_lock() is used in an extended quiescent state |
| |
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 <paulmck@linux.vnet.ibm.com> > > > --- > > > 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 <linux/mutex.h> > > > +#include <linux/rcupdate.h> > > > > > > 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.
| |