[openib-general] Re: [PATCH] cm refcount race fix

[Roland Dreier]

It does seem we can simplify mthca_cq in a slightly different way.
mthca_cq_clean() doesn't need to take a CQ reference, because we know
the CQ can't go away before all associated QPs are gone, and at least
one QP will stay around until mthca_cq_clean() returns.

So the below patch is both a fix and a decent cleanup:

--- infiniband/hw/mthca/mthca_provider.h	(revision 6945)
+++ infiniband/hw/mthca/mthca_provider.h	(working copy)
@@ -197,7 +197,7 @@ struct mthca_cq_resize {
 struct mthca_cq {
 	struct ib_cq		ibcq;
 	spinlock_t		lock;
-	atomic_t		refcount;
+	int			refcount;
 	int			cqn;
 	u32			cons_index;
 	struct mthca_cq_buf	buf;
--- infiniband/hw/mthca/mthca_dev.h	(revision 6945)
+++ infiniband/hw/mthca/mthca_dev.h	(working copy)
@@ -496,7 +496,7 @@ void mthca_free_cq(struct mthca_dev *dev
 void mthca_cq_completion(struct mthca_dev *dev, u32 cqn);
 void mthca_cq_event(struct mthca_dev *dev, u32 cqn,
 		    enum ib_event_type event_type);
-void mthca_cq_clean(struct mthca_dev *dev, u32 cqn, u32 qpn,
+void mthca_cq_clean(struct mthca_dev *dev, struct mthca_cq *cq, u32 qpn,
 		    struct mthca_srq *srq);
 void mthca_cq_resize_copy_cqes(struct mthca_cq *cq);
 int mthca_alloc_cq_buf(struct mthca_dev *dev, struct mthca_cq_buf *buf, int nent);
--- infiniband/hw/mthca/mthca_cq.c	(revision 6945)
+++ infiniband/hw/mthca/mthca_cq.c	(working copy)
@@ -234,14 +234,19 @@ void mthca_cq_event(struct mthca_dev *de
 {
 	struct mthca_cq *cq;
 	struct ib_event event;
+	unsigned long flags;
 
-	spin_lock(&dev->cq_table.lock);
+	spin_lock_irqsave(&dev->cq_table.lock, flags);
 
 	cq = mthca_array_get(&dev->cq_table.cq, cqn & (dev->limits.num_cqs - 1));
 
-	if (cq)
-		atomic_inc(&cq->refcount);
-	spin_unlock(&dev->cq_table.lock);
+	if (cq) {
+		spin_lock(&cq->lock);
+		++cq->refcount;
+		spin_unlock(&cq->lock);
+	}
+
+	spin_unlock_irqrestore(&dev->cq_table.lock, flags);
 
 	if (!cq) {
 		mthca_warn(dev, "Async event for bogus CQ %08x\n", cqn);
@@ -254,8 +259,10 @@ void mthca_cq_event(struct mthca_dev *de
 	if (cq->ibcq.event_handler)
 		cq->ibcq.event_handler(&event, cq->ibcq.cq_context);
 
-	if (atomic_dec_and_test(&cq->refcount))
+	spin_lock_irqsave(&cq->lock, flags);
+	if (!--cq->refcount)
 		wake_up(&cq->wait);
+	spin_unlock_irqrestore(&cq->lock, flags);
 }
 
 static inline int is_recv_cqe(struct mthca_cqe *cqe)
@@ -267,23 +274,13 @@ static inline int is_recv_cqe(struct mth
 		return !(cqe->is_send & 0x80);
 }
 
-void mthca_cq_clean(struct mthca_dev *dev, u32 cqn, u32 qpn,
+void mthca_cq_clean(struct mthca_dev *dev, struct mthca_cq *cq, u32 qpn,
 		    struct mthca_srq *srq)
 {
-	struct mthca_cq *cq;
 	struct mthca_cqe *cqe;
 	u32 prod_index;
 	int nfreed = 0;
 
-	spin_lock_irq(&dev->cq_table.lock);
-	cq = mthca_array_get(&dev->cq_table.cq, cqn & (dev->limits.num_cqs - 1));
-	if (cq)
-		atomic_inc(&cq->refcount);
-	spin_unlock_irq(&dev->cq_table.lock);
-
-	if (!cq)
-		return;
-
 	spin_lock_irq(&cq->lock);
 
 	/*
@@ -301,7 +298,7 @@ void mthca_cq_clean(struct mthca_dev *de
 
 	if (0)
 		mthca_dbg(dev, "Cleaning QPN %06x from CQN %06x; ci %d, pi %d\n",
-			  qpn, cqn, cq->cons_index, prod_index);
+			  qpn, cq->cqn, cq->cons_index, prod_index);
 
 	/*
 	 * Now sweep backwards through the CQ, removing CQ entries
@@ -323,10 +320,6 @@ void mthca_cq_clean(struct mthca_dev *de
 		cq->cons_index += nfreed;
 		update_cons_index(dev, cq, nfreed);
 	}
-
-	spin_unlock_irq(&cq->lock);
-	if (atomic_dec_and_test(&cq->refcount))
-		wake_up(&cq->wait);
 }
 
 void mthca_cq_resize_copy_cqes(struct mthca_cq *cq)
@@ -821,7 +814,7 @@ int mthca_init_cq(struct mthca_dev *dev,
 	}
 
 	spin_lock_init(&cq->lock);
-	atomic_set(&cq->refcount, 1);
+	cq->refcount = 1;
 	init_waitqueue_head(&cq->wait);
 
 	memset(cq_context, 0, sizeof *cq_context);
@@ -896,6 +889,17 @@ err_out:
 	return err;
 }
 
+static int get_cq_refcount(struct mthca_cq *cq)
+{
+	int c;
+
+	spin_lock_irq(&cq->lock);
+	c = cq->refcount;
+	spin_unlock_irq(&cq->lock);
+
+	return c;
+}
+
 void mthca_free_cq(struct mthca_dev *dev,
 		   struct mthca_cq *cq)
 {
@@ -936,8 +940,11 @@ void mthca_free_cq(struct mthca_dev *dev
 	else
 		synchronize_irq(dev->pdev->irq);
 
-	atomic_dec(&cq->refcount);
-	wait_event(cq->wait, !atomic_read(&cq->refcount));
+	spin_lock_irq(&cq->lock);
+	--cq->refcount;
+	spin_unlock_irq(&cq->lock);
+
+	wait_event(cq->wait, !get_cq_refcount(cq));
 
 	if (cq->is_kernel) {
 		mthca_free_cq_buf(dev, &cq->buf, cq->ibcq.cqe);
--- infiniband/hw/mthca/mthca_qp.c	(revision 6945)
+++ infiniband/hw/mthca/mthca_qp.c	(working copy)
@@ -831,10 +831,10 @@ int mthca_modify_qp(struct ib_qp *ibqp, 
 	 * entries and reinitialize the QP.
 	 */
 	if (new_state == IB_QPS_RESET && !qp->ibqp.uobject) {
-		mthca_cq_clean(dev, to_mcq(qp->ibqp.send_cq)->cqn, qp->qpn,
+		mthca_cq_clean(dev, to_mcq(qp->ibqp.send_cq), qp->qpn,
 			       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 		if (qp->ibqp.send_cq != qp->ibqp.recv_cq)
-			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq)->cqn, qp->qpn,
+			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
 				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 
 		mthca_wq_init(&qp->sq);
@@ -1356,10 +1356,10 @@ void mthca_free_qp(struct mthca_dev *dev
 	 * unref the mem-free tables and free the QPN in our table.
 	 */
 	if (!qp->ibqp.uobject) {
-		mthca_cq_clean(dev, to_mcq(qp->ibqp.send_cq)->cqn, qp->qpn,
+		mthca_cq_clean(dev, to_mcq(qp->ibqp.send_cq), qp->qpn,
 			       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 		if (qp->ibqp.send_cq != qp->ibqp.recv_cq)
-			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq)->cqn, qp->qpn,
+			mthca_cq_clean(dev, to_mcq(qp->ibqp.recv_cq), qp->qpn,
 				       qp->ibqp.srq ? to_msrq(qp->ibqp.srq) : NULL);
 
 		mthca_free_memfree(dev, qp);