diff mbox series

[for-next,v11,12/13] RDMA/rxe: Convert read side locking to rcu

Message ID 20220304000808.225811-13-rpearsonhpe@gmail.com (mailing list archive)
State Superseded
Headers show
Series Fix race conditions in rxe_pool | expand

Commit Message

Bob Pearson March 4, 2022, 12:08 a.m. UTC
Use rcu_read_lock() for protecting read side operations in rxe_pool.c.

Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com>
---
 drivers/infiniband/sw/rxe/rxe_pool.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

Comments

Jason Gunthorpe March 16, 2022, 12:18 a.m. UTC | #1
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@gmail.com>
>  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
Bob Pearson March 16, 2022, 4:05 a.m. UTC | #2
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@gmail.com>
>>  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
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c
index 4fb6c7dd32ad..ec464b03d120 100644
--- a/drivers/infiniband/sw/rxe/rxe_pool.c
+++ 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();
 
 	return obj;
 }
@@ -259,7 +258,7 @@  int __rxe_wait(struct rxe_pool_elem *elem)
 		pool->cleanup(elem);
 
 	if (pool->flags & RXE_POOL_ALLOC)
-		kfree(elem->obj);
+		kfree_rcu(elem->obj);
 
 	atomic_dec(&pool->num_elem);