| Message ID | 20220304000808.225811-13-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix race conditions in rxe_pool | expand |
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
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 --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);
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(-)