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@xxxxxxxxx>
>> 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
