Messages in this thread | | | Date | Thu, 23 Feb 2023 14:01:58 +0100 | From | Peter Zijlstra <> | Subject | Re: [PATCH 1/2] lockdep: lock_set_lock_cmp_fn() |
| |
On Mon, Feb 20, 2023 at 06:51:59PM -0500, Kent Overstreet wrote: > On Mon, Feb 20, 2023 at 04:09:33PM +0100, Peter Zijlstra wrote: > > which is much simpler to work with. Can we improve on this? Give the > > cmp_fn an additinoal optional argument of a string pointer or so to fill > > out with actual details to be printed? > > Here you go, example bcache output: > > Patch is not as pretty as I'd like because not every path that prints a > lock has held_lock available - but the ones we care about in this > scenario do.
Right, unavoidable that.
> ============================================ > WARNING: possible recursive locking detected > 6.2.0-rc8-00003-g7d81e591ca6a-dirty #15 Not tainted > -------------------------------------------- > kworker/14:3/938 is trying to acquire lock: > ffff8880143218c8 (&b->lock l=0 0:2803368){++++}-{3:3}, at: bch_btree_node_get.part.0+0x81/0x2b0 > > but task is already holding lock: > ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0 > and the lock comparison function returns 1: > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&b->lock l=1 1048575:9223372036854775807); > lock(&b->lock l=0 0:2803368); > > *** DEADLOCK *** > > May be due to missing lock nesting notation > > 3 locks held by kworker/14:3/938: > #0: ffff888005ea9d38 ((wq_completion)bcache){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530 > #1: ffff8880098c3e70 ((work_completion)(&cl->work)#3){+.+.}-{0:0}, at: process_one_work+0x1ec/0x530 > #2: ffff8880143de8c8 (&b->lock l=1 1048575:9223372036854775807){++++}-{3:3}, at: __bch_btree_map_nodes+0xea/0x1e0
Much better indeed. Thanks!
> -- >8 -- > Subject: [PATCH] lockdep: lock_set_print_fn() > > This implements a new interface to lockedp, lock_set_print_fn(), for > printing additional information about a lock. > > This is intended to be useful with the previous lock_set_cmp_fn(); when > defininig an ordering for locks of a given type, we'll want to print out > information about that lock that's relevant for the ordering we defined. > > Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Ingo Molnar <mingo@redhat.com> > --- > include/linux/lockdep.h | 3 ++ > include/linux/lockdep_types.h | 2 + > kernel/locking/lockdep.c | 81 ++++++++++++++++++++++------------- > 3 files changed, 57 insertions(+), 29 deletions(-) > > diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h > index 98e0338a74..66d8a5a24e 100644 > --- a/include/linux/lockdep.h > +++ b/include/linux/lockdep.h > @@ -664,10 +664,13 @@ lockdep_rcu_suspicious(const char *file, const int line, const char *s) > > #ifdef CONFIG_PROVE_LOCKING > void lockdep_set_lock_cmp_fn(struct lockdep_map *lock, lock_cmp_fn fn); > +void lockdep_set_lock_print_fn(struct lockdep_map *lock, lock_print_fn fn);
I would suggest the same as last time; integrate this in the class setting zoo of functions. If you insiste, please keep it one function and force print_fn when cmp_fn. We don't want people to skimp out on this.
Other than that, I don't think this can fully replace subclasses, since subclasses would allow lock hierarchies with other classes inter-twined, while this really relies on pure class nesting.
That is:
A/0 B A/1
is a valid subclass nesting set, but you can't achieve the same with this.
That said; it does seem like a very useful additional annotation for the more complex nesting sets.
Thanks!
| |