Re: [PATCH] IB/cm: use rwlock for MAD agent lock

[Zhu Yanjun <yanjun.zhu@linux.dev>]

在 2025/2/22 8:38, Eric Dumazet 写道:
> On Sat, Feb 22, 2025 at 7:21 AM Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
>>
>> 在 2025/2/21 18:32, Eric Dumazet 写道:
>>> On Fri, Feb 21, 2025 at 6:04 PM Zhu Yanjun <yanjun.zhu@linux.dev> wrote:
>>>>
>>>> On 20.02.25 18:56, Jacob Moroni wrote:
>>>>> In workloads where there are many processes establishing
>>>>> connections using RDMA CM in parallel (large scale MPI),
>>>>> there can be heavy contention for mad_agent_lock in
>>>>> cm_alloc_msg.
>>>>>
>>>>> This contention can occur while inside of a spin_lock_irq
>>>>> region, leading to interrupts being disabled for extended
>>>>> durations on many cores. Furthermore, it leads to the
>>>>> serialization of rdma_create_ah calls, which has negative
>>>>> performance impacts for NICs which are capable of processing
>>>>> multiple address handle creations in parallel.
>>>>
>>>> In the link:
>>>> https://www.cs.columbia.edu/~jae/4118-LAST/L12-interrupt-spinlock.html
>>>> "
>>>> ...
>>>> spin_lock() / spin_unlock()
>>>>
>>>> must not lose CPU while holding a spin lock, other threads will wait for
>>>> the lock for a long time
>>>>
>>>> spin_lock() prevents kernel preemption by ++preempt_count in
>>>> uniprocessor, that’s all spin_lock() does
>>>>
>>>> must NOT call any function that can potentially sleep
>>>> ex) kmalloc, copy_from_user
>>>>
>>>> hardware interrupt is ok unless the interrupt handler may try to lock
>>>> this spin lock
>>>> spin lock not recursive: same thread locking twice will deadlock
>>>>
>>>> keep the critical section as small as possible
>>>> ...
>>>> "
>>>> And from the source code, it seems that spin_lock/spin_unlock are not
>>>> related with interrupts.
>>>>
>>>> I wonder why "leading to interrupts being disabled for extended
>>>> durations on many cores" with spin_lock/spin_unlock?
>>>>
>>>> I am not against this commit. I am just obvious why
>>>> spin_lock/spin_unlock are related with "interrupts being disabled".
>>>
>>> Look at drivers/infiniband/core/cm.c
>>>
>>> spin_lock_irqsave(&cm_id_priv->lock, flags);
>>
>> Thanks a lot. spin_lock_irq should be spin_lock_irqsave?
> 
> Both spin_lock_irq and spin_lock_irqsave are masking all interrupts.

Maybe the commit log should be written clearly. Thanks.

Zhu Yanjun

> 
> 
> 
>>
>> Follow the steps of reproducer, I can not reproduce this problem on
>> KVMs. Maybe I need a powerful host.
>>
>> Anyway, read_lock should be a lighter lock than spin_lock.
>>
>> Thanks,
>> Reviewed-by: Zhu Yanjun <yanjun.zhu@linux.dev>
>>
>> Zhu Yanjun
>>
>>>
>>> -> Then call cm_alloc_msg() while hard IRQ are masked.
>>