Re: [PATCH] lockdep: Speed up lockdep_unregister_key() with expedited RCU synchronization

[Paul E. McKenney]

On Wed, Mar 26, 2025 at 01:02:12PM -0400, Waiman Long wrote:
> 
> On 3/26/25 12:47 PM, Boqun Feng wrote:
> > On Wed, Mar 26, 2025 at 12:40:59PM -0400, Waiman Long wrote:
> > > On 3/26/25 11:39 AM, Waiman Long wrote:
> > > > On 3/26/25 1:25 AM, Boqun Feng wrote:
> > > > > > It looks like you are trying hard to find a use case for hazard pointer in
> > > > > > the kernel 🙂
> > > > > > 
> > > > > Well, if it does the job, why not use it 😉 Also this shows how
> > > > > flexible hazard pointers can be.
> > > > > 
> > > > > At least when using hazard pointers, the reader side of the hash list
> > > > > iteration is still lockless. Plus, since the synchronization part
> > > > > doesn't need to wait for the RCU readers in the whole system, it will be
> > > > > faster (I tried with the protecting-the-whole-hash-list approach as
> > > > > well, it's the same result on the tc command). This is why I choose to
> > > > > look into hazard pointers. Another mechanism can achieve the similar
> > > > > behavior is SRCU, but SRCU is slightly heavier compared to hazard
> > > > > pointers in this case (of course SRCU has more functionalities).
> > > > > 
> > > > > We can provide a lockdep_unregister_key_nosync() without the
> > > > > synchronize_rcu() in it and let users do the synchronization, but it's
> > > > > going to be hard to enforce and review, especially when someone
> > > > > refactors the code and move the free code to somewhere else.
> > > > Providing a second API and ask callers to do the right thing is probably
> > > > not a good idea and mistake is going to be made sooner or later.
> > > > > > Anyway, that may work. The only problem that I see is the issue of nesting
> > > > > > of an interrupt context on top of a task context. It is possible that the
> > > > > > first use of a raw_spinlock may happen in an interrupt context. If the
> > > > > > interrupt happens when the task has set the hazard pointer and iterating the
> > > > > > hash list, the value of the hazard pointer may be overwritten. Alternatively
> > > > > > we could have multiple slots for the hazard pointer, but that will make the
> > > > > > code more complicated. Or we could disable interrupt before setting the
> > > > > > hazard pointer.
> > > > > Or we can use lockdep_recursion:
> > > > > 
> > > > > 	preempt_disable();
> > > > > 	lockdep_recursion_inc();
> > > > > 	barrier();
> > > > > 
> > > > > 	WRITE_ONCE(*hazptr, ...);
> > > > > 
> > > > > , it should prevent the re-entrant of lockdep in irq.
> > > > That will probably work. Or we can disable irq. I am fine with both.
> > > > > > The solution that I am thinking about is to have a simple unfair rwlock to
> > > > > > protect just the hash list iteration. lockdep_unregister_key() and
> > > > > > lockdep_register_key() take the write lock with interrupt disabled. While
> > > > > > is_dynamic_key() takes the read lock. Nesting in this case isn't a problem
> > > > > > and we don't need RCU to protect the iteration process and so the last
> > > > > > synchronize_rcu() call isn't needed. The level of contention should be low
> > > > > > enough that live lock isn't an issue.
> > > > > > 
> > > > > This could work, one thing though is that locks don't compose. Using a
> > > > > hash write_lock in lockdep_unregister_key() will create a lockdep_lock()
> > > > > -> "hash write_lock" dependency, and that means you cannot
> > > > > lockdep_lock() while you're holding a hash read_lock, although it's
> > > > > not the case today, but it certainly complicates the locking design
> > > > > inside lockdep where there's no lockdep to help 😉
> > > > Thinking about it more, doing it in a lockless way is probably a good
> > > > idea.
> > > > 
> > > If we are using hazard pointer for synchronization, should we also take off
> > > "_rcu" from the list iteration/insertion/deletion macros to avoid the
> > > confusion that RCU is being used?
> > > 
> > We can, but we probably want to introduce a new set of API with suffix
> > "_lockless" or something because they will still need a lockless fashion
> > similar to RCU list iteration/insertion/deletion.
> 
> The lockless part is just the iteration of the list. Insertion and deletion
> is protected by lockdep_lock().
> 
> The current hlist_*_rcu() macros are doing the right things for lockless use
> case too. We can either document that RCU is not being used or have some
> _lockless helpers that just call the _rcu equivalent.

We used to have _lockless helper, but we got rid of them.  Not necessarily
meaning that we should not add them back in, but...  ;-)

							Thanx, Paul