Re: [PATCH RFC v3 08/15] locking/lockdep: use hazptr to wait for dynamic key lookups

[Lai Jiangshan <jiangshanlai@gmail.com>]

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/20250625031101.12555-1-boqun.feng@gmail.com/

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. :)

Thanks
Lai

> Signed-off-by: Kunwu Chan <kunwu.chan@gmail.com>
> ---
>  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
>