RDMA fixes for 4.8

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

In case of such bug, the reverted patch patch will cause to reentry

(EAGAIN) and kprints storm again and again.

It is undesired and misleading behaviour.

This reverts commit 5412352fcd8f ("IB/mlx4: Return EAGAIN for any error in mlx4_ib_poll_one")

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/hw/mlx4/cq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/infiniband/hw/mlx4/cq.c +3 -4

index 006db64..15b6289 100644
@@ -690,7 +690,7 @@ repoll:
 	if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) == MLX4_OPCODE_NOP &&
 		     is_send)) {
 		pr_warn("Completion for NOP opcode detected!\n");
-		return -EAGAIN;
+		return -EINVAL;
 	}
 
 	/* Resize CQ in progress */

@@ -721,7 +721,7 @@ repoll:
 		if (unlikely(!mqp)) {
 			pr_warn("CQ %06x with entry for unknown QPN %06x\n",
 			       cq->mcq.cqn, be32_to_cpu(cqe->vlan_my_qpn) & MLX4_CQE_QPN_MASK);
-			return -EAGAIN;
+			return -EINVAL;
 		}
 
 		*cur_qp = to_mibqp(mqp);

@@ -739,7 +739,7 @@ repoll:
 		if (unlikely(!msrq)) {
 			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
 				cq->mcq.cqn, srq_num);
-			return -EAGAIN;
+			return -EINVAL;
 		}
 	}
 
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 6 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

In case of such bug, the reverted patch patch will cause to reentry

s/patch patch/atch

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Quoted 6 lines

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

See patches 4 and 6, they completely removed these EINVALs.

Quoted 8 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Quoted 6 lines

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

See patches 4 and 6, they completely removed these EINVALs.

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

Quoted 1 line

See patches 4 and 6, they completely removed these EINVALs.

So the commit message is still wrong.

Why do we need this revert? Just squash it and mark it fixup the original.

Jason

We need this revert, because the original commit is wrong and as was presented

Quoted 17 lines · Jason Gunthorpe

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

Quoted 6 lines

Quoted 1 line

See patches 4 and 6, they completely removed these EINVALs.

So the commit message is still wrong.

Why do we need this revert? Just squash it and mark it fixup the original.

We need this revert, because the original commit is wrong and as was presented by Sasha Levin in his talk about automatic creation of stable trees [1], he needs this information to ensure that this commit won't be in stable tree by mistake.

[1]
https://lcccna2016.sched.org/event/7JW4/automating-the-creation-of-stable-trees-sasha-levin-verizon-labs

Quoted 29 lines · Leon Romanovsky

Quoted 17 lines · Jason Gunthorpe

Quoted 16 lines · Leon Romanovsky

Quoted 14 lines · Jason Gunthorpe

Quoted 4 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

By Mellanox HW design and SW implementation poll_cq never fails and returns errors, so all these prints are to catch ULP bugs.

Eh? How can a ULP cause poll_cq to get errors?

Are you sure these are not driver bugs?

Why can't you just print and discard the broken CQ entry?

What should use ULP do when it get EINVAL? You say poll again is not correct, so you suggest a full QP tear down?

Quoted 6 lines

Quoted 1 line

See patches 4 and 6, they completely removed these EINVALs.

So the commit message is still wrong.

Why do we need this revert? Just squash it and mark it fixup the original.

We need this revert, because the original commit is wrong and as was presented by Sasha Levin in his talk about automatic creation of stable trees [1], he needs this information to ensure that this commit won't be in stable tree by mistake.

Quoted 2 lines

[1]
https://lcccna2016.sched.org/event/7JW4/automating-the-creation-of-stable-trees-sasha-levin-verizon-labs

I didn't see the presentation at this link, but I'm not sure I agree with your statement here. There should be no difference, effectively, between a commit to revert a bad commit, and a commit to fix a bad commit that uses a Fixes: tag referencing the original commit.

--
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD

Quoted 10 lines

		/* SRQ is also in the radix tree */
		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
				       srq_num);

(quoted diff) +0 -5

-		if (unlikely(!msrq)) {
-			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
-				cq->mcq.cqn, srq_num);
-			return -EINVAL;
-		}

	}

