Re: [PATCH for-next 1/6] RDMA/rxe: Make rxe_alloc() take pool lock

[Bob Pearson <rpearsonhpe@gmail.com>]

On 10/20/21 6:16 PM, Jason Gunthorpe wrote:
> On Sun, Oct 10, 2021 at 06:59:26PM -0500, Bob Pearson wrote:
>> In rxe there are two separate pool APIs for creating a new object
>> rxe_alloc() and rxe_alloc_locked(). Currently they are identical.
>> Make rxe_alloc() take the pool lock which is in line with the other
>> APIs in the library.
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
>>  drivers/infiniband/sw/rxe/rxe_pool.c | 21 ++++-----------------
>>  1 file changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
>> index ffa8420b4765..7a288ebacceb 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
>> @@ -352,27 +352,14 @@ void *rxe_alloc_locked(struct rxe_pool *pool)
>>  
>>  void *rxe_alloc(struct rxe_pool *pool)
>>  {
>> -	struct rxe_type_info *info = &rxe_type_info[pool->type];
>> -	struct rxe_pool_entry *elem;
>> +	unsigned long flags;
>>  	u8 *obj;
>>  
>> -	if (atomic_inc_return(&pool->num_elem) > pool->max_elem)
>> -		goto out_cnt;
>> -
>> -	obj = kzalloc(info->size, GFP_KERNEL);
>> -	if (!obj)
>> -		goto out_cnt;
>> -
>> -	elem = (struct rxe_pool_entry *)(obj + info->elem_offset);
>> -
>> -	elem->pool = pool;
>> -	kref_init(&elem->ref_cnt);
>> +	write_lock_irqsave(&pool->pool_lock, flags);
>> +	obj = rxe_alloc_locked(pool);
>> +	write_unlock_irqrestore(&pool->pool_lock, flags);
> 
> But why? This just makes a GFP_KERNEL allocation into a GFP_ATOMIC
> allocation, which is bad.
> 
> Jason
> 
how bad? It only has to happen once in the driver for mcast group elements where
currently I have (to avoid the race when two QPs try to join the same mcast grp
on different CPUs at the same time)

	spin_lock()
	grp = rxe_get_key_locked(pool, mgid)
	if !grp
		grp = rxe_alloc_locked(pool)
	spin_unlock()

Here the kzalloc has to be GFP_ATOMIC. But I could write after fixing things to
move the kzalloc out of the lock in rxe_alloc().

	newgrp = rxe_alloc(pool)	/* using GFP_KERNEL */
	spin_lock()
	grp = rxe_get_key_locked(pool, mgid)
	if (grp)
		kfree(newgrp)
	else {
		grp = newgrp
		<set key in grp>
	}
	spin_unlock()

A typical use case would be for a bunch of QPs to join a mcast group and most of the
time the key lookup succeeds. The trade off is between extra malloc/free and occasional
bad behavior from GFP_ATOMIC.

The majority of uses for rxe_alloc() do not have these issues and I can move the kzalloc
outside of the lock and use GFP_KERNEL.

Bob