| Message ID | 20220606141804.4040-3-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix race conditions in rxe_pool | expand |
On Mon, Jun 06, 2022 at 09:18:04AM -0500, 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 | 26 ++++++++++---------------- > 1 file changed, 10 insertions(+), 16 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c > index 7a5685f0713e..103bf0c03441 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > @@ -195,16 +195,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; > } > @@ -221,16 +220,15 @@ int __rxe_cleanup(struct rxe_pool_elem *elem, bool sleepable) > struct rxe_pool *pool = elem->pool; > struct xarray *xa = &pool->xa; > static int timeout = RXE_POOL_TIMEOUT; > - unsigned long flags; > int ret, err = 0; > void *xa_ret; > > + WARN_ON(!in_task()); I think this should just be if (sleepable) might_sleep(); ? It all seems OK, any chance the AH bit can be split to its own patch or is it all interconnected? It would be easier for rc this way. Jason
-----Original Message----- From: Jason Gunthorpe <jgg@nvidia.com> Sent: Monday, June 6, 2022 12:07 PM To: Bob Pearson <rpearsonhpe@gmail.com> Cc: zyjzyj2000@gmail.com; bvanassche@acm.org; linux-rdma@vger.kernel.org; Zago, Frank <frank.zago@hpe.com>; Hack, Jenny (Ft. Collins) <jhack@hpe.com> Subject: Re: [PATCH for-next v15 2/2] RDMA/rxe: Convert read side locking to rcu On Mon, Jun 06, 2022 at 09:18:04AM -0500, 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 | 26 ++++++++++---------------- > 1 file changed, 10 insertions(+), 16 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c > b/drivers/infiniband/sw/rxe/rxe_pool.c > index 7a5685f0713e..103bf0c03441 100644 > +++ b/drivers/infiniband/sw/rxe/rxe_pool.c > @@ -195,16 +195,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; > } > @@ -221,16 +220,15 @@ int __rxe_cleanup(struct rxe_pool_elem *elem, bool sleepable) > struct rxe_pool *pool = elem->pool; > struct xarray *xa = &pool->xa; > static int timeout = RXE_POOL_TIMEOUT; > - unsigned long flags; > int ret, err = 0; > void *xa_ret; > > + WARN_ON(!in_task()); I think this should just be if (sleepable) might_sleep(); ? It all seems OK, any chance the AH bit can be split to its own patch or is it all interconnected? It would be easier for rc this way. I can't not do anything about AH because it throws kernel splats. I'd rather not break the driver even for one patch. If one says this won't work unless you apply all of them you may as well just have one patch. Bob Jason
diff --git a/drivers/infiniband/sw/rxe/rxe_pool.c b/drivers/infiniband/sw/rxe/rxe_pool.c index 7a5685f0713e..103bf0c03441 100644 --- a/drivers/infiniband/sw/rxe/rxe_pool.c +++ b/drivers/infiniband/sw/rxe/rxe_pool.c @@ -195,16 +195,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; } @@ -221,16 +220,15 @@ int __rxe_cleanup(struct rxe_pool_elem *elem, bool sleepable) struct rxe_pool *pool = elem->pool; struct xarray *xa = &pool->xa; static int timeout = RXE_POOL_TIMEOUT; - unsigned long flags; int ret, err = 0; void *xa_ret; + WARN_ON(!in_task()); + /* erase xarray entry to prevent looking up * the pool elem from its index */ - xa_lock_irqsave(xa, flags); - xa_ret = __xa_erase(xa, elem->index); - xa_unlock_irqrestore(xa, flags); + xa_ret = xa_erase(xa, elem->index); WARN_ON(xa_err(xa_ret)); /* if this is the last call to rxe_put complete the @@ -275,7 +273,7 @@ int __rxe_cleanup(struct rxe_pool_elem *elem, bool sleepable) pool->cleanup(elem); if (pool->type == RXE_TYPE_MR) - kfree(elem->obj); + kfree_rcu(elem->obj); atomic_dec(&pool->num_elem); @@ -294,12 +292,8 @@ int __rxe_put(struct rxe_pool_elem *elem) void __rxe_finalize(struct rxe_pool_elem *elem) { - struct xarray *xa = &elem->pool->xa; - unsigned long flags; - void *ret; - - xa_lock_irqsave(xa, flags); - ret = __xa_store(&elem->pool->xa, elem->index, elem, GFP_KERNEL); - xa_unlock_irqrestore(xa, flags); - WARN_ON(xa_err(ret)); + void *xa_ret; + + xa_ret = xa_store(&elem->pool->xa, elem->index, elem, GFP_KERNEL); + WARN_ON(xa_err(xa_ret)); }
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 | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-)