BTW, this is completely unrelated to this patch, but the current implementation of shared receive queues in Mellanox drivers is *very* inefficient. Each completion that relates to a srq the mlx4/mlx5 drivers perform a device-wide locked srq lookup which is pretty bad... (it kills consumers that want to use more then one SRQ)

Other drivers that support kernel SRQs that I've looked at are ocrdma and i40iw and they don't seem to have this lock everything approach.

At the very-least we should try to make it a rcu + percpu_ref instead of a killer device-wide lock. It'd be even better if we use refcounting in the IB core and have the drivers not worry about the kernel consumers destroying SRQs while processing IO (i.e. when all the related QPs and CQs are destroyed).

I have some code in the works on this but it's not high on my todo list at the moment. Mellanox folks, any thoughts on this?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 18 lines · Sagi Grimberg

Quoted 10 lines

		/* SRQ is also in the radix tree */
		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
				       srq_num);

(quoted diff) +0 -5

-		if (unlikely(!msrq)) {
-			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
-				cq->mcq.cqn, srq_num);
-			return -EINVAL;
-		}

	}

BTW, this is completely unrelated to this patch, but the current implementation of shared receive queues in Mellanox drivers is *very* inefficient. Each completion that relates to a srq the mlx4/mlx5 drivers perform a device-wide locked srq lookup which is pretty bad... (it kills consumers that want to use more then one SRQ)

Quoted 8 lines

Other drivers that support kernel SRQs that I've looked at are ocrdma and i40iw and they don't seem to have this lock everything approach.

At the very-least we should try to make it a rcu + percpu_ref instead of a killer device-wide lock. It'd be even better if we use refcounting in the IB core and have the drivers not worry about the kernel consumers destroying SRQs while processing IO (i.e. when all the related QPs and CQs are destroyed).

This can be as a beginning.

drivers/net/ethernet/mellanox/mlx4/srq.c +0 -0

index 6714662..e53d366 100644
@@ -303,10 +303,10 @@ struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn)
 	struct mlx4_srq *srq;
 	unsigned long flags;

Quoted 8 lines

I have some code in the works on this but it's not high on my todo list at the moment. Mellanox folks, any thoughts on this?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 36 lines · Leon Romanovsky

Quoted 18 lines · Sagi Grimberg

Quoted 10 lines

		/* SRQ is also in the radix tree */
		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
				       srq_num);

(quoted diff) +0 -5

-		if (unlikely(!msrq)) {
-			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
-				cq->mcq.cqn, srq_num);
-			return -EINVAL;
-		}

	}

BTW, this is completely unrelated to this patch, but the current implementation of shared receive queues in Mellanox drivers is *very* inefficient. Each completion that relates to a srq the mlx4/mlx5 drivers perform a device-wide locked srq lookup which is pretty bad... (it kills consumers that want to use more then one SRQ)

Quoted 8 lines

Other drivers that support kernel SRQs that I've looked at are ocrdma and i40iw and they don't seem to have this lock everything approach.

At the very-least we should try to make it a rcu + percpu_ref instead of a killer device-wide lock. It'd be even better if we use refcounting in the IB core and have the drivers not worry about the kernel consumers destroying SRQs while processing IO (i.e. when all the related QPs and CQs are destroyed).

This can be as a beginning.

drivers/net/ethernet/mellanox/mlx4/srq.c +0 -0

index 6714662..e53d366 100644
@@ -303,10 +303,10 @@ struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn)
 	struct mlx4_srq *srq;
 	unsigned long flags;

drivers/net/ethernet/mellanox/mlx4/srq.c struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn) · line 312

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/srq.c +2 -2

 
-	spin_lock_irqsave(&srq_table->lock, flags);
+	rcu_read_lock();
 	srq = radix_tree_lookup(&srq_table->tree,
 				srqn & (dev->caps.num_srqs - 1));
-	spin_unlock_irqrestore(&srq_table->lock, flags);
+	rcu_read_unlock();
 
 	return srq;
 }

Or more complete patch (untested),

From 8695e5b807610e08de055b04ddd16caa6a5b61f2 Mon Sep 17 00:00:00 2001
From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
Date: Mon, 29 Aug 2016 12:42:24 +0300
Subject: [PATCH] IB/mlx4: Use RCU to perform radix tree lookup for SRQ

