Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Thu, Apr 12, 2018 at 11:12:17AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 12, 2018 at 10:12:33AM +0800, Boqun Feng wrote:
> > A trivial fix/hack would be adding local_irq_disable() and
> > local_irq_enable() around srcu_lock_sync() like:
> >
> > static inline void srcu_lock_sync(struct lockdep_map *map)
> > {
> > local_irq_disable();
> > lock_map_acquire(map);
> > lock_map_release(map);
> > local_irq_enable();
> > }
> >
> > However, it might be better, if lockdep could provide some annotation
> > API for such an empty critical section to say the grap-and-drop is
> > atomic. Something like:
> >
> > /*
> > * Annotate a wait point for all previous critical section to
> > * go out.
> > *
> > * This won't make @map a irq unsafe lock, no matter it's called
> > * w/ or w/o irq disabled.
> > */
> > lock_wait_unlock(struct lockdep_map *map, ..)
> >
> > And in this primitive, we do something similar like
> > lock_acquire()+lock_release(). This primitive could be used elsewhere,
> > as I bebieve we have several empty grab-and-drop critical section for
> > lockdep annotations, e.g. in start_flush_work().
> >
> > Thoughts?
> >
> > This cerntainly requires a bit more work, in the meanwhile, I will add
> > another self testcase which has a srcu_read_lock() called in irq.
>
> Yeah, I've never really bothered to clean those things up, but I don't
> see any reason to stop you from doing it ;-)
>
> As to the initial pattern with disabling IRQs, I think I've seen code
> like that before, and in general performance isn't a top priority
Yeah, I saw we used that pattern in del_timer_sync()
> (within reason) when you're running lockdep kernels, so I've usually let
> it be.
Turns out it's not very hard to write a working version of
lock_wait_unlock() ;-) Just call __lock_acquire() and __lock_release()
back-to-back with the @hardirqoff for __lock_acquire() to be 1:
/*
* lock_sync() - synchronize with all previous critical sections to
finish.
*
* Simply a acquire+release annotation with hardirqoff is true, because
no lock
* is actually held, so this annotaion alone is safe to be interrupted
as if
* irqs are off
*/
void lock_sync(struct lockdep_map *lock, unsigned subclass, int read,
int check, struct lockdep_map *nest_lock, unsigned long
ip)
{
unsigned long flags;
if (unlikely(current->lockdep_recursion))
return;
raw_local_irq_save(flags);
check_flags(flags);
current->lockdep_recursion = 1;
__lock_acquire(lock, subclass, 0, read, check, 1, nest_lock,
ip, 0, 0);
if (__lock_release(lock, 0, ip))
check_chain_key(current);
current->lockdep_recursion = 0;
raw_local_irq_restore(flags);
}
EXPORT_SYMBOL_GPL(lock_sync);
I rename as lock_sync(), because most of the time, we annotate with this
for a "sync point" with other critical sections. We can avoid some
overhead if we refactor __lock_acquire() and __lock_release() with some
helper functions, but I think this version is good enough for now, at
least better than disabling IRQs around lock_map_acquire() +
lock_map_release() ;-)
Thoughts?
Regards,
Boqun
signature.asc
Description: PGP signature
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Thu, Apr 12, 2018 at 11:12:17AM +0200, Peter Zijlstra wrote:
> On Thu, Apr 12, 2018 at 10:12:33AM +0800, Boqun Feng wrote:
> > A trivial fix/hack would be adding local_irq_disable() and
> > local_irq_enable() around srcu_lock_sync() like:
> >
> > static inline void srcu_lock_sync(struct lockdep_map *map)
> > {
> > local_irq_disable();
> > lock_map_acquire(map);
> > lock_map_release(map);
> > local_irq_enable();
> > }
> >
> > However, it might be better, if lockdep could provide some annotation
> > API for such an empty critical section to say the grap-and-drop is
> > atomic. Something like:
> >
> > /*
> > * Annotate a wait point for all previous critical section to
> > * go out.
> > *
> > * This won't make @map a irq unsafe lock, no matter it's called
> > * w/ or w/o irq disabled.
> > */
> > lock_wait_unlock(struct lockdep_map *map, ..)
> >
> > And in this primitive, we do something similar like
> > lock_acquire()+lock_release(). This primitive could be used elsewhere,
> > as I bebieve we have several empty grab-and-drop critical section for
> > lockdep annotations, e.g. in start_flush_work().
> >
> > Thoughts?
> >
> > This cerntainly requires a bit more work, in the meanwhile, I will add
> > another self testcase which has a srcu_read_lock() called in irq.
>
> Yeah, I've never really bothered to clean those things up, but I don't
> see any reason to stop you from doing it ;-)
>
> As to the initial pattern with disabling IRQs, I think I've seen code
> like that before, and in general performance isn't a top priority
Yeah, I saw we used that pattern in del_timer_sync()
> (within reason) when you're running lockdep kernels, so I've usually let
> it be.
Turns out it's not very hard to write a working version of
lock_wait_unlock() ;-) Just call __lock_acquire() and __lock_release()
back-to-back with the @hardirqoff for __lock_acquire() to be 1:
/*
* lock_sync() - synchronize with all previous critical sections to
finish.
*
* Simply a acquire+release annotation with hardirqoff is true, because
no lock
* is actually held, so this annotaion alone is safe to be interrupted
as if
* irqs are off
*/
void lock_sync(struct lockdep_map *lock, unsigned subclass, int read,
int check, struct lockdep_map *nest_lock, unsigned long
ip)
{
unsigned long flags;
if (unlikely(current->lockdep_recursion))
return;
raw_local_irq_save(flags);
check_flags(flags);
current->lockdep_recursion = 1;
__lock_acquire(lock, subclass, 0, read, check, 1, nest_lock,
ip, 0, 0);
if (__lock_release(lock, 0, ip))
check_chain_key(current);
current->lockdep_recursion = 0;
raw_local_irq_restore(flags);
}
EXPORT_SYMBOL_GPL(lock_sync);
I rename as lock_sync(), because most of the time, we annotate with this
for a "sync point" with other critical sections. We can avoid some
overhead if we refactor __lock_acquire() and __lock_release() with some
helper functions, but I think this version is good enough for now, at
least better than disabling IRQs around lock_map_acquire() +
lock_map_release() ;-)
Thoughts?
Regards,
Boqun
signature.asc
Description: PGP signature
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Thu, Apr 12, 2018 at 10:12:33AM +0800, Boqun Feng wrote:
> A trivial fix/hack would be adding local_irq_disable() and
> local_irq_enable() around srcu_lock_sync() like:
>
> static inline void srcu_lock_sync(struct lockdep_map *map)
> {
> local_irq_disable();
> lock_map_acquire(map);
> lock_map_release(map);
> local_irq_enable();
> }
>
> However, it might be better, if lockdep could provide some annotation
> API for such an empty critical section to say the grap-and-drop is
> atomic. Something like:
>
> /*
>* Annotate a wait point for all previous critical section to
>* go out.
>*
>* This won't make @map a irq unsafe lock, no matter it's called
>* w/ or w/o irq disabled.
>*/
> lock_wait_unlock(struct lockdep_map *map, ..)
>
> And in this primitive, we do something similar like
> lock_acquire()+lock_release(). This primitive could be used elsewhere,
> as I bebieve we have several empty grab-and-drop critical section for
> lockdep annotations, e.g. in start_flush_work().
>
> Thoughts?
>
> This cerntainly requires a bit more work, in the meanwhile, I will add
> another self testcase which has a srcu_read_lock() called in irq.
Yeah, I've never really bothered to clean those things up, but I don't
see any reason to stop you from doing it ;-)
As to the initial pattern with disabling IRQs, I think I've seen code
like that before, and in general performance isn't a top priority
(within reason) when you're running lockdep kernels, so I've usually let
it be.
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Thu, Apr 12, 2018 at 10:12:33AM +0800, Boqun Feng wrote:
> A trivial fix/hack would be adding local_irq_disable() and
> local_irq_enable() around srcu_lock_sync() like:
>
> static inline void srcu_lock_sync(struct lockdep_map *map)
> {
> local_irq_disable();
> lock_map_acquire(map);
> lock_map_release(map);
> local_irq_enable();
> }
>
> However, it might be better, if lockdep could provide some annotation
> API for such an empty critical section to say the grap-and-drop is
> atomic. Something like:
>
> /*
>* Annotate a wait point for all previous critical section to
>* go out.
>*
>* This won't make @map a irq unsafe lock, no matter it's called
>* w/ or w/o irq disabled.
>*/
> lock_wait_unlock(struct lockdep_map *map, ..)
>
> And in this primitive, we do something similar like
> lock_acquire()+lock_release(). This primitive could be used elsewhere,
> as I bebieve we have several empty grab-and-drop critical section for
> lockdep annotations, e.g. in start_flush_work().
>
> Thoughts?
>
> This cerntainly requires a bit more work, in the meanwhile, I will add
> another self testcase which has a srcu_read_lock() called in irq.
Yeah, I've never really bothered to clean those things up, but I don't
see any reason to stop you from doing it ;-)
As to the initial pattern with disabling IRQs, I think I've seen code
like that before, and in general performance isn't a top priority
(within reason) when you're running lockdep kernels, so I've usually let
it be.
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Wed, Apr 11, 2018 at 11:57:30AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 11, 2018 at 09:56:44PM +0800, Boqun Feng wrote:
> > Although all flavors of RCU are annotated correctly with lockdep
> > annotations as recursive read locks, the 'check' parameter for their
> > calls to lock_acquire() is unset. Which means RCU read locks are not
> > added into the lockdep dependency graph. This is fine for all flavors
> > except sleepable RCU, because the deadlock scenarios for them are
> > simple: calling synchronize_rcu() and its friends inside their read-side
> > critical sections. But for sleepable RCU, as there may be multiple
> > instances with multiple classes, there are more deadlock cases.
> > Considering the following:
> >
> > TASK 1 TASK 2
> > ===
> > i = srcu_read_lock();i = srcu_read_lock();
> > synchronize_srcu(); synchronize_srcu();
> > srcu_read_unlock(); srcu_read_unlock();
> >
> > Neither TASK 1 or 2 could go out of the read-side critical sections,
> > because they are waiting for each other at synchronize_srcu().
> >
> > With the new improvement for lockdep, which allows us to detect
> > deadlocks for recursive read locks, we can actually detect this. What we
> > need to do are simply: a) mark srcu_read_{,un}lock() as 'check'
> > lock_acquire() and b) annotate synchronize_srcu() as a empty
> > grab-and-drop for a write lock (because synchronize_srcu() will wait for
> > previous srcu_read_lock() to release, and won't block the next
> > srcu_read_lock(), just like a empty write lock section).
> >
> > This patch adds those to allow we check deadlocks related to sleepable
> > RCU with lockdep.
> >
> > Cc: Paul E. McKenney
> > Signed-off-by: Boqun Feng
>
> Very cool!
>
> One question though... Won't this report a false-positive self-deadlock if
> srcu_read_lock() is invoked from an interrupt handler?
>
Ah.. right. And the false-positive happens because synchronize_srcu() is
annotated as a irq-write-unsafe lock, which should be fixed because
synchronize_srcu() doesn't block a srcu_read_lock() and the empty
write lock critical section in srcu_lock_sync() should mean the
grab-and-drop is atomic (i.e. no one could interrupt), therefore no irq
inversion problem.
A trivial fix/hack would be adding local_irq_disable() and
local_irq_enable() around srcu_lock_sync() like:
static inline void srcu_lock_sync(struct lockdep_map *map)
{
local_irq_disable();
lock_map_acquire(map);
lock_map_release(map);
local_irq_enable();
}
However, it might be better, if lockdep could provide some annotation
API for such an empty critical section to say the grap-and-drop is
atomic. Something like:
/*
* Annotate a wait point for all previous critical section to
* go out.
*
* This won't make @map a irq unsafe lock, no matter it's called
* w/ or w/o irq disabled.
*/
lock_wait_unlock(struct lockdep_map *map, ..)
And in this primitive, we do something similar like
lock_acquire()+lock_release(). This primitive could be used elsewhere,
as I bebieve we have several empty grab-and-drop critical section for
lockdep annotations, e.g. in start_flush_work().
Thoughts?
This cerntainly requires a bit more work, in the meanwhile, I will add
another self testcase which has a srcu_read_lock() called in irq.
Thanks!
Regards,
Boqun
> Thanx, Paul
>
> > ---
> > include/linux/srcu.h | 51
> > +--
> > kernel/rcu/srcutiny.c | 2 ++
> > kernel/rcu/srcutree.c | 2 ++
> > 3 files changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 33c1c698df09..23f397bd192c 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -99,6 +99,49 @@ static inline int srcu_read_lock_held(const struct
> > srcu_struct *sp)
> > return lock_is_held(>dep_map);
> > }
> >
> > +/**
> > + * lockdep annotations for srcu_read_{un,}lock, and synchronize_srcu():
> > + *
> > + * srcu_read_lock() and srcu_read_unlock() are similar to rcu_read_lock()
> > and
> > + * rcu_read_unlock(), they are recursive read locks. But we mark them as
> > + * "check", they will be added into lockdep dependency graph for deadlock
> > + * detection. And we also annotate synchronize_srcu() as a
> > + * write_lock()+write_unlock(), because synchronize_srcu() will wait for
> > any
> > + * corresponding previous srcu_read_lock() to release, and that acts like a
> > + * empty grab-and-drop write lock.
> > + *
> > + * We do so because multiple sleepable rcu instances may cause deadlock as
> > + * follow:
> > + *
> > + * Task 1:
> > + * ia = srcu_read_lock(_A);
> > + *
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Wed, Apr 11, 2018 at 11:57:30AM -0700, Paul E. McKenney wrote:
> On Wed, Apr 11, 2018 at 09:56:44PM +0800, Boqun Feng wrote:
> > Although all flavors of RCU are annotated correctly with lockdep
> > annotations as recursive read locks, the 'check' parameter for their
> > calls to lock_acquire() is unset. Which means RCU read locks are not
> > added into the lockdep dependency graph. This is fine for all flavors
> > except sleepable RCU, because the deadlock scenarios for them are
> > simple: calling synchronize_rcu() and its friends inside their read-side
> > critical sections. But for sleepable RCU, as there may be multiple
> > instances with multiple classes, there are more deadlock cases.
> > Considering the following:
> >
> > TASK 1 TASK 2
> > ===
> > i = srcu_read_lock();i = srcu_read_lock();
> > synchronize_srcu(); synchronize_srcu();
> > srcu_read_unlock(); srcu_read_unlock();
> >
> > Neither TASK 1 or 2 could go out of the read-side critical sections,
> > because they are waiting for each other at synchronize_srcu().
> >
> > With the new improvement for lockdep, which allows us to detect
> > deadlocks for recursive read locks, we can actually detect this. What we
> > need to do are simply: a) mark srcu_read_{,un}lock() as 'check'
> > lock_acquire() and b) annotate synchronize_srcu() as a empty
> > grab-and-drop for a write lock (because synchronize_srcu() will wait for
> > previous srcu_read_lock() to release, and won't block the next
> > srcu_read_lock(), just like a empty write lock section).
> >
> > This patch adds those to allow we check deadlocks related to sleepable
> > RCU with lockdep.
> >
> > Cc: Paul E. McKenney
> > Signed-off-by: Boqun Feng
>
> Very cool!
>
> One question though... Won't this report a false-positive self-deadlock if
> srcu_read_lock() is invoked from an interrupt handler?
>
Ah.. right. And the false-positive happens because synchronize_srcu() is
annotated as a irq-write-unsafe lock, which should be fixed because
synchronize_srcu() doesn't block a srcu_read_lock() and the empty
write lock critical section in srcu_lock_sync() should mean the
grab-and-drop is atomic (i.e. no one could interrupt), therefore no irq
inversion problem.
A trivial fix/hack would be adding local_irq_disable() and
local_irq_enable() around srcu_lock_sync() like:
static inline void srcu_lock_sync(struct lockdep_map *map)
{
local_irq_disable();
lock_map_acquire(map);
lock_map_release(map);
local_irq_enable();
}
However, it might be better, if lockdep could provide some annotation
API for such an empty critical section to say the grap-and-drop is
atomic. Something like:
/*
* Annotate a wait point for all previous critical section to
* go out.
*
* This won't make @map a irq unsafe lock, no matter it's called
* w/ or w/o irq disabled.
*/
lock_wait_unlock(struct lockdep_map *map, ..)
And in this primitive, we do something similar like
lock_acquire()+lock_release(). This primitive could be used elsewhere,
as I bebieve we have several empty grab-and-drop critical section for
lockdep annotations, e.g. in start_flush_work().
Thoughts?
This cerntainly requires a bit more work, in the meanwhile, I will add
another self testcase which has a srcu_read_lock() called in irq.
Thanks!
Regards,
Boqun
> Thanx, Paul
>
> > ---
> > include/linux/srcu.h | 51
> > +--
> > kernel/rcu/srcutiny.c | 2 ++
> > kernel/rcu/srcutree.c | 2 ++
> > 3 files changed, 53 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 33c1c698df09..23f397bd192c 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -99,6 +99,49 @@ static inline int srcu_read_lock_held(const struct
> > srcu_struct *sp)
> > return lock_is_held(>dep_map);
> > }
> >
> > +/**
> > + * lockdep annotations for srcu_read_{un,}lock, and synchronize_srcu():
> > + *
> > + * srcu_read_lock() and srcu_read_unlock() are similar to rcu_read_lock()
> > and
> > + * rcu_read_unlock(), they are recursive read locks. But we mark them as
> > + * "check", they will be added into lockdep dependency graph for deadlock
> > + * detection. And we also annotate synchronize_srcu() as a
> > + * write_lock()+write_unlock(), because synchronize_srcu() will wait for
> > any
> > + * corresponding previous srcu_read_lock() to release, and that acts like a
> > + * empty grab-and-drop write lock.
> > + *
> > + * We do so because multiple sleepable rcu instances may cause deadlock as
> > + * follow:
> > + *
> > + * Task 1:
> > + * ia = srcu_read_lock(_A);
> > + * synchronize_srcu(_B);
> > + * srcu_read_unlock(_A,
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Wed, Apr 11, 2018 at 09:56:44PM +0800, Boqun Feng wrote:
> Although all flavors of RCU are annotated correctly with lockdep
> annotations as recursive read locks, the 'check' parameter for their
> calls to lock_acquire() is unset. Which means RCU read locks are not
> added into the lockdep dependency graph. This is fine for all flavors
> except sleepable RCU, because the deadlock scenarios for them are
> simple: calling synchronize_rcu() and its friends inside their read-side
> critical sections. But for sleepable RCU, as there may be multiple
> instances with multiple classes, there are more deadlock cases.
> Considering the following:
>
> TASK 1 TASK 2
> ===
> i = srcu_read_lock();i = srcu_read_lock();
> synchronize_srcu(); synchronize_srcu();
> srcu_read_unlock(); srcu_read_unlock();
>
> Neither TASK 1 or 2 could go out of the read-side critical sections,
> because they are waiting for each other at synchronize_srcu().
>
> With the new improvement for lockdep, which allows us to detect
> deadlocks for recursive read locks, we can actually detect this. What we
> need to do are simply: a) mark srcu_read_{,un}lock() as 'check'
> lock_acquire() and b) annotate synchronize_srcu() as a empty
> grab-and-drop for a write lock (because synchronize_srcu() will wait for
> previous srcu_read_lock() to release, and won't block the next
> srcu_read_lock(), just like a empty write lock section).
>
> This patch adds those to allow we check deadlocks related to sleepable
> RCU with lockdep.
>
> Cc: Paul E. McKenney
> Signed-off-by: Boqun Feng
Very cool!
One question though... Won't this report a false-positive self-deadlock if
srcu_read_lock() is invoked from an interrupt handler?
Thanx, Paul
> ---
> include/linux/srcu.h | 51
> +--
> kernel/rcu/srcutiny.c | 2 ++
> kernel/rcu/srcutree.c | 2 ++
> 3 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 33c1c698df09..23f397bd192c 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -99,6 +99,49 @@ static inline int srcu_read_lock_held(const struct
> srcu_struct *sp)
> return lock_is_held(>dep_map);
> }
>
> +/**
> + * lockdep annotations for srcu_read_{un,}lock, and synchronize_srcu():
> + *
> + * srcu_read_lock() and srcu_read_unlock() are similar to rcu_read_lock() and
> + * rcu_read_unlock(), they are recursive read locks. But we mark them as
> + * "check", they will be added into lockdep dependency graph for deadlock
> + * detection. And we also annotate synchronize_srcu() as a
> + * write_lock()+write_unlock(), because synchronize_srcu() will wait for any
> + * corresponding previous srcu_read_lock() to release, and that acts like a
> + * empty grab-and-drop write lock.
> + *
> + * We do so because multiple sleepable rcu instances may cause deadlock as
> + * follow:
> + *
> + * Task 1:
> + * ia = srcu_read_lock(_A);
> + * synchronize_srcu(_B);
> + * srcu_read_unlock(_A, ia);
> + *
> + * Task 2:
> + * ib = srcu_read_lock(_B);
> + * synchronize_srcu(_A);
> + * srcu_read_unlock(_B, ib);
> + *
> + * And we want lockdep to detect this or more complicated deadlock with SRCU
> + * involved.
> + */
> +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_acquire(map);
> + lock_map_release(map);
> +}
> +
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> static inline int srcu_read_lock_held(const struct srcu_struct *sp)
> @@ -106,6 +149,10 @@ static inline int srcu_read_lock_held(const struct
> srcu_struct *sp)
> 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 */
>
> /**
> @@ -157,7 +204,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp)
> __acquires(sp)
> int retval;
>
> retval = __srcu_read_lock(sp);
> - rcu_lock_acquire(&(sp)->dep_map);
> + srcu_lock_acquire(&(sp)->dep_map);
> return retval;
> }
>
> @@ -171,7 +218,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)
> {
> - rcu_lock_release(&(sp)->dep_map);
> + srcu_lock_release(&(sp)->dep_map);
> __srcu_read_unlock(sp, idx);
> }
>
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index
Re: [RFC tip/locking/lockdep v6 19/20] rcu: Equip sleepable RCU with lockdep dependency graph checks
On Wed, Apr 11, 2018 at 09:56:44PM +0800, Boqun Feng wrote:
> Although all flavors of RCU are annotated correctly with lockdep
> annotations as recursive read locks, the 'check' parameter for their
> calls to lock_acquire() is unset. Which means RCU read locks are not
> added into the lockdep dependency graph. This is fine for all flavors
> except sleepable RCU, because the deadlock scenarios for them are
> simple: calling synchronize_rcu() and its friends inside their read-side
> critical sections. But for sleepable RCU, as there may be multiple
> instances with multiple classes, there are more deadlock cases.
> Considering the following:
>
> TASK 1 TASK 2
> ===
> i = srcu_read_lock();i = srcu_read_lock();
> synchronize_srcu(); synchronize_srcu();
> srcu_read_unlock(); srcu_read_unlock();
>
> Neither TASK 1 or 2 could go out of the read-side critical sections,
> because they are waiting for each other at synchronize_srcu().
>
> With the new improvement for lockdep, which allows us to detect
> deadlocks for recursive read locks, we can actually detect this. What we
> need to do are simply: a) mark srcu_read_{,un}lock() as 'check'
> lock_acquire() and b) annotate synchronize_srcu() as a empty
> grab-and-drop for a write lock (because synchronize_srcu() will wait for
> previous srcu_read_lock() to release, and won't block the next
> srcu_read_lock(), just like a empty write lock section).
>
> This patch adds those to allow we check deadlocks related to sleepable
> RCU with lockdep.
>
> Cc: Paul E. McKenney
> Signed-off-by: Boqun Feng
Very cool!
One question though... Won't this report a false-positive self-deadlock if
srcu_read_lock() is invoked from an interrupt handler?
Thanx, Paul
> ---
> include/linux/srcu.h | 51
> +--
> kernel/rcu/srcutiny.c | 2 ++
> kernel/rcu/srcutree.c | 2 ++
> 3 files changed, 53 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> index 33c1c698df09..23f397bd192c 100644
> --- a/include/linux/srcu.h
> +++ b/include/linux/srcu.h
> @@ -99,6 +99,49 @@ static inline int srcu_read_lock_held(const struct
> srcu_struct *sp)
> return lock_is_held(>dep_map);
> }
>
> +/**
> + * lockdep annotations for srcu_read_{un,}lock, and synchronize_srcu():
> + *
> + * srcu_read_lock() and srcu_read_unlock() are similar to rcu_read_lock() and
> + * rcu_read_unlock(), they are recursive read locks. But we mark them as
> + * "check", they will be added into lockdep dependency graph for deadlock
> + * detection. And we also annotate synchronize_srcu() as a
> + * write_lock()+write_unlock(), because synchronize_srcu() will wait for any
> + * corresponding previous srcu_read_lock() to release, and that acts like a
> + * empty grab-and-drop write lock.
> + *
> + * We do so because multiple sleepable rcu instances may cause deadlock as
> + * follow:
> + *
> + * Task 1:
> + * ia = srcu_read_lock(_A);
> + * synchronize_srcu(_B);
> + * srcu_read_unlock(_A, ia);
> + *
> + * Task 2:
> + * ib = srcu_read_lock(_B);
> + * synchronize_srcu(_A);
> + * srcu_read_unlock(_B, ib);
> + *
> + * And we want lockdep to detect this or more complicated deadlock with SRCU
> + * involved.
> + */
> +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_acquire(map);
> + lock_map_release(map);
> +}
> +
> #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
>
> static inline int srcu_read_lock_held(const struct srcu_struct *sp)
> @@ -106,6 +149,10 @@ static inline int srcu_read_lock_held(const struct
> srcu_struct *sp)
> 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 */
>
> /**
> @@ -157,7 +204,7 @@ static inline int srcu_read_lock(struct srcu_struct *sp)
> __acquires(sp)
> int retval;
>
> retval = __srcu_read_lock(sp);
> - rcu_lock_acquire(&(sp)->dep_map);
> + srcu_lock_acquire(&(sp)->dep_map);
> return retval;
> }
>
> @@ -171,7 +218,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)
> {
> - rcu_lock_release(&(sp)->dep_map);
> + srcu_lock_release(&(sp)->dep_map);
> __srcu_read_unlock(sp, idx);
> }
>
> diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> index 76ac5f50b2c7..bc89cb48d800 100644
> --- a/kernel/rcu/srcutiny.c
