Re: [PATCH rdma-next v2 0/9] Restore failure of destroy commands

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Sep 07, 2020 at 03:09:12PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@nvidia.com>
> 
> Changelog:
> v2:
>  * Rebased on top of the 524d8ffd07f0
>  * Removed "udata" check in destroy flows
>  * Changed ib_free_cq to return early
>  * Used Jason's suggestion to implement "RDMA/mlx5: Issue FW command to destroy
>    SRQ on reentry" patch.
> v1
>  * Changed returned value in efa_destroy_ah() from EINVAL to EOPNOTSUPP
>  * https://lore.kernel.org/lkml/20200830084010.102381-1-leon@kernel.org
> v0:
>  * https://lore.kernel.org/lkml/20200824103247.1088464-1-leon@kernel.org
> 
> Hi,
> 
> This series restores the ability to fail on destroy commands, due to the
> fact that mlx5_ib DEVX implementation interleaved ib_core objects
> with FW objects without sharing reference counters.
> 
> In retrospect, every part of the mlx5_ib flow is correct.
> 
> It started from IBTA which was written by HW engineers with HW in mind and
> they allowed to fail in destruction. FW implemented it with symmetrical
> interface like any other command and propagated error back to the kernel,
> which forwarded it to the libibverbs and kernel ULPs.
> 
> Libibverbs was designed with IBTA spec in hand putting destroy errors in
> stone. Up till mlx5_ib DEVX, it worked well, because the IB verbs objects
> are counted by the kernel and ib_core ensures that FW destroy will success
> by managing various reference counters on such objects.
> 
> The extension of the mlx5 driver changed this flow when allowed DEVX objects
> that are not managed by ib_core to be interleaved with the ones under ib_core
> responsibility.
> 
> The drivers that want to implement DEVX flows must ensure that FW/HW
> destroys are performed as early as possible before any other internal
> cleanup. After HW destroys, drivers are not allowed to fail.
> 
> This series includes two patches (WQ and "potential race") that will
> require extra work in mlx5_ib, they both theoretical. WQ is not in use
> in DEVX, but is needed to make interface symmetrical to other objects.
> "Potential race" is in ULP flow that ensures that SRQ is destroyed in
> proper order.
> 
> Thanks
> 
> Leon Romanovsky (9):
>   RDMA: Restore ability to fail on PD deallocate
>   RDMA: Restore ability to fail on AH destroy
>   RDMA/mlx5: Issue FW command to destroy SRQ on reentry
>   RDMA: Restore ability to fail on SRQ destroy
>   RDMA/core: Delete function indirection for alloc/free kernel CQ
>   RDMA: Allow fail of destroy CQ
>   RDMA: Change XRCD destroy return value
>   RDMA: Restore ability to return error for destroy WQ
>   RDMA: Make counters destroy symmetrical

Thanks, applied to for-next with the changes I noted:

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index b2381e01bf6345..35e5bbb44d3d8e 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -1031,15 +1031,14 @@ int mlx5_ib_destroy_cq(struct ib_cq *cq, struct ib_udata *udata)
 	int ret;
 
 	ret = mlx5_core_destroy_cq(dev->mdev, &mcq->mcq);
-	if (ret && udata)
+	if (ret)
 		return ret;
 
-	if (udata) {
+	if (udata)
 		destroy_cq_user(mcq, udata);
-		return 0;
-	}
-	destroy_cq_kernel(dev, mcq);
-	return ret;
+	else
+		destroy_cq_kernel(dev, mcq);
+	return 0;
 }
 
 static int is_equal_rsn(struct mlx5_cqe64 *cqe64, u32 rsn)
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 039f55fd067640..6dfdc13bc36395 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -5092,11 +5092,11 @@ int mlx5_ib_destroy_wq(struct ib_wq *wq, struct ib_udata *udata)
 	int ret;
 
 	ret = mlx5_core_destroy_rq_tracked(dev, &rwq->core_qp);
-	if (ret && udata)
+	if (ret)
 		return ret;
 	destroy_user_rq(dev, wq->pd, rwq, udata);
 	kfree(rwq);
-	return ret;
+	return 0;
 }
 
 struct ib_rwq_ind_table *mlx5_ib_create_rwq_ind_table(struct ib_device *device,
diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 6789b8a6927467..e2f720eec1e18b 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -396,17 +396,14 @@ int mlx5_ib_destroy_srq(struct ib_srq *srq, struct ib_udata *udata)
 	int ret;
 
 	ret = mlx5_cmd_destroy_srq(dev, &msrq->msrq);
-	if (ret && udata)
+	if (ret)
 		return ret;
 
-	if (udata) {
+	if (udata)
 		destroy_srq_user(srq->pd, msrq, udata);
-		return 0;
-	}
-
-	/* We are cleaning kernel resources anyway */
-	destroy_srq_kernel(dev, msrq);
-	return ret;
+	else
+		destroy_srq_kernel(dev, msrq);
+	return 0;
 }
 
 void mlx5_ib_free_srq_wqe(struct mlx5_ib_srq *srq, int wqe_index)
diff --git a/drivers/infiniband/hw/mlx5/srq_cmd.c b/drivers/infiniband/hw/mlx5/srq_cmd.c
index 1a707c2d364c1f..db889ec3fd48e8 100644
--- a/drivers/infiniband/hw/mlx5/srq_cmd.c
+++ b/drivers/infiniband/hw/mlx5/srq_cmd.c
@@ -598,7 +598,7 @@ int mlx5_cmd_destroy_srq(struct mlx5_ib_dev *dev, struct mlx5_core_srq *srq)
 
 	/* Delete entry, but leave index occupied */
 	tmp = xa_cmpxchg_irq(&table->array, srq->srqn, srq, XA_ZERO_ENTRY, 0);
-	if (WARN_ON(!tmp || tmp != srq) || xa_err(tmp))
+	if (WARN_ON(tmp != srq))
 		return xa_err(tmp) ?: -EINVAL;
 
 	err = destroy_srq_split(dev, srq);


Jason