On Tue, Oct 13, 2026 at 3:40 PM Lai Jiangshan <[email protected]> wrote: > > On Tue, Oct 06, 2026 at 01:15:22AM +0800, Kunwu Chan wrote: > > lockdep_unregister_key() waits for is_dynamic_key() callers with > > synchronize_rcu_expedited(), which requires system-wide coordination. > > Make is_dynamic_key() protect the hash bucket with a hazard pointer > > and use hazptr_synchronize() to wait for traversals of that bucket. > > > > The hash bucket returned by keyhashentry() has a stable address, so > > it can be used as the hazptr synchronization target. Keep the key > > hashlist lifetime RCU-based with hlist_del_rcu() and call_rcu(). > > > > Protect the traversal with the hazptr scoped_guard() added earlier > > in this series. > > > > This adapts the lockdep use case from Boqun Feng's shazptr series > > to the current hazptr API. > > > > Link: > > https://lore.kernel.org/lkml/[email protected]/ > > Hello, > > I think this is a misuse of hazard pointers. > > The bucket has a permanent, static address; its lifetime does not > need hazard-pointer protection. Instead, this patch combines a > hazard pointer with that address to construct a read-side/synchronize > mechanism in the same category as RCU. However, this construction > lacks the generation distinction needed to separate pre-existing > readers from subsequent readers. > > The object being removed is the key, but the synchronization target > is the bucket. Unlinking the key does not prevent new lookups from > acquiring the same bucket address, including lookups for unrelated > keys that hash to that bucket. > > This breaks the forward-progress argument used by the underlying > hazptr implementation: after removing an object from publication, > there should not be a continuing stream of new readers acquiring > that same object. Here, the bucket is never unpublished. > > For example, let B be the bucket containing key K: > > Updater Reader Scan kthread > ------- ------ ------------ > Unlink K from B > hazptr_synchronize(B) > Acquire B > Observe B; > retain waiter > Release B > Acquire B again > Observe B; > retain waiter > Release B > ... ... > > Every reader critical section can finish, and the scan kthread > can keep running, while the updater waits indefinitely. Readers > that could have observed K may already have completed; later > lookups alone can keep the waiter pending. > > RCU does not need the reader population to become empty. Its > generation (grace-period) machinery allows pre-existing readers to > drain without letting a continuous stream of subsequent readers > indefinitely extend that wait. This address-based construction has > no equivalent distinction. > > So this is not merely protecting a different object with hazptr: > it is using hazptr as a different synchronization primitive without > providing the corresponding progress guarantee. > > I think we should retain the RCU wait here. If the intention is to > implement bucket-scoped read-side synchronization, that needs an > explicit protocol to distinguish the readers being drained from > new arrivals; a permanent bucket address is not sufficient. > > Sorry for the lengthy reply, which was written with the help of an AI > agent. I noticed this issue when this use case was first proposed, but > I struggled to articulate my concerns clearly back then. Well, now we > have AI. :) >
Hi Lai, thanks for pointing this out. And thanks Boqun, Mathieu, and Paul for the helpful replies. I'll look into adding an RCU-stall-like mechanism to detect long-lived pointer waiters, and work with Boqun and Mathieu to find a more suitable scenario to exercise this case. Thanks, Kunwu > Thanks > Lai > > > Signed-off-by: Kunwu Chan <[email protected]> > > --- > > kernel/locking/lockdep.c | 25 +++++++++++++++---------- > > 1 file changed, 15 insertions(+), 10 deletions(-) > > > > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c > > index c56a7f91d72e..d1ce5ace7fbf 100644 > > --- a/kernel/locking/lockdep.c > > +++ b/kernel/locking/lockdep.c > > @@ -58,6 +58,7 @@ > > #include <linux/context_tracking.h> > > #include <linux/console.h> > > #include <linux/kasan.h> > > +#include <linux/hazptr.h> > > > > #include <asm/sections.h> > > > > @@ -1280,14 +1281,19 @@ static bool is_dynamic_key(const struct > > lock_class_key *key) > > > > hash_head = keyhashentry(key); > > > > - rcu_read_lock(); > > - hlist_for_each_entry_rcu(k, hash_head, hash_entry) { > > - if (k == key) { > > - found = true; > > - break; > > + { > > + struct hazptr_ctx ctx; > > + void *bucket = hash_head; > > + > > + scoped_guard(hazptr, &ctx, &bucket) { > > + hlist_for_each_entry_rcu(k, hash_head, hash_entry, 1) > > { > > + if (k == key) { > > + found = true; > > + break; > > + } > > + } > > } > > } > > - rcu_read_unlock(); > > > > return found; > > } > > @@ -6683,11 +6689,10 @@ void lockdep_unregister_key(struct lock_class_key > > *key) > > * > > * Some operations like __qdisc_destroy() will call this in a debug > > * kernel, and the network traffic is disabled while waiting, hence > > - * the delay of the wait matters in debugging cases. Currently use a > > - * synchronize_rcu_expedited() to speed up the wait at the cost of > > - * system IPIs. TODO: Replace RCU with hazptr for this. > > + * the delay of the wait matters in debugging cases. Replace the > > + * expedited RCU wait with hazptr_synchronize(). > > */ > > - synchronize_rcu_expedited(); > > + hazptr_synchronize(keyhashentry(key)); > > } > > EXPORT_SYMBOL_GPL(lockdep_unregister_key); > > > > -- > > 2.43.0 > >