Radix tree lookup can be performed without locking.

Suggested-by Sagi Grimberg <[email protected]>

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/net/ethernet/mellanox/mlx4/srq.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

drivers/net/ethernet/mellanox/mlx4/srq.c +5 -10

index 6714662..f44d089 100644
@@ -45,15 +45,12 @@ void mlx4_srq_event(struct mlx4_dev *dev, u32 srqn, int event_type)
 	struct mlx4_srq_table *srq_table = &mlx4_priv(dev)->srq_table;
 	struct mlx4_srq *srq;
 
-	spin_lock(&srq_table->lock);
-
+	rcu_read_lock();
 	srq = radix_tree_lookup(&srq_table->tree, srqn & (dev->caps.num_srqs - 1));
+	rcu_read_unlock();
 	if (srq)
 		atomic_inc(&srq->refcount);
-
-	spin_unlock(&srq_table->lock);
-
-	if (!srq) {
+	else {
 		mlx4_warn(dev, "Async event for bogus SRQ %08x\n", srqn);
 		return;
 	}

@@ -301,12 +298,11 @@ struct mlx4_srq *mlx4_srq_lookup(struct mlx4_dev *dev, u32 srqn)
 {
 	struct mlx4_srq_table *srq_table = &mlx4_priv(dev)->srq_table;
 	struct mlx4_srq *srq;
-	unsigned long flags;
 
-	spin_lock_irqsave(&srq_table->lock, flags);
+	rcu_read_lock();
 	srq = radix_tree_lookup(&srq_table->tree,
 				srqn & (dev->caps.num_srqs - 1));
-	spin_unlock_irqrestore(&srq_table->lock, flags);
+	rcu_read_unlock();
 
 	return srq;
 }
--

2.7.4

Quoted 8 lines

Quoted 8 lines

I have some code in the works on this but it's not high on my todo list at the moment. Mellanox folks, any thoughts on this?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

This patch introduces new MLX5_INFINIBAND_VERBOSE Kconfig option to enable the output of dump_wqe() which was never printed.

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/hw/mlx5/Kconfig | 6 ++++++
 drivers/infiniband/hw/mlx5/qp.c    | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

drivers/infiniband/hw/mlx5/Kconfig +6 -0

index bce263b..789fef4 100644
@@ -6,3 +6,9 @@ config MLX5_INFINIBAND
 	  Mellanox Connect-IB PCI Express host channel adapters (HCAs).
 	  This is required to use InfiniBand protocols such as
 	  IP-over-IB or SRP with these devices.
+
+config MLX5_INFINIBAND_VERBOSE
+	bool "Enable ConnectX-4/Connect-IB verbosity"
+	depends on MLX5_INFINIBAND
+	---help---
+	  This is a configuration option to enable verbose output to easy debug.

drivers/infiniband/hw/mlx5/qp.c +0 -0

index 174d09b..67b58f7 100644

Quoted 33 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

This patch introduces new MLX5_INFINIBAND_VERBOSE Kconfig option to enable the output of dump_wqe() which was never printed.

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/hw/mlx5/Kconfig | 6 ++++++
 drivers/infiniband/hw/mlx5/qp.c    | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)

drivers/infiniband/hw/mlx5/Kconfig +6 -0

index bce263b..789fef4 100644
@@ -6,3 +6,9 @@ config MLX5_INFINIBAND
 	  Mellanox Connect-IB PCI Express host channel adapters (HCAs).
 	  This is required to use InfiniBand protocols such as
 	  IP-over-IB or SRP with these devices.
+
+config MLX5_INFINIBAND_VERBOSE
+	bool "Enable ConnectX-4/Connect-IB verbosity"
+	depends on MLX5_INFINIBAND
+	---help---
+	  This is a configuration option to enable verbose output to easy debug.

drivers/infiniband/hw/mlx5/qp.c +0 -0

index 174d09b..67b58f7 100644
@@ -3984,7 +3984,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 			   get_fence(fence, wr), next_fence,
 			   mlx5_ib_opcode[wr->opcode]);

drivers/infiniband/hw/mlx5/qp.c int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr, · line 3989

