| Message ID | 1315332049-2604-53-git-send-email-paulmck@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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: 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. > return 1; > } > > @@ -150,7 +155,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) > { > int retval = __srcu_read_lock(sp); > > - srcu_read_acquire(sp); > + rcu_lock_acquire(&(sp)->dep_map); > return retval; > } > > @@ -164,7 +169,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) > static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) > __releases(sp) > { > - srcu_read_release(sp); > + rcu_lock_release(&(sp)->dep_map); > __srcu_read_unlock(sp, idx); > } > > -- > 1.7.3.2 > > -- > 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/
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. Thanx, Paul > > return 1; > > } > > > > @@ -150,7 +155,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) > > { > > int retval = __srcu_read_lock(sp); > > > > - srcu_read_acquire(sp); > > + rcu_lock_acquire(&(sp)->dep_map); > > return retval; > > } > > > > @@ -164,7 +169,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) > > static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) > > __releases(sp) > > { > > - srcu_read_release(sp); > > + rcu_lock_release(&(sp)->dep_map); > > __srcu_read_unlock(sp, idx); > > } > > > > -- > > 1.7.3.2 > > > > -- > > 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/
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.
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; return 1; } @@ -150,7 +155,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) { int retval = __srcu_read_lock(sp); - srcu_read_acquire(sp); + rcu_lock_acquire(&(sp)->dep_map); return retval; } @@ -164,7 +169,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp) __acquires(sp) static inline void srcu_read_unlock(struct srcu_struct *sp, int idx) __releases(sp) { - srcu_read_release(sp); + rcu_lock_release(&(sp)->dep_map); __srcu_read_unlock(sp, idx); }
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(-)