Messages in this thread | | | Subject | Re: possible deadlock in send_sigio | From | Waiman Long <> | Date | Mon, 15 Jun 2020 12:37:26 -0400 |
| |
On 6/12/20 3:01 AM, Boqun Feng wrote: > On Fri, Jun 12, 2020 at 07:55:26AM +0800, Boqun Feng wrote: >> Hi Peter and Waiman, >> >> On Thu, Jun 11, 2020 at 12:09:59PM -0400, Waiman Long wrote: >>> On 6/11/20 10:22 AM, Peter Zijlstra wrote: >>>> On Thu, Jun 11, 2020 at 09:51:29AM -0400, Waiman Long wrote: >>>> >>>>> There was an old lockdep patch that I think may address the issue, but was >>>>> not merged at the time. I will need to dig it out and see if it can be >>>>> adapted to work in the current kernel. It may take some time. >>>> Boqun was working on that; I can't remember what happened, but ISTR it >>>> was shaping up nice. >>>> >>> Yes, I am talking about Boqun's patch. However, I think he had moved to >>> another company and so may not be able to actively work on that again. >>> >> I think you are talking about the rescursive read deadlock detection >> patchset: >> >> https://lore.kernel.org/lkml/20180411135110.9217-1-boqun.feng@gmail.com/ >> >> Let me have a good and send a new version based on today's master of tip >> tree. >> > FWIW, with the following patch, I think we can avoid to the false > positives. But solely with this patch, we don't have the ability to > detect deadlocks with recursive locks.. > > I've managed to rebase my patchset, but need some time to tweak it to > work properly, in the meantime, Dmitry, could you give this a try? > > Regards, > Boqun > > ------------->8 > Subject: [PATCH] locking: More accurate annotations for read_lock() > > On the archs using QUEUED_RWLOCKS, read_lock() is not always a recursive > read lock, actually it's only recursive if in_interrupt() is true. So > change the annotation accordingly to catch more deadlocks. > > Note we used to treat read_lock() as pure recursive read locks in > lib/locking-seftest.c, and this is useful, especially for the lockdep > development selftest, so we keep this via a variable to force switching > lock annotation for read_lock(). > > Signed-off-by: Boqun Feng <boqun.feng@gmail.com> > --- > include/linux/lockdep.h | 35 ++++++++++++++++++++++++++++++++++- > lib/locking-selftest.c | 11 +++++++++++ > 2 files changed, 45 insertions(+), 1 deletion(-) > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index 8fce5c98a4b0..50aedbba0812 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -43,6 +43,7 @@ enum lockdep_wait_type { > #include <linux/list.h> > #include <linux/debug_locks.h> > #include <linux/stacktrace.h> > +#include <linux/preempt.h> > > /* > * We'd rather not expose kernel/lockdep_states.h this wide, but we do need > @@ -640,6 +641,31 @@ static inline void print_irqtrace_events(struct task_struct *curr) > } > #endif > > +/* Variable used to make lockdep treat read_lock() as recursive in selftests */ > +#ifdef CONFIG_DEBUG_LOCKING_API_SELFTESTS > +extern unsigned int force_read_lock_recursive; > +#else /* CONFIG_DEBUG_LOCKING_API_SELFTESTS */ > +#define force_read_lock_recursive 0 > +#endif /* CONFIG_DEBUG_LOCKING_API_SELFTESTS */ > + > +#ifdef CONFIG_LOCKDEP > +/* > + * read_lock() is recursive if: > + * 1. We force lockdep think this way in selftests or > + * 2. The implementation is not queued read/write lock or > + * 3. The locker is at an in_interrupt() context. > + */ > +static inline bool read_lock_is_recursive(void) > +{ > + return force_read_lock_recursive || > + !IS_ENABLED(CONFIG_QUEUED_RWLOCKS) || > + in_interrupt(); > +} > +#else /* CONFIG_LOCKDEP */ > +/* If !LOCKDEP, the value is meaningless */ > +#define read_lock_is_recursive() 0 > +#endif > + > /* > * For trivial one-depth nesting of a lock-class, the following > * global define can be used. (Subsystems with multiple levels > @@ -661,7 +687,14 @@ static inline void print_irqtrace_events(struct task_struct *curr) > #define spin_release(l, i) lock_release(l, i) > > #define rwlock_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i) > -#define rwlock_acquire_read(l, s, t, i) lock_acquire_shared_recursive(l, s, t, NULL, i) > +#define rwlock_acquire_read(l, s, t, i) \ > +do { \ > + if (read_lock_is_recursive()) \ > + lock_acquire_shared_recursive(l, s, t, NULL, i); \ > + else \ > + lock_acquire_shared(l, s, t, NULL, i); \ > +} while (0) > + > #define rwlock_release(l, i) lock_release(l, i) > > #define seqcount_acquire(l, s, t, i) lock_acquire_exclusive(l, s, t, NULL, i) > diff --git a/lib/locking-selftest.c b/lib/locking-selftest.c > index 14f44f59e733..caadc4dd3368 100644 > --- a/lib/locking-selftest.c > +++ b/lib/locking-selftest.c > @@ -28,6 +28,7 @@ > * Change this to 1 if you want to see the failure printouts: > */ > static unsigned int debug_locks_verbose; > +unsigned int force_read_lock_recursive; > > static DEFINE_WD_CLASS(ww_lockdep); > > @@ -1978,6 +1979,11 @@ void locking_selftest(void) > return; > } > > + /* > + * treats read_lock() as recursive read locks for testing purpose > + */ > + force_read_lock_recursive = 1; > + > /* > * Run the testsuite: > */ > @@ -2073,6 +2079,11 @@ void locking_selftest(void) > > ww_tests(); > > + force_read_lock_recursive = 0; > + /* > + * queued_read_lock() specific test cases can be put here > + */ > + > if (unexpected_testcase_failures) { > printk("-----------------------------------------------------------------\n"); > debug_locks = 0;
Your patch looks good to me.
Reviewed-by: Waiman Long <longman@redhat.com>
| |