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
