Re: much about ah objects in rxe

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Apr 22, 2022 at 01:32:24PM -0500, Bob Pearson wrote:
> Jason,
> 
> I am confused a little.
> 
>  - xa_alloc_xxx internally takes xa->xa_lock with a spinlock but
>    has a gfp_t parameter which is normally GFP_KERNEL. So I trust them when they say
>    that it releases the lock around kmalloc's by 'magic' as you say.
> 
>  - The only read side operation on the rxe pool xarrays is in rxe_pool_get_index() but
>    that will be protected by a rcu_read_lock so it can't deadlock with the write
>    side spinlocks regardless of type (plain, _bh, _saveirq)
> 
>  - Apparently CM is calling ib_create_ah while holding spin locks. This would
>    call xa_alloc_xxx which would unlock xa_lock and call kmalloc(..., GFP_KERNEL)
>    which should cause a warning for AH. You say it does not because xarray doesn't
>    call might_sleep().
> 
> I am not sure how might_sleep() works. When I add might_sleep() just ahead of
> xa_alloc_xxx() it does not cause warnings for CM test cases (e.g. rping.)
> Another way to study this would be to test for in_atomic() but

might_sleep should work, it definately triggers from inside a
spinlock. Perhaps you don't have all the right debug kconfig enabled?

> that seems to be discouraged and may not work as assumed. It's hard to reproduce
> evidence that ib_create_ah really has spinlocks held by the caller. I think it
> was seen in lockdep traces but I have a hard time reading them.

There is a call to create_ah inside RDMA CM that is under a spinlock
 
>  - There is a lot of effort trying to make 'deadlocks' go away. But the read side
>    is going to end as up rcu_read_lock so there soon will be no deadlocks with
>    rxe_pool_get_index() possible. xarrays were designed to work well with rcu
>    so it would better to just go ahead and do it. Verbs objects tend to be long
>    lived with lots of IO on each instance. This is a perfect use case for rcu.

Yes

> I think this means there is no reason for anything but a plain spinlock in rxe_alloc
> and rxe_add_to_pool.

Maybe, are you sure there are no other xa spinlocks held from an IRQ?

And you still have to deal with the create AH called in an atomic
region.

> To sum up once we have rcu enabled the only required change is to use GFP_ATOMIC
> or find a way to pre-allocate for AH objects (assuming that I can convince myself
> that ib_create_ah really comes with spinlocks held).

Possibly yes

Jason