Quoted 5 lines

drivers/infiniband/hw/mlx5/qp.c +1 -1

 skip_psv:
-		if (0)
+		if (IS_BUILTIN(CONFIG_MLX5_INFINIBAND_VERBOSE))
 			dump_wqe(qp, idx, size);
 	}

This patch is just a big old bunch of no. There is no way I'm sending in Kconfig changes in a late -rc.

Further, there is no way I'm sending
in a Kconfig option for a single debug print function.

Third, there is
no way I'm sending in a Kconfig setting for what should be a runtime debug switch or something like that.

--
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD

Hi Doug,

This is compilation of fixes to RDMA core, IPoIB, Mellanox mlx4 and mlx5 drivers all together. Most of the fixes are one liners and for easy handling, I'm sending them in one patchset.

Thanks

Chuck Lever (1):
IB/mlx5: Return EINVAL when caller specifies too many SGEs

Erez Shitrit (2):
IB/core: Fix use after free in send_leave function IB/ipoib: Fix memory corruption during ipoib cm connection
establishment

Leon Romanovsky (5):
Revert "IB/mlx4: Return EAGAIN for any error in mlx4_ib_poll_one" IB/mlx4: Don't return errors from poll_cq
IB/mlx5: Simplify code by removing return variable IB/mlx5: Add VERBOSITY Kconfig option
IB/mlx5: Don't return errors from poll_cq

Yishai Hadas (1):
IB/mlx5: Use TIR number based on selector

 drivers/infiniband/core/multicast.c       | 13 ++-----------
 drivers/infiniband/hw/mlx4/cq.c           | 26 ++------------------------
 drivers/infiniband/hw/mlx5/Kconfig        |  6 ++++++
 drivers/infiniband/hw/mlx5/cq.c           | 22 ++--------------------
 drivers/infiniband/hw/mlx5/main.c         |  6 +++++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h      |  1 +
 drivers/infiniband/hw/mlx5/qp.c           | 15 ++++++---------
 drivers/infiniband/ulp/ipoib/ipoib.h      |  1 +
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 16 ++++++++++++++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  2 +-
 10 files changed, 42 insertions(+), 66 deletions(-)

Quoted 26 lines · Leon Romanovsky

Hi Doug,

This is compilation of fixes to RDMA core, IPoIB, Mellanox mlx4 and mlx5 drivers all together. Most of the fixes are one liners and for easy handling, I'm sending them in one patchset.

Thanks

Chuck Lever (1):
IB/mlx5: Return EINVAL when caller specifies too many SGEs

Erez Shitrit (2):
IB/core: Fix use after free in send_leave function IB/ipoib: Fix memory corruption during ipoib cm connection
establishment

Leon Romanovsky (5):
Revert "IB/mlx4: Return EAGAIN for any error in mlx4_ib_poll_one" IB/mlx4: Don't return errors from poll_cq
IB/mlx5: Simplify code by removing return variable IB/mlx5: Add VERBOSITY Kconfig option
IB/mlx5: Don't return errors from poll_cq

Yishai Hadas (1):
IB/mlx5: Use TIR number based on selector

Quoted 11 lines

 drivers/infiniband/core/multicast.c       | 13 ++-----------
 drivers/infiniband/hw/mlx4/cq.c           | 26 ++------------------------
 drivers/infiniband/hw/mlx5/Kconfig        |  6 ++++++
 drivers/infiniband/hw/mlx5/cq.c           | 22 ++--------------------
 drivers/infiniband/hw/mlx5/main.c         |  6 +++++-
 drivers/infiniband/hw/mlx5/mlx5_ib.h      |  1 +
 drivers/infiniband/hw/mlx5/qp.c           | 15 ++++++---------
 drivers/infiniband/ulp/ipoib/ipoib.h      |  1 +
 drivers/infiniband/ulp/ipoib/ipoib_cm.c   | 16 ++++++++++++++++
 drivers/infiniband/ulp/ipoib/ipoib_main.c |  2 +-
 10 files changed, 42 insertions(+), 66 deletions(-)

I took everything except patch 7. That one had to go. It would take a very strong argument about why that needs a Kconfig option instead of a runtime config switch to make me change my mind.

--
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD