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

[Waiman Long <llong@redhat.com>]

On 3/31/25 5:21 PM, Boqun Feng wrote:
> On Mon, Mar 31, 2025 at 02:57:20PM -0400, Waiman Long wrote:
>> On 3/31/25 2:33 PM, Paul E. McKenney wrote:
>>> On Mon, Mar 31, 2025 at 01:33:22PM -0400, Waiman Long wrote:
>>>> On 3/31/25 1:26 PM, Boqun Feng wrote:
>>>>> On Wed, Mar 26, 2025 at 11:39:49AM -0400, Waiman Long wrote:
>>>>> [...]
>>>>>>>> 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.
>>>>> Disabling irq may not work in this case, because an NMI can also happen
>>>>> and call register_lock_class().
>>>> Right, disabling irq doesn't work with NMI. So incrementing the recursion
>>>> count is likely the way to go and I think it will work even in the NMI case.
>>>>
>>>>> I'm experimenting a new idea here, it might be better (for general
>>>>> cases), and this has the similar spirit that we could move the
>>>>> protection scope of a hazard pointer from a key to a hash_list: we can
>>>>> introduce a wildcard address, and whenever we do a synchronize_hazptr(),
>>>>> if the hazptr slot equal to wildcard, we treat as it matches to any ptr,
>>>>> hence synchronize_hazptr() will still wait until it's zero'd. Not only
>>>>> this could help in the nesting case, it can also be used if the users
>>>>> want to protect multiple things with this simple hazard pointer
>>>>> implementation.
>>>> I think it is a good idea to add a wildcard for the general use case.
>>>> Setting the hazptr to the list head will be enough for this particular case.
>>> Careful!  If we enable use of wildcards outside of the special case
>>> of synchronize_hazptr(), we give up the small-memory-footprint advantages
>>> of hazard pointers.  You end up having to wait on all hazard-pointer
>>> readers, which was exactly why RCU was troublesome here.  ;-)
> Technically, only the hazard-pointer readers that have switched to
> wildcard mode because multiple hazptr critical sections ;-)
>
>> If the plan is to have one global set of hazard pointers for all the
> A global set of hazard pointers for all the possible use cases is the
> current plan (at least it should be when we have fully-featured hazptr
> [1]). Because the hazard pointer value already points the the data to
> protect, so no need to group things into "domain"s.
>
>> possible use cases, supporting wildcard may be a problem. If we allow
> I had some off-list discussions with Paul, and I ended up with the idea
> of user-specific wildcard (i.e. different users can have different
> wildcards) + one global set of hazard pointers. However, it just occured
> to me that it won'd quite work in this simple hazard pointer
> implementation (one slot per-CPU) :( Because you can have a user A's
> hazptr critical interrupted by a user B's interrupt handler, and if both
> A & B are using customized wildcard but they don't know each other, it's
> not going to work by setting either wildcard value into the slot.
>
> To make it clear for the discussion, we have two hazard pointer
> implementations:
>
> 1. The fully-featured one [1], which allow users to provide memory for
>     hazptr slots, so no issue about nesting/re-entry etc. And wildcard
>     doesn't make sense in this implemenation.
>
> 2. The simple variant, which is what I've proposed in this thread, and
>     since it only has one slot per CPU, either all the users need to
>     prevent the re-entries or we need a global wildcard. Also the readers
>     of the simple variant need to disable preemption regardlessly because
>     it only has one hazptr slot to use. That means its read-side critical
>     section should be short usually.
>
> I could try to use the fully-featured one in lockdep, what I need to do
> is creating enough hazard_context so we have enough slots for lockdep
> and may or may not need lockdep_recursion to prevent reentries. However,
> I still believe (or I don't have data to show otherwise) that the simple
> variant with one slot per CPU + global wildcard will work fine in
> practice.
>
> So what I would like to do is introducing the simple variant as a
> general API with a global wildcard (because without it, it cannot be a
> general API because one user have to prevent entering another user's
> critical section), and lockdep can use it. And we can monitor the
> delay of synchronize_shazptr() and if wildcard becomes a problem, move
> to a fully-featured hazptr implementation. Sounds like a plan?
>
> [1]: https://lore.kernel.org/lkml/20240917143402.930114-2-boqun.feng@gmail.com/

Thank for the detailed explanation. I am looking forward to your new 
hazptr patch series.

Cheers,
Longman

>
> Regards,
> Boqun
>
>> different sets of hazard pointers for different use cases, it will be less
>> an issue. Anyway, maybe we should skip wildcard for the current case so that
>> we have more time to think through it first.
>>
>> Cheers,
>> Longman
>>