On 3/15/22 19:18, Jason Gunthorpe wrote:
> On Thu, Mar 03, 2022 at 06:08:08PM -0600, Bob Pearson wrote:
>> Use rcu_read_lock() for protecting read side operations in rxe_pool.c.
>>
>> Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
>> drivers/infiniband/sw/rxe/rxe_pool.c | 7 +++----
>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
>> index 4fb6c7dd32ad..ec464b03d120 100644
>> +++ b/drivers/infiniband/sw/rxe/rxe_pool.c
>> @@ -207,16 +207,15 @@ void *rxe_pool_get_index(struct rxe_pool *pool, u32 index)
>> {
>> struct rxe_pool_elem *elem;
>> struct xarray *xa = &pool->xa;
>> - unsigned long flags;
>> void *obj;
>>
>> - xa_lock_irqsave(xa, flags);
>> + rcu_read_lock();
>> elem = xa_load(xa, index);
>> if (elem && kref_get_unless_zero(&elem->ref_cnt))
>> obj = elem->obj;
>> else
>> obj = NULL;
>> - xa_unlock_irqrestore(xa, flags);
>> + rcu_read_unlock();
>
> This makes the hide even more confusing because hide is racey with
> a RCU lookup.
>
> Looks OK otherwise though
>
> Jason
This conforms with my understanding of RCU. hide() is a write side operation
since it changes the xarray contents while pool_get_index() is a read side
operation. The xa_load here just has to pick a side of the fence for what
to return as the link. It should either return elem or NULL just not something
in the middle. If we have to put the spinlock around the lookup there is no
point in using RCU. In theory there are typically many packets received for
each object create/destroy operation so we should benefit from RCU.
Bob
