| Message ID | 20220421014042.26985-1-rpearsonhpe@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix race conditions in rxe_pool | expand |
On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > Bob Pearson (10): > RDMA/rxe: Remove IB_SRQ_INIT_MASK > RDMA/rxe: Add rxe_srq_cleanup() > RDMA/rxe: Check rxe_get() return value > RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() > RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() > RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() > RDMA/rxe: Enforce IBA C11-17 I took these patches with the small edits I noted > RDMA/rxe: Stop lookup of partially built objects > RDMA/rxe: Convert read side locking to rcu > RDMA/rxe: Cleanup rxe_pool.c It seems OK, but we need to fix the AH problem at least in the destroy path first - lets try to fix it in alloc as well? Jason
在 2022/5/10 2:23, Jason Gunthorpe 写道: > On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > >> Bob Pearson (10): >> RDMA/rxe: Remove IB_SRQ_INIT_MASK >> RDMA/rxe: Add rxe_srq_cleanup() >> RDMA/rxe: Check rxe_get() return value >> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() >> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() >> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() >> RDMA/rxe: Enforce IBA C11-17 > > I took these patches with the small edits I noted > >> RDMA/rxe: Stop lookup of partially built objects >> RDMA/rxe: Convert read side locking to rcu >> RDMA/rxe: Cleanup rxe_pool.c > > It seems OK, but we need to fix the AH problem at least in the destroy > path first - lets try to fix it in alloc as well? I do not mean to bring more efforts to Bob. But normally a new feature is merged into rdma. some test cases should also be added to rdma-core to verify this new feature. But if Bob is busy with AH problem, I am OK with it. Zhu Yanjun > > Jason
On Tue, May 10, 2022 at 08:35:34AM +0800, Yanjun Zhu wrote: > 在 2022/5/10 2:23, Jason Gunthorpe 写道: > > On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > > > > > Bob Pearson (10): > > > RDMA/rxe: Remove IB_SRQ_INIT_MASK > > > RDMA/rxe: Add rxe_srq_cleanup() > > > RDMA/rxe: Check rxe_get() return value > > > RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() > > > RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() > > > RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() > > > RDMA/rxe: Enforce IBA C11-17 > > > > I took these patches with the small edits I noted > > > > > RDMA/rxe: Stop lookup of partially built objects > > > RDMA/rxe: Convert read side locking to rcu > > > RDMA/rxe: Cleanup rxe_pool.c > > > > It seems OK, but we need to fix the AH problem at least in the destroy > > path first - lets try to fix it in alloc as well? > > I do not mean to bring more efforts to Bob. > But normally a new feature is merged into rdma. some test cases should also > be added to rdma-core to verify this new feature. It isn't a feature. Jason
On 2022/5/10 2:23, Jason Gunthorpe wrote: > On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > >> Bob Pearson (10): >> RDMA/rxe: Remove IB_SRQ_INIT_MASK >> RDMA/rxe: Add rxe_srq_cleanup() >> RDMA/rxe: Check rxe_get() return value >> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() >> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() >> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() >> RDMA/rxe: Enforce IBA C11-17 > > I took these patches with the small edits I noted > >> RDMA/rxe: Stop lookup of partially built objects >> RDMA/rxe: Convert read side locking to rcu >> RDMA/rxe: Cleanup rxe_pool.c > > It seems OK, but we need to fix the AH problem at least in the destroy > path first - lets try to fix it in alloc as well? Hi Jason, Bob Could you tell me what the AH problem is? Thanks a lot. Best Regards, Xiao Yang > > Jason
On Tue, May 24, 2022 at 03:53:30AM +0000, yangx.jy@fujitsu.com wrote: > On 2022/5/10 2:23, Jason Gunthorpe wrote: > > On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > > > >> Bob Pearson (10): > >> RDMA/rxe: Remove IB_SRQ_INIT_MASK > >> RDMA/rxe: Add rxe_srq_cleanup() > >> RDMA/rxe: Check rxe_get() return value > >> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() > >> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() > >> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() > >> RDMA/rxe: Enforce IBA C11-17 > > > > I took these patches with the small edits I noted > > > >> RDMA/rxe: Stop lookup of partially built objects > >> RDMA/rxe: Convert read side locking to rcu > >> RDMA/rxe: Cleanup rxe_pool.c > > > > It seems OK, but we need to fix the AH problem at least in the destroy > > path first - lets try to fix it in alloc as well? > Hi Jason, Bob > > Could you tell me what the AH problem is? Thanks a lot. rxe doesn't implement RDMA_CREATE_AH_SLEEPABLE / RDMA_DESTROY_AH_SLEEPABLE Jason
On 5/24/22 06:57, Jason Gunthorpe wrote: > On Tue, May 24, 2022 at 03:53:30AM +0000, yangx.jy@fujitsu.com wrote: >> On 2022/5/10 2:23, Jason Gunthorpe wrote: >>> On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: >>> >>>> Bob Pearson (10): >>>> RDMA/rxe: Remove IB_SRQ_INIT_MASK >>>> RDMA/rxe: Add rxe_srq_cleanup() >>>> RDMA/rxe: Check rxe_get() return value >>>> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() >>>> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() >>>> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() >>>> RDMA/rxe: Enforce IBA C11-17 >>> >>> I took these patches with the small edits I noted >>> >>>> RDMA/rxe: Stop lookup of partially built objects >>>> RDMA/rxe: Convert read side locking to rcu >>>> RDMA/rxe: Cleanup rxe_pool.c >>> >>> It seems OK, but we need to fix the AH problem at least in the destroy >>> path first - lets try to fix it in alloc as well? >> Hi Jason, Bob >> >> Could you tell me what the AH problem is? Thanks a lot. > > rxe doesn't implement RDMA_CREATE_AH_SLEEPABLE / > RDMA_DESTROY_AH_SLEEPABLE > > Jason First I have heard of those. Should we implement them? Bob
On Tue, May 24, 2022 at 01:22:07PM -0500, Bob Pearson wrote: > On 5/24/22 06:57, Jason Gunthorpe wrote: > > On Tue, May 24, 2022 at 03:53:30AM +0000, yangx.jy@fujitsu.com wrote: > >> On 2022/5/10 2:23, Jason Gunthorpe wrote: > >>> On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > >>> > >>>> Bob Pearson (10): > >>>> RDMA/rxe: Remove IB_SRQ_INIT_MASK > >>>> RDMA/rxe: Add rxe_srq_cleanup() > >>>> RDMA/rxe: Check rxe_get() return value > >>>> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() > >>>> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() > >>>> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() > >>>> RDMA/rxe: Enforce IBA C11-17 > >>> > >>> I took these patches with the small edits I noted > >>> > >>>> RDMA/rxe: Stop lookup of partially built objects > >>>> RDMA/rxe: Convert read side locking to rcu > >>>> RDMA/rxe: Cleanup rxe_pool.c > >>> > >>> It seems OK, but we need to fix the AH problem at least in the destroy > >>> path first - lets try to fix it in alloc as well? > >> Hi Jason, Bob > >> > >> Could you tell me what the AH problem is? Thanks a lot. > > > > rxe doesn't implement RDMA_CREATE_AH_SLEEPABLE / > > RDMA_DESTROY_AH_SLEEPABLE > > > > Jason > > First I have heard of those. Should we implement them? Yes, it is the source of all these AH lockdep bugs. Jason
On 5/24/22 13:26, Jason Gunthorpe wrote: > On Tue, May 24, 2022 at 01:22:07PM -0500, Bob Pearson wrote: >> On 5/24/22 06:57, Jason Gunthorpe wrote: >>> On Tue, May 24, 2022 at 03:53:30AM +0000, yangx.jy@fujitsu.com wrote: >>>> On 2022/5/10 2:23, Jason Gunthorpe wrote: >>>>> On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: >>>>> >>>>>> Bob Pearson (10): >>>>>> RDMA/rxe: Remove IB_SRQ_INIT_MASK >>>>>> RDMA/rxe: Add rxe_srq_cleanup() >>>>>> RDMA/rxe: Check rxe_get() return value >>>>>> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() >>>>>> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() >>>>>> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() >>>>>> RDMA/rxe: Enforce IBA C11-17 >>>>> >>>>> I took these patches with the small edits I noted >>>>> >>>>>> RDMA/rxe: Stop lookup of partially built objects >>>>>> RDMA/rxe: Convert read side locking to rcu >>>>>> RDMA/rxe: Cleanup rxe_pool.c >>>>> >>>>> It seems OK, but we need to fix the AH problem at least in the destroy >>>>> path first - lets try to fix it in alloc as well? >>>> Hi Jason, Bob >>>> >>>> Could you tell me what the AH problem is? Thanks a lot. >>> >>> rxe doesn't implement RDMA_CREATE_AH_SLEEPABLE / >>> RDMA_DESTROY_AH_SLEEPABLE >>> >>> Jason >> >> First I have heard of those. Should we implement them? > > Yes, it is the source of all these AH lockdep bugs. > > OK but what is RDMA_CREATE_AH_SLEEPABLE?
On Tue, May 24, 2022 at 01:27:49PM -0500, Bob Pearson wrote: > On 5/24/22 13:26, Jason Gunthorpe wrote: > > On Tue, May 24, 2022 at 01:22:07PM -0500, Bob Pearson wrote: > >> On 5/24/22 06:57, Jason Gunthorpe wrote: > >>> On Tue, May 24, 2022 at 03:53:30AM +0000, yangx.jy@fujitsu.com wrote: > >>>> On 2022/5/10 2:23, Jason Gunthorpe wrote: > >>>>> On Wed, Apr 20, 2022 at 08:40:33PM -0500, Bob Pearson wrote: > >>>>> > >>>>>> Bob Pearson (10): > >>>>>> RDMA/rxe: Remove IB_SRQ_INIT_MASK > >>>>>> RDMA/rxe: Add rxe_srq_cleanup() > >>>>>> RDMA/rxe: Check rxe_get() return value > >>>>>> RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() > >>>>>> RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() > >>>>>> RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() > >>>>>> RDMA/rxe: Enforce IBA C11-17 > >>>>> > >>>>> I took these patches with the small edits I noted > >>>>> > >>>>>> RDMA/rxe: Stop lookup of partially built objects > >>>>>> RDMA/rxe: Convert read side locking to rcu > >>>>>> RDMA/rxe: Cleanup rxe_pool.c > >>>>> > >>>>> It seems OK, but we need to fix the AH problem at least in the destroy > >>>>> path first - lets try to fix it in alloc as well? > >>>> Hi Jason, Bob > >>>> > >>>> Could you tell me what the AH problem is? Thanks a lot. > >>> > >>> rxe doesn't implement RDMA_CREATE_AH_SLEEPABLE / > >>> RDMA_DESTROY_AH_SLEEPABLE > >>> > >>> Jason > >> > >> First I have heard of those. Should we implement them? > > > > Yes, it is the source of all these AH lockdep bugs. > > > > > > OK but what is RDMA_CREATE_AH_SLEEPABLE? If it is not set then create_ah is not allowed to sleep. Same for destroy. Not allowed to sleep means you can't use GFP_KERNEL/etc. Jason
There are several race conditions discovered in the current rdma_rxe driver. They mostly relate to races between normal operations and destroying objects. This patch series - Makes several minor cleanups in rxe_pool.[ch] - Adds wait for completions to the paths in verbs APIs which destroy objects. - Changes read side locking to rcu. - Moves object cleanup code to after ref count is zero Signed-off-by: Bob Pearson <rpearsonhpe@gmail.com> --- v14 Rebased to current wip/jgg-for-next Removed patch 3 as unnecessary Waited for resolution of bugs in rxe_resp and some locking bugs. Note: With rcu read lock in rxe_pool_get_index there are no bottom half spinlocks from looking up AH or non AH objects to conflict with the default xa_lock so no lockdep warnings occur. The rxe_pool alloc functions can hold locks simultanteously with the rcu read lock so it does not have to prevent soft or hard IRQs. v13 Rebased to current for-next Addressed Jason's comments on patch 1, 8 and 9. 8 and 9 are combined into one patch. v12 Rebased to current wip/jgg-for-next Dropped patches already accepted from v11. Moved all object cleanup code to type specific cleanup routines. Renamed to match Jason's requests. Addressed some other issued raised. Kept the contentious rxe_hide() function but renamed to rxe_disable_lookup() which says what it does. I am still convinced this cleaner than other alternatives like moving xa_erase to the top of destroy routines but just for indexed objects. v11 Rebased to current for-next. Reordered patches and made other changes to respond to issues reported by Jason Gunthorpe. v10 Rebased to current wip/jgg-for-next. Split some patches into smaller ones. v9 Corrected issues reported by Jason Gunthorpe, Converted locking in rxe_mcast.c and rxe_pool.c to use RCU Split up the patches into smaller changes v8 Fixed an additional race in 3/8 which was not handled correctly. v7 Corrected issues reported by Jason Gunthorpe Link: https://lore.kernel.org/linux-rdma/20211207190947.GH6385@nvidia.com/ Link: https://lore.kernel.org/linux-rdma/20211207191857.GI6385@nvidia.com/ Link: https://lore.kernel.org/linux-rdma/20211207192824.GJ6385@nvidia.com/ v6 Fixed a kzalloc flags bug. Fixed comment bug reported by 'Kernel Test Robot'. Changed type of rxe_pool.c in __rxe_fini(). v5 Removed patches already accepted into for-next and addressed comments from Jason Gunthorpe. v4 Restructured patch series to change to xarray earlier which greatly simplified the changes. Rebased to current for-next v3 Changed rxe_alloc to use GFP_KERNEL Addressed other comments by Jason Gunthorp Merged the previous 06/10 and 07/10 patches into one since they overlapped Added some minor cleanups as 10/10 v2 Rebased to current for-next. Added 4 additional patches Bob Pearson (10): RDMA/rxe: Remove IB_SRQ_INIT_MASK RDMA/rxe: Add rxe_srq_cleanup() RDMA/rxe: Check rxe_get() return value RDMA/rxe: Move qp cleanup code to rxe_qp_do_cleanup() RDMA/rxe: Move mr cleanup code to rxe_mr_cleanup() RDMA/rxe: Move mw cleanup code to rxe_mw_cleanup() RDMA/rxe: Enforce IBA C11-17 RDMA/rxe: Stop lookup of partially built objects RDMA/rxe: Convert read side locking to rcu RDMA/rxe: Cleanup rxe_pool.c drivers/infiniband/sw/rxe/rxe_comp.c | 3 +- drivers/infiniband/sw/rxe/rxe_loc.h | 17 ++-- drivers/infiniband/sw/rxe/rxe_mr.c | 12 +-- drivers/infiniband/sw/rxe/rxe_mw.c | 61 ++++++------ drivers/infiniband/sw/rxe/rxe_pool.c | 84 +++++++++++++++-- drivers/infiniband/sw/rxe/rxe_pool.h | 11 ++- drivers/infiniband/sw/rxe/rxe_qp.c | 22 +++-- drivers/infiniband/sw/rxe/rxe_req.c | 3 +- drivers/infiniband/sw/rxe/rxe_resp.c | 3 +- drivers/infiniband/sw/rxe/rxe_srq.c | 129 ++++++++++++++++---------- drivers/infiniband/sw/rxe/rxe_verbs.c | 68 ++++++++------ drivers/infiniband/sw/rxe/rxe_verbs.h | 1 + 12 files changed, 266 insertions(+), 148 deletions(-) base-commit: b5a93e79df64c32814f0edefdb920b540cbc986a prerequisite-patch-id: 376fbfd86efaaa87f16eec9b4000b2db2594d404 prerequisite-patch-id: 872b59d5187d9ffc2ef5bbca0c6b92148662a8cf prerequisite-patch-id: 89ae7ec3782cb2bc6ed0861bd2df80fba5f24837 prerequisite-patch-id: 98b76b1e695ea473644df0ae427539c4593b36fb prerequisite-patch-id: deac497973b5842e6838196f509f5315afef1521