Re: [PATCH for-next] RDMA/rxe: Fix "Replace red-black trees by xarrays"

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

在 2022/4/12 21:39, Jason Gunthorpe 写道:
> On Tue, Apr 12, 2022 at 09:32:25PM +0800, Yanjun Zhu wrote:
>> 在 2022/4/12 0:02, Jason Gunthorpe 写道:
>>> On Mon, Apr 11, 2022 at 03:52:23PM +0000, Pearson, Robert B wrote:
>>>>> Yes, you cannot use irq_save variants here. You have to know your calling context is non-atomic already and use the irq wrapper.
>>>> Not sure why. If you call irqsave in a non-atomic context doesn't it
>>>> behave the same as irq? I.e. flags = 0.  xarray provides
>>>> xa_lock_xxx() for people to use. Are you saying I have to call
>>>> xa_alloc_cyclic_xxx() instead which is the same thing?
>>> The xa_lock is a magic thing, when you call a __xa_*(.., GFP_KERNEL)
>>> type function then it will unlock and relock the xa_lock internally.
>>>
>>> Thus you cannot wrapper an irqsave lock across the GFP_KERNEL call
>>> sites because XA doesn't know the 'flags' to be able to properly
>>> unlock it.
>>>
>>>> The problem is I've gotten gun shy about people calling into the
>>>> verbs APIs in strange contexts. The rules don't seem to be written
>>>> down. If I could be certain that no one ever is allowed to call a
>>>> verbs API in an interrupt then this is correct but how do I know
>>>> that?
>>> rxe used GFP_KERNEL so it already has assumed it is a process context.
>> Got it.
>>
>> __xa_alloc_cyclic(..., GFP_NOWAIT) will unlock xa lock internally. But it
>> does not handle flags. So the irq is still disabled. Because GFP_NOWAIT is
>> used in __xa_alloc_cyclic, it will not sleep.
>>
>> __xa_alloc_cyclic will lock xa lock and return to xa_unlock_irqrestore.
>>
>> So the following code should be OK?
>>
>>          xa_lock_irqsave(&pool->xa, flags);
>>          err = __xa_alloc_cyclic(&pool->xa, &elem->index, elem, pool->limit,
>>                                  &pool->next, GFP_NOWAIT);
>>          xa_unlock_irqrestore(&pool->xa, flags);
> No, use GFP_KERNEL an use the proper irq or bh version of
> xa_alloc_cyclic.
>
> save is only required if you don't know the contex you are in, and
> here we know we are in a process context.

if we use xa_lock_irq/xa_unlock_irq or xa_lock_bh/xa_unlock_bh instead 
of xa_unlock_irqrestore,

the warning as below will appear. This means that __rxe_add_to_pool 
disables softirq, but fpu_clone enables softirq.

"
Apr 12 16:24:53 kernel: softirqs last  enabled at (13086): 
[<ffffffff91830d26>] fpu_clone+0xf6/0x570
Apr 12 16:24:53 kernel: softirqs last disabled at (13129): 
[<ffffffffc077f319>] __rxe_add_to_pool+0x49/0xa0 [rdma_rxe]
"

As such, it is better to use xa_unlock_irqrestore + 
__xa_alloc(...,GFP_ATOMIC/GFP_NOWAIT).

Zhu Yanjun

>
> Jason