[ofa-general] error with ibv_poll_cq() call

[Roland Dreier]

 > It looks like we have a race condition in mlx4_destroy_qp.  We clean the
 > cq BEFORE modifying the QP to reset (done in kernel as part of
 > the ibv_cmd_destroy_qp() flow).

ouch, you're right.  however I think your patch is not the best way to
handle this:

 > -	mlx4_cq_clean(to_mcq(ibqp->recv_cq), ibqp->qp_num,
 > -		       ibqp->srq ? to_msrq(ibqp->srq) : NULL);
 > -	if (ibqp->send_cq != ibqp->recv_cq)
 > -		mlx4_cq_clean(to_mcq(ibqp->send_cq), ibqp->qp_num, NULL);
 > -
 >  	mlx4_lock_cqs(ibqp);
 >  	mlx4_clear_qp(to_mctx(ibqp->context), ibqp->qp_num);
 >  	mlx4_unlock_cqs(ibqp);
 > @@ -576,6 +571,11 @@ int mlx4_destroy_qp(struct ibv_qp *ibqp)
 >  		return ret;
 >  	}
 >  
 > +	mlx4_cq_clean(to_mcq(ibqp->recv_cq), ibqp->qp_num,
 > +		       ibqp->srq ? to_msrq(ibqp->srq) : NULL);
 > +	if (ibqp->send_cq != ibqp->recv_cq)
 > +		mlx4_cq_clean(to_mcq(ibqp->send_cq), ibqp->qp_num, NULL);
 > +

because you leave the clear_qp call before the cq_clean and they are
not within the same cq lock, there is a window where a consumer could
poll a valid CQE for a qp that is no longer in the table.  I think
it's better to really match the logic of the kernel driver (which I
think is correct).

untested patch below:

diff --git a/src/cq.c b/src/cq.c
index 91297e4..53c6e06 100644
--- a/src/cq.c
+++ b/src/cq.c
@@ -381,15 +381,13 @@ void mlx4_cq_event(struct ibv_cq *cq)
 	to_mcq(cq)->arm_sn++;
 }
 
-void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq)
+void __mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq)
 {
 	struct mlx4_cqe *cqe, *dest;
 	uint32_t prod_index;
 	uint8_t owner_bit;
 	int nfreed = 0;
 
-	pthread_spin_lock(&cq->lock);
-
 	/*
 	 * First we need to find the current producer index, so we
 	 * know where to start cleaning from.  It doesn't matter if HW
@@ -429,7 +427,12 @@ void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq)
 		wmb();
 		update_cons_index(cq);
 	}
+}
 
+void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq)
+{
+	pthread_spin_lock(&cq->lock);
+	__mlx4_cq_clean(cq, qpn, srq);
 	pthread_spin_unlock(&cq->lock);
 }
 
diff --git a/src/mlx4.h b/src/mlx4.h
index 3710a17..9b0ad6a 100644
--- a/src/mlx4.h
+++ b/src/mlx4.h
@@ -312,8 +312,8 @@ int mlx4_destroy_cq(struct ibv_cq *cq);
 int mlx4_poll_cq(struct ibv_cq *cq, int ne, struct ibv_wc *wc);
 int mlx4_arm_cq(struct ibv_cq *cq, int solicited);
 void mlx4_cq_event(struct ibv_cq *cq);
-void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn,
-		    struct mlx4_srq *srq);
+__void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq);
+void mlx4_cq_clean(struct mlx4_cq *cq, uint32_t qpn, struct mlx4_srq *srq);
 void mlx4_cq_resize_copy_cqes(struct mlx4_cq *cq, void *buf, int new_cqe);
 
 struct ibv_srq *mlx4_create_srq(struct ibv_pd *pd,
diff --git a/src/verbs.c b/src/verbs.c
index 50e0947..26174f8 100644
--- a/src/verbs.c
+++ b/src/verbs.c
@@ -532,23 +532,20 @@ int mlx4_destroy_qp(struct ibv_qp *ibqp)
 	struct mlx4_qp *qp = to_mqp(ibqp);
 	int ret;
 
-	mlx4_cq_clean(to_mcq(ibqp->recv_cq), ibqp->qp_num,
-		       ibqp->srq ? to_msrq(ibqp->srq) : NULL);
-	if (ibqp->send_cq != ibqp->recv_cq)
-		mlx4_cq_clean(to_mcq(ibqp->send_cq), ibqp->qp_num, NULL);
+	ret = ibv_cmd_destroy_qp(ibqp);
+	if (ret)
+		return ret;
 
 	mlx4_lock_cqs(ibqp);
-	mlx4_clear_qp(to_mctx(ibqp->context), ibqp->qp_num);
-	mlx4_unlock_cqs(ibqp);
 
-	ret = ibv_cmd_destroy_qp(ibqp);
-	if (ret) {
-		mlx4_lock_cqs(ibqp);
-		mlx4_store_qp(to_mctx(ibqp->context), ibqp->qp_num, qp);
-		mlx4_unlock_cqs(ibqp);
+	__mlx4_cq_clean(to_mcq(ibqp->recv_cq), ibqp->qp_num,
+			ibqp->srq ? to_msrq(ibqp->srq) : NULL);
+	if (ibqp->send_cq != ibqp->recv_cq)
+		__mlx4_cq_clean(to_mcq(ibqp->send_cq), ibqp->qp_num, NULL);
 
-		return ret;
-	}
+	mlx4_clear_qp(to_mctx(ibqp->context), ibqp->qp_num);
+
+	mlx4_unlock_cqs(ibqp);
 
 	if (!ibqp->srq)
 		mlx4_free_db(to_mctx(ibqp->context), MLX4_DB_TYPE_RQ, qp->db);