[rdma-rc,4/9] IB/mlx4: Don't return errors from poll_cq

Commit Message

Leon Romanovsky Aug. 28, 2016, 7:58 a.m. UTC

From: Leon Romanovsky <[e-mail]>

Remove returning errors from mlx4 poll_cq function. Polling CQ
operation in kernel never fails by Mellanox HCA architecture and
respective driver design.

Signed-off-by: Leon Romanovsky <[e-mail]>
---
 drivers/infiniband/hw/mlx4/cq.c | 26 ++------------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

--
2.7.4

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

Comments

Sagi Grimberg Aug. 28, 2016, 4:05 p.m. UTC | #1

>  		/* SRQ is also in the radix tree */
>  		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
>  				       srq_num);
> -		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 [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/infiniband/hw/mlx4/cq.c b/drivers/infiniband/hw/mlx4/cq.c
index 15b6289..5df63da 100644
--- a/drivers/infiniband/hw/mlx4/cq.c
+++ b/drivers/infiniband/hw/mlx4/cq.c
@@ -687,12 +687,6 @@  repoll:
 	is_error = (cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) ==
 		MLX4_CQE_OPCODE_ERROR;

-	if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) == MLX4_OPCODE_NOP &&
-		     is_send)) {
-		pr_warn("Completion for NOP opcode detected!\n");
-		return -EINVAL;
-	}
-
 	/* Resize CQ in progress */
 	if (unlikely((cqe->owner_sr_opcode & MLX4_CQE_OPCODE_MASK) == MLX4_CQE_OPCODE_RESIZE)) {
 		if (cq->resize_buf) {
@@ -718,12 +712,6 @@  repoll:
 		 */
 		mqp = __mlx4_qp_lookup(to_mdev(cq->ibcq.device)->dev,
 				       be32_to_cpu(cqe->vlan_my_qpn));
-		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 -EINVAL;
-		}
-
 		*cur_qp = to_mibqp(mqp);
 	}

@@ -736,11 +724,6 @@  repoll:
 		/* SRQ is also in the radix tree */
 		msrq = mlx4_srq_lookup(to_mdev(cq->ibcq.device)->dev,
 				       srq_num);
-		if (unlikely(!msrq)) {
-			pr_warn("CQ %06x with entry for unknown SRQN %06x\n",
-				cq->mcq.cqn, srq_num);
-			return -EINVAL;
-		}
 	}

 	if (is_send) {
@@ -891,7 +874,6 @@  int mlx4_ib_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc)
 	struct mlx4_ib_qp *cur_qp = NULL;
 	unsigned long flags;
 	int npolled;
-	int err = 0;
 	struct mlx4_ib_dev *mdev = to_mdev(cq->ibcq.device);

 	spin_lock_irqsave(&cq->lock, flags);
@@ -901,8 +883,7 @@  int mlx4_ib_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc)
 	}

 	for (npolled = 0; npolled < num_entries; ++npolled) {
-		err = mlx4_ib_poll_one(cq, &cur_qp, wc + npolled);
-		if (err)
+		if (mlx4_ib_poll_one(cq, &cur_qp, wc + npolled))
 			break;
 	}

@@ -911,10 +892,7 @@  int mlx4_ib_poll_cq(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc)
 out:
 	spin_unlock_irqrestore(&cq->lock, flags);

-	if (err == 0 || err == -EAGAIN)
-		return npolled;
-	else
-		return err;
+	return npolled;
 }

 int mlx4_ib_arm_cq(struct ib_cq *ibcq, enum ib_cq_notify_flags flags)