| Message ID | 20230113065955.815667-3-boqun.feng@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | cfd6ec70dd2bb9205fbc3182f427f7462c76dff3 |
| Headers | show |
| Series | Detect SRCU related deadlocks | expand |
On Thu, Jan 12, 2023 at 10:59:54PM -0800, Boqun Feng wrote: > Although all flavors of RCU are annotated correctly with lockdep as > recursive read locks, their 'check' parameter of lock_acquire() is > unset. It means that RCU read locks are not added into the lockdep > dependency graph therefore deadlock detection based on dependency graph > won't catch deadlock caused by RCU. This is fine for "non-sleepable" RCU > flavors since wait-context detection and other context based detection > can catch these deadlocks. However for sleepable RCU, this is limited. > > Actually we can detect the deadlocks caused by SRCU by 1) making > srcu_read_lock() a 'check'ed recursive read lock and 2) making > synchronize_srcu() a empty write lock critical section. Even better, > with the newly introduced lock_sync(), we can avoid false positives > about irq-unsafe/safe. So do it. > > Note that NMI safe SRCU read side critical sections are currently not > annonated, since step-by-step approach can help us deal with > false-positives. These may be annotated in the future. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> Nice, thank you!!! Acked-by: Paul E. McKenney <paulmck@kernel.org> Or if you would prefer that I take the series through -rcu, please just let me know. Thanx, Paul > --- > include/linux/srcu.h | 23 +++++++++++++++++++++-- > kernel/rcu/srcutiny.c | 2 ++ > kernel/rcu/srcutree.c | 2 ++ > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > index 9b9d0bbf1d3c..a1595f8c5155 100644 > --- a/include/linux/srcu.h > +++ b/include/linux/srcu.h > @@ -102,6 +102,21 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > return lock_is_held(&ssp->dep_map); > } > > +static inline void srcu_lock_acquire(struct lockdep_map *map) > +{ > + lock_map_acquire_read(map); > +} > + > +static inline void srcu_lock_release(struct lockdep_map *map) > +{ > + lock_map_release(map); > +} > + > +static inline void srcu_lock_sync(struct lockdep_map *map) > +{ > + lock_map_sync(map); > +} > + > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > @@ -109,6 +124,10 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > return 1; > } > > +#define srcu_lock_acquire(m) do { } while (0) > +#define srcu_lock_release(m) do { } while (0) > +#define srcu_lock_sync(m) do { } while (0) > + > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > #define SRCU_NMI_UNKNOWN 0x0 > @@ -182,7 +201,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > srcu_check_nmi_safety(ssp, false); > retval = __srcu_read_lock(ssp); > - rcu_lock_acquire(&(ssp)->dep_map); > + srcu_lock_acquire(&(ssp)->dep_map); > return retval; > } > > @@ -226,7 +245,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx) > { > WARN_ON_ONCE(idx & ~0x1); > srcu_check_nmi_safety(ssp, false); > - rcu_lock_release(&(ssp)->dep_map); > + srcu_lock_release(&(ssp)->dep_map); > __srcu_read_unlock(ssp, idx); > } > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > index b12fb0cec44d..336af24e0fe3 100644 > --- a/kernel/rcu/srcutiny.c > +++ b/kernel/rcu/srcutiny.c > @@ -197,6 +197,8 @@ void synchronize_srcu(struct srcu_struct *ssp) > { > struct rcu_synchronize rs; > > + srcu_lock_sync(&ssp->dep_map); > + > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > lock_is_held(&rcu_bh_lock_map) || > lock_is_held(&rcu_lock_map) || > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > index ca4b5dcec675..408088c73e0e 100644 > --- a/kernel/rcu/srcutree.c > +++ b/kernel/rcu/srcutree.c > @@ -1267,6 +1267,8 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm) > { > struct rcu_synchronize rcu; > > + srcu_lock_sync(&ssp->dep_map); > + > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > lock_is_held(&rcu_bh_lock_map) || > lock_is_held(&rcu_lock_map) || > -- > 2.38.1 >
On Fri, Jan 13, 2023 at 03:29:49AM -0800, Paul E. McKenney wrote: > On Thu, Jan 12, 2023 at 10:59:54PM -0800, Boqun Feng wrote: > > Although all flavors of RCU are annotated correctly with lockdep as > > recursive read locks, their 'check' parameter of lock_acquire() is > > unset. It means that RCU read locks are not added into the lockdep > > dependency graph therefore deadlock detection based on dependency graph > > won't catch deadlock caused by RCU. This is fine for "non-sleepable" RCU > > flavors since wait-context detection and other context based detection > > can catch these deadlocks. However for sleepable RCU, this is limited. > > > > Actually we can detect the deadlocks caused by SRCU by 1) making > > srcu_read_lock() a 'check'ed recursive read lock and 2) making > > synchronize_srcu() a empty write lock critical section. Even better, > > with the newly introduced lock_sync(), we can avoid false positives > > about irq-unsafe/safe. So do it. > > > > Note that NMI safe SRCU read side critical sections are currently not > > annonated, since step-by-step approach can help us deal with > > false-positives. These may be annotated in the future. > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > Nice, thank you!!! > > Acked-by: Paul E. McKenney <paulmck@kernel.org> > > Or if you would prefer that I take the series through -rcu, please just > let me know. > I prefer that the first two patches go through your tree, because it reduces the synchronization among locking, rcu and KVM trees to the synchronization betwen rcu and KVM trees. For patch #3, since it's not really ready yet, so I don't know, but I guess when it's finished, probably better go through -rcu. Regards, Boqun > Thanx, Paul > > > --- > > include/linux/srcu.h | 23 +++++++++++++++++++++-- > > kernel/rcu/srcutiny.c | 2 ++ > > kernel/rcu/srcutree.c | 2 ++ > > 3 files changed, 25 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > index 9b9d0bbf1d3c..a1595f8c5155 100644 > > --- a/include/linux/srcu.h > > +++ b/include/linux/srcu.h > > @@ -102,6 +102,21 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > return lock_is_held(&ssp->dep_map); > > } > > > > +static inline void srcu_lock_acquire(struct lockdep_map *map) > > +{ > > + lock_map_acquire_read(map); > > +} > > + > > +static inline void srcu_lock_release(struct lockdep_map *map) > > +{ > > + lock_map_release(map); > > +} > > + > > +static inline void srcu_lock_sync(struct lockdep_map *map) > > +{ > > + lock_map_sync(map); > > +} > > + > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > @@ -109,6 +124,10 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > return 1; > > } > > > > +#define srcu_lock_acquire(m) do { } while (0) > > +#define srcu_lock_release(m) do { } while (0) > > +#define srcu_lock_sync(m) do { } while (0) > > + > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > #define SRCU_NMI_UNKNOWN 0x0 > > @@ -182,7 +201,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > > > srcu_check_nmi_safety(ssp, false); > > retval = __srcu_read_lock(ssp); > > - rcu_lock_acquire(&(ssp)->dep_map); > > + srcu_lock_acquire(&(ssp)->dep_map); > > return retval; > > } > > > > @@ -226,7 +245,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx) > > { > > WARN_ON_ONCE(idx & ~0x1); > > srcu_check_nmi_safety(ssp, false); > > - rcu_lock_release(&(ssp)->dep_map); > > + srcu_lock_release(&(ssp)->dep_map); > > __srcu_read_unlock(ssp, idx); > > } > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > index b12fb0cec44d..336af24e0fe3 100644 > > --- a/kernel/rcu/srcutiny.c > > +++ b/kernel/rcu/srcutiny.c > > @@ -197,6 +197,8 @@ void synchronize_srcu(struct srcu_struct *ssp) > > { > > struct rcu_synchronize rs; > > > > + srcu_lock_sync(&ssp->dep_map); > > + > > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > > lock_is_held(&rcu_bh_lock_map) || > > lock_is_held(&rcu_lock_map) || > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > index ca4b5dcec675..408088c73e0e 100644 > > --- a/kernel/rcu/srcutree.c > > +++ b/kernel/rcu/srcutree.c > > @@ -1267,6 +1267,8 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm) > > { > > struct rcu_synchronize rcu; > > > > + srcu_lock_sync(&ssp->dep_map); > > + > > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > > lock_is_held(&rcu_bh_lock_map) || > > lock_is_held(&rcu_lock_map) || > > -- > > 2.38.1 > >
On Fri, Jan 13, 2023 at 10:05:22AM -0800, Boqun Feng wrote: > On Fri, Jan 13, 2023 at 03:29:49AM -0800, Paul E. McKenney wrote: > > On Thu, Jan 12, 2023 at 10:59:54PM -0800, Boqun Feng wrote: > > > Although all flavors of RCU are annotated correctly with lockdep as > > > recursive read locks, their 'check' parameter of lock_acquire() is > > > unset. It means that RCU read locks are not added into the lockdep > > > dependency graph therefore deadlock detection based on dependency graph > > > won't catch deadlock caused by RCU. This is fine for "non-sleepable" RCU > > > flavors since wait-context detection and other context based detection > > > can catch these deadlocks. However for sleepable RCU, this is limited. > > > > > > Actually we can detect the deadlocks caused by SRCU by 1) making > > > srcu_read_lock() a 'check'ed recursive read lock and 2) making > > > synchronize_srcu() a empty write lock critical section. Even better, > > > with the newly introduced lock_sync(), we can avoid false positives > > > about irq-unsafe/safe. So do it. > > > > > > Note that NMI safe SRCU read side critical sections are currently not > > > annonated, since step-by-step approach can help us deal with > > > false-positives. These may be annotated in the future. > > > > > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > > > > Nice, thank you!!! > > > > Acked-by: Paul E. McKenney <paulmck@kernel.org> > > > > Or if you would prefer that I take the series through -rcu, please just > > let me know. > > I prefer that the first two patches go through your tree, because it > reduces the synchronization among locking, rcu and KVM trees to the > synchronization betwen rcu and KVM trees. Very well, I have queued and pushed these with the usual wordsmithing, thank you! On the possibility of annotating __srcu_read_lock_nmisafe() and __srcu_read_unlock_nmisafe(), are those lockdep annotations themselves NMI-safe? > For patch #3, since it's not really ready yet, so I don't know, but I > guess when it's finished, probably better go through -rcu. Please let me know when it is ready! Thanx, Paul > Regards, > Boqun > > > Thanx, Paul > > > > > --- > > > include/linux/srcu.h | 23 +++++++++++++++++++++-- > > > kernel/rcu/srcutiny.c | 2 ++ > > > kernel/rcu/srcutree.c | 2 ++ > > > 3 files changed, 25 insertions(+), 2 deletions(-) > > > > > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > > > index 9b9d0bbf1d3c..a1595f8c5155 100644 > > > --- a/include/linux/srcu.h > > > +++ b/include/linux/srcu.h > > > @@ -102,6 +102,21 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > > return lock_is_held(&ssp->dep_map); > > > } > > > > > > +static inline void srcu_lock_acquire(struct lockdep_map *map) > > > +{ > > > + lock_map_acquire_read(map); > > > +} > > > + > > > +static inline void srcu_lock_release(struct lockdep_map *map) > > > +{ > > > + lock_map_release(map); > > > +} > > > + > > > +static inline void srcu_lock_sync(struct lockdep_map *map) > > > +{ > > > + lock_map_sync(map); > > > +} > > > + > > > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > > > static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > > @@ -109,6 +124,10 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > > > return 1; > > > } > > > > > > +#define srcu_lock_acquire(m) do { } while (0) > > > +#define srcu_lock_release(m) do { } while (0) > > > +#define srcu_lock_sync(m) do { } while (0) > > > + > > > #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ > > > > > > #define SRCU_NMI_UNKNOWN 0x0 > > > @@ -182,7 +201,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) > > > > > > srcu_check_nmi_safety(ssp, false); > > > retval = __srcu_read_lock(ssp); > > > - rcu_lock_acquire(&(ssp)->dep_map); > > > + srcu_lock_acquire(&(ssp)->dep_map); > > > return retval; > > > } > > > > > > @@ -226,7 +245,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx) > > > { > > > WARN_ON_ONCE(idx & ~0x1); > > > srcu_check_nmi_safety(ssp, false); > > > - rcu_lock_release(&(ssp)->dep_map); > > > + srcu_lock_release(&(ssp)->dep_map); > > > __srcu_read_unlock(ssp, idx); > > > } > > > > > > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c > > > index b12fb0cec44d..336af24e0fe3 100644 > > > --- a/kernel/rcu/srcutiny.c > > > +++ b/kernel/rcu/srcutiny.c > > > @@ -197,6 +197,8 @@ void synchronize_srcu(struct srcu_struct *ssp) > > > { > > > struct rcu_synchronize rs; > > > > > > + srcu_lock_sync(&ssp->dep_map); > > > + > > > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > > > lock_is_held(&rcu_bh_lock_map) || > > > lock_is_held(&rcu_lock_map) || > > > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c > > > index ca4b5dcec675..408088c73e0e 100644 > > > --- a/kernel/rcu/srcutree.c > > > +++ b/kernel/rcu/srcutree.c > > > @@ -1267,6 +1267,8 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm) > > > { > > > struct rcu_synchronize rcu; > > > > > > + srcu_lock_sync(&ssp->dep_map); > > > + > > > RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || > > > lock_is_held(&rcu_bh_lock_map) || > > > lock_is_held(&rcu_lock_map) || > > > -- > > > 2.38.1 > > >
On 1/13/23 20:11, Paul E. McKenney wrote: > On Fri, Jan 13, 2023 at 10:05:22AM -0800, Boqun Feng wrote: >> On Fri, Jan 13, 2023 at 03:29:49AM -0800, Paul E. McKenney wrote: >> I prefer that the first two patches go through your tree, because it >> reduces the synchronization among locking, rcu and KVM trees to the >> synchronization betwen rcu and KVM trees. > > Very well, I have queued and pushed these with the usual wordsmithing, > thank you! I'm worried about this case: CPU 0 CPU 1 -------------------- ------------------ lock A srcu lock B srcu lock B lock A srcu unlock B unlock A unlock A srcu unlock B While a bit unclean, there is nothing that downright forbids this; as long as synchronize_srcu does not happen inside lock A, no deadlock can occur. However, if srcu is replaced with an rwlock then lockdep should and does report a deadlock. Boqun, do you get a false positive or do your patches correctly suppress this? Paolo
On Mon, Jan 16, 2023 at 06:36:43PM +0100, Paolo Bonzini wrote: > On 1/13/23 20:11, Paul E. McKenney wrote: > > On Fri, Jan 13, 2023 at 10:05:22AM -0800, Boqun Feng wrote: > > > On Fri, Jan 13, 2023 at 03:29:49AM -0800, Paul E. McKenney wrote: > > > I prefer that the first two patches go through your tree, because it > > > reduces the synchronization among locking, rcu and KVM trees to the > > > synchronization betwen rcu and KVM trees. > > > > Very well, I have queued and pushed these with the usual wordsmithing, > > thank you! > > I'm worried about this case: > > CPU 0 CPU 1 > -------------------- ------------------ > lock A srcu lock B > srcu lock B lock A > srcu unlock B unlock A > unlock A srcu unlock B > > While a bit unclean, there is nothing that downright forbids this; as long > as synchronize_srcu does not happen inside lock A, no deadlock can occur. > First, even with my change, lockdep won't report this as a deadlock because srcu_read_lock() is annotated as a recursive (unfair) read lock (the "read" parameter for lock_acquire() is 2) and in this case lockdep knows that it won't cause deadlock. For SRCU, the annotation mapping that is 1) srcu_read_lock() is marked as recursive read lock and 2) synchronize_srcu() is marked as write lock sync, recursive read locks themselves cannot cause deadlocks and lockdep is aware of it. Will add a selftest for this later. > However, if srcu is replaced with an rwlock then lockdep should and does > report a deadlock. Boqun, do you get a false positive or do your patches To be more precise, to have a deadlock, the read lock on CPU 0 has to be a *fair* read lock (i.e. non-recursive reader, the "read" parameter for lock_acquire is 1) > correctly suppress this? > I'm pretty sure that lockdep handles this ;-) Regards, Boqun > Paolo >
On Mon, Jan 16, 2023 at 09:54:32AM -0800, Boqun Feng wrote: > On Mon, Jan 16, 2023 at 06:36:43PM +0100, Paolo Bonzini wrote: > > On 1/13/23 20:11, Paul E. McKenney wrote: > > > On Fri, Jan 13, 2023 at 10:05:22AM -0800, Boqun Feng wrote: > > > > On Fri, Jan 13, 2023 at 03:29:49AM -0800, Paul E. McKenney wrote: > > > > I prefer that the first two patches go through your tree, because it > > > > reduces the synchronization among locking, rcu and KVM trees to the > > > > synchronization betwen rcu and KVM trees. > > > > > > Very well, I have queued and pushed these with the usual wordsmithing, > > > thank you! > > > > I'm worried about this case: > > > > CPU 0 CPU 1 > > -------------------- ------------------ > > lock A srcu lock B > > srcu lock B lock A > > srcu unlock B unlock A > > unlock A srcu unlock B > > > > While a bit unclean, there is nothing that downright forbids this; as long > > as synchronize_srcu does not happen inside lock A, no deadlock can occur. > > > > First, even with my change, lockdep won't report this as a deadlock > because srcu_read_lock() is annotated as a recursive (unfair) read lock > (the "read" parameter for lock_acquire() is 2) and in this case lockdep > knows that it won't cause deadlock. > > For SRCU, the annotation mapping that is 1) srcu_read_lock() is marked > as recursive read lock and 2) synchronize_srcu() is marked as write lock > sync, recursive read locks themselves cannot cause deadlocks and lockdep > is aware of it. > > Will add a selftest for this later. > > > However, if srcu is replaced with an rwlock then lockdep should and does > > report a deadlock. Boqun, do you get a false positive or do your patches > > To be more precise, to have a deadlock, the read lock on CPU 0 has to be > a *fair* read lock (i.e. non-recursive reader, the "read" parameter for > lock_acquire is 1) > > > correctly suppress this? > > I'm pretty sure that lockdep handles this ;-) And lockdep agrees, refusing to complain about the following: idx = srcu_read_lock(&srcu1); mutex_lock(&mut1); mutex_unlock(&mut1); srcu_read_unlock(&srcu1, idx); mutex_lock(&mut1); idx = srcu_read_lock(&srcu1); srcu_read_unlock(&srcu1, idx); mutex_unlock(&mut1); Thanx, Paul
On 1/13/23 01:59, Boqun Feng wrote: > Although all flavors of RCU are annotated correctly with lockdep as > recursive read locks, their 'check' parameter of lock_acquire() is > unset. It means that RCU read locks are not added into the lockdep > dependency graph therefore deadlock detection based on dependency graph > won't catch deadlock caused by RCU. This is fine for "non-sleepable" RCU > flavors since wait-context detection and other context based detection > can catch these deadlocks. However for sleepable RCU, this is limited. > > Actually we can detect the deadlocks caused by SRCU by 1) making > srcu_read_lock() a 'check'ed recursive read lock and 2) making > synchronize_srcu() a empty write lock critical section. Even better, > with the newly introduced lock_sync(), we can avoid false positives > about irq-unsafe/safe. So do it. > > Note that NMI safe SRCU read side critical sections are currently not > annonated, since step-by-step approach can help us deal with > false-positives. These may be annotated in the future. > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > include/linux/srcu.h | 23 +++++++++++++++++++++-- > kernel/rcu/srcutiny.c | 2 ++ > kernel/rcu/srcutree.c | 2 ++ > 3 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/include/linux/srcu.h b/include/linux/srcu.h > index 9b9d0bbf1d3c..a1595f8c5155 100644 > --- a/include/linux/srcu.h > +++ b/include/linux/srcu.h > @@ -102,6 +102,21 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) > return lock_is_held(&ssp->dep_map); > } > > +static inline void srcu_lock_acquire(struct lockdep_map *map) > +{ > + lock_map_acquire_read(map); > +} > + > +static inline void srcu_lock_release(struct lockdep_map *map) > +{ > + lock_map_release(map); > +} > + > +static inline void srcu_lock_sync(struct lockdep_map *map) > +{ > + lock_map_sync(map); > +} > + > #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ It would be nice if a comment is added to document the difference between the 2 sets of rcu_lock_* and srcu_lock_* functions. It is described in patch description, but it is not easy to figure that out just by looking at the source files. Cheers, Longman
diff --git a/include/linux/srcu.h b/include/linux/srcu.h index 9b9d0bbf1d3c..a1595f8c5155 100644 --- a/include/linux/srcu.h +++ b/include/linux/srcu.h @@ -102,6 +102,21 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) return lock_is_held(&ssp->dep_map); } +static inline void srcu_lock_acquire(struct lockdep_map *map) +{ + lock_map_acquire_read(map); +} + +static inline void srcu_lock_release(struct lockdep_map *map) +{ + lock_map_release(map); +} + +static inline void srcu_lock_sync(struct lockdep_map *map) +{ + lock_map_sync(map); +} + #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) @@ -109,6 +124,10 @@ static inline int srcu_read_lock_held(const struct srcu_struct *ssp) return 1; } +#define srcu_lock_acquire(m) do { } while (0) +#define srcu_lock_release(m) do { } while (0) +#define srcu_lock_sync(m) do { } while (0) + #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */ #define SRCU_NMI_UNKNOWN 0x0 @@ -182,7 +201,7 @@ static inline int srcu_read_lock(struct srcu_struct *ssp) __acquires(ssp) srcu_check_nmi_safety(ssp, false); retval = __srcu_read_lock(ssp); - rcu_lock_acquire(&(ssp)->dep_map); + srcu_lock_acquire(&(ssp)->dep_map); return retval; } @@ -226,7 +245,7 @@ static inline void srcu_read_unlock(struct srcu_struct *ssp, int idx) { WARN_ON_ONCE(idx & ~0x1); srcu_check_nmi_safety(ssp, false); - rcu_lock_release(&(ssp)->dep_map); + srcu_lock_release(&(ssp)->dep_map); __srcu_read_unlock(ssp, idx); } diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c index b12fb0cec44d..336af24e0fe3 100644 --- a/kernel/rcu/srcutiny.c +++ b/kernel/rcu/srcutiny.c @@ -197,6 +197,8 @@ void synchronize_srcu(struct srcu_struct *ssp) { struct rcu_synchronize rs; + srcu_lock_sync(&ssp->dep_map); + RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || lock_is_held(&rcu_bh_lock_map) || lock_is_held(&rcu_lock_map) || diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c index ca4b5dcec675..408088c73e0e 100644 --- a/kernel/rcu/srcutree.c +++ b/kernel/rcu/srcutree.c @@ -1267,6 +1267,8 @@ static void __synchronize_srcu(struct srcu_struct *ssp, bool do_norm) { struct rcu_synchronize rcu; + srcu_lock_sync(&ssp->dep_map); + RCU_LOCKDEP_WARN(lockdep_is_held(ssp) || lock_is_held(&rcu_bh_lock_map) || lock_is_held(&rcu_lock_map) ||
Although all flavors of RCU are annotated correctly with lockdep as recursive read locks, their 'check' parameter of lock_acquire() is unset. It means that RCU read locks are not added into the lockdep dependency graph therefore deadlock detection based on dependency graph won't catch deadlock caused by RCU. This is fine for "non-sleepable" RCU flavors since wait-context detection and other context based detection can catch these deadlocks. However for sleepable RCU, this is limited. Actually we can detect the deadlocks caused by SRCU by 1) making srcu_read_lock() a 'check'ed recursive read lock and 2) making synchronize_srcu() a empty write lock critical section. Even better, with the newly introduced lock_sync(), we can avoid false positives about irq-unsafe/safe. So do it. Note that NMI safe SRCU read side critical sections are currently not annonated, since step-by-step approach can help us deal with false-positives. These may be annotated in the future. Signed-off-by: Boqun Feng <boqun.feng@gmail.com> --- include/linux/srcu.h | 23 +++++++++++++++++++++-- kernel/rcu/srcutiny.c | 2 ++ kernel/rcu/srcutree.c | 2 ++ 3 files changed, 25 insertions(+), 2 deletions(-)