IPoIB: Fix unregister_netdev hang

While using IPoIB over EHCA (rc6 bits), unregister_netdev hangs with the message: "waiting for ib2 to become free. Usage count = -515276", etc.

The problem is that the poll handler does netif_rx_complete (which does a dev_put) followed by netif_rx_reschedule() to schedule for more receives (which again does a dev_put). This reduces refcount to < 0 (depending on how many times netif_rx_complete followed by netif_rx_reschedule was called).

The following patch fixes the bug, but I don't know if there is some specific IB issue that prevents this approach.

---
 ipoib_ib.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff -ruNp org/drivers/infiniband/ulp/ipoib/ipoib_ib.c new1/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -291,7 +291,6 @@ int ipoib_poll(struct napi_struct *napi,
 
 	done  = 0;
 
-poll_more:
 	while (done < budget) {
 		int max = (budget - done);
 

@@ -316,12 +315,10 @@ poll_more:
 	}
 
 	if (done < budget) {
-		netif_rx_complete(dev, napi);
-		if (unlikely(ib_req_notify_cq(priv->cq,
-					      IB_CQ_NEXT_COMP |
-					      IB_CQ_REPORT_MISSED_EVENTS)) &&
-		    netif_rx_reschedule(napi))
-			goto poll_more;
+		if (likely(!ib_req_notify_cq(priv->cq,
+					     IB_CQ_NEXT_COMP |
+					     IB_CQ_REPORT_MISSED_EVENTS)))
+			netif_rx_complete(dev, napi);
 	}
 
 	return done;

Follow-up cleanup and "while loop" optimization in the poll handler. net_rx_action guarantees that 'budget' is atleast 1.

Note: This could also be done for poll handlers of other drivers.

---
 ipoib_ib.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff -ruNp new1/drivers/infiniband/ulp/ipoib/ipoib_ib.c new2/drivers/infiniband/ulp/ipoib/ipoib_ib.c
@@ -285,19 +285,16 @@ int ipoib_poll(struct napi_struct *napi,
 {
 	struct ipoib_dev_priv *priv = container_of(napi, struct ipoib_dev_priv, napi);
 	struct net_device *dev = priv->dev;
-	int done;
-	int t;
-	int n, i;
+	int num_wc, max_wc;
+	int done = 0;
 
-	done  = 0;
-
-	while (done < budget) {
-		int max = (budget - done);
+	do {
+		int i;
 
-		t = min(IPOIB_NUM_WC, max);
-		n = ib_poll_cq(priv->cq, t, priv->ibwc);
+		max_wc = min(IPOIB_NUM_WC, budget - done);
+		num_wc = ib_poll_cq(priv->cq, max_wc, priv->ibwc);
 
-		for (i = 0; i < n; i++) {
+		for (i = 0; i < num_wc; i++) {
 			struct ib_wc *wc = priv->ibwc + i;
 
 			if (wc->wr_id & IPOIB_CM_OP_SRQ) {

@@ -309,10 +306,7 @@ int ipoib_poll(struct napi_struct *napi,
 			} else
 				ipoib_ib_handle_tx_wc(dev, wc);
 		}
-
-		if (n != t)
-			break;
-	}
+	} while (num_wc == max_wc && done < budget);
 
 	if (done < budget) {
 		if (likely(!ib_req_notify_cq(priv->cq,

Thanks for testing on ehca...

I don't think you're actually using rc6 bits, since in your patch you have:

and I think that is only in Dave's net-2.6.24 tree now, right?

Dave, the real problem seems to be that netif_rx_recschedule() calls __napi_schedule() rather than __netif_rx_schedule(), so it misses the call to dev_hold() that is needed to balance the dev_put() in netif_rx_complete(). The current netif_rx_reschedule() looks like it really should be napi_reschedule(), and we need a new function that takes a netdev too. Or am I misunderstanding the refcounting?

I'll send a patch once I've had some breakfast and had a chance to at least compile it...

Krishna, unfortunately your proposed fix has a race:

Hi Roland,

Roland Dreier <[email protected]> wrote on 09/18/2007 07:57:24 PM:

have:

Nope, that was what I downloaded yesterday:

VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 23
EXTRAVERSION =-rc6
NAME = Pink Farting Weasel

IB_CQ_NEXT_COMP |

IB_CQ_REPORT_MISSED_EVENTS)))

To be clear, netif_rx_schedule while we are still in the poll list will not do any harm as it does nothing since NAPI_STATE_SCHED is still set (cleared by netif_rx_complete which has not yet run). Effectively we lost/delayed processing an interrupt, if I understood the code right.

I agree with you on the new patch.

thanks,

- KK

Please double check your tree. I just very carefully looked at my trees, and the poll_more: label is added in commit 6b460a71 ("[NET]:
Make NAPI polling independent of struct net_device objects.") which is only in the net-2.6.24 tree. Of course Dave did not change the version information in the Makefile since he wouldn't want Linus to pick up any extra strange changes when he pulls, so a net-2.6.24 tree will look like 2.6.23-rc6 as you quoted.

And the refcounting bug I fixed is only in net-2.6.24.

Right, we lose an interrupt, and since the CQ events are one-shot, we never get another one, and the interface is effectively dead.

- R.

Hi Roland,

You are absolutely right. My wording was incorrect, I should have said net-2.6.24 (which is *at* rev rc6).

will not

(cleared

lost/delayed

Thanks,

- KK

netif_rx_complete() takes a netdev parameter and does dev_put() on that netdev, so netif_rx_reschedule() needs to also take a netdev parameter and do dev_hold() on it to avoid reference counts from getting becoming negative because of unbalanced dev_put()s.

This should fix the problem reported by Krishna Kumar <[email protected]> with IPoIB waiting forever for netdev refcounts to become 0 during module unload.

---
Dave, feel free to roll this up into earlier NAPI conversion patches (assuming I'm understanding things correctly and this patch actually makes sense!).

BTW, it looks like drivers/net/ibm_emac/ibm_emac_mal.c would not have built in the current net-2.6.24 tree, since its call to netif_rx_reschedule() was left with the netdev parameter. So that file does not need to be touched in this patch.

 drivers/infiniband/ulp/ipoib/ipoib_ib.c |    2 +-
 drivers/net/arm/ep93xx_eth.c            |    2 +-
 drivers/net/ehea/ehea_main.c            |    2 +-
 drivers/net/ibmveth.c                   |    2 +-
 include/linux/netdevice.h               |   21 +++++++++++----------
 5 files changed, 15 insertions(+), 14 deletions(-)

index 6a2bff4..481e4b6 100644
@@ -320,7 +320,7 @@ poll_more:
 		if (unlikely(ib_req_notify_cq(priv->cq,
 					      IB_CQ_NEXT_COMP |
 					      IB_CQ_REPORT_MISSED_EVENTS)) &&
-		    netif_rx_reschedule(napi))
+		    netif_rx_reschedule(dev, napi))
 			goto poll_more;
 	}
 

index f3858d1..7f016f3 100644
@@ -309,7 +309,7 @@ poll_some_more:
 		}
 		spin_unlock_irq(&ep->rx_lock);
 
-		if (more && netif_rx_reschedule(napi))
+		if (more && netif_rx_reschedule(dev, napi))
 			goto poll_some_more;
 	}
 

index 4a5ab4a..9a499f4 100644
@@ -636,7 +636,7 @@ static int ehea_poll(struct napi_struct *napi, int budget)
 		if (!cqe && !cqe_skb)
 			return 0;
 
-		if (!netif_rx_reschedule(napi))
+		if (!netif_rx_reschedule(dev, napi))
 			return 0;
 	}
 

index b8d7cec..b94f266 100644
@@ -973,7 +973,7 @@ static int ibmveth_poll(struct napi_struct *napi, int budget)
 		netif_rx_complete(netdev, napi);
 
 		if (ibmveth_rxq_pending_buffer(adapter) &&
-		    netif_rx_reschedule(napi)) {
+		    netif_rx_reschedule(netdev, napi)) {
 			lpar_rc = h_vio_signal(adapter->vdev->unit_address,
 					       VIO_IRQ_DISABLE);
 			goto restart_poll;

index be5fe05..0dbf185 100644
@@ -1198,16 +1198,6 @@ static inline u32 netif_msg_init(int debug_value, int default_msg_enable_bits)
 	return (1 << debug_value) - 1;
 }
 
-/* Try to reschedule poll. Called by dev->poll() after netif_rx_complete().  */
-static inline int netif_rx_reschedule(struct napi_struct *n)
-{
-	if (napi_schedule_prep(n)) {
-		__napi_schedule(n);
-		return 1;
-	}
-	return 0;
-}
-
 /* Test if receive needs to be scheduled but only if up */
 static inline int netif_rx_schedule_prep(struct net_device *dev,
 					 struct napi_struct *napi)

@@ -1234,6 +1224,17 @@ static inline void netif_rx_schedule(struct net_device *dev,
 		__netif_rx_schedule(dev, napi);
 }
 
+/* Try to reschedule poll. Called by dev->poll() after netif_rx_complete().  */
+static inline int netif_rx_reschedule(struct net_device *dev,
+				      struct napi_struct *napi)
+{
+	if (napi_schedule_prep(napi)) {
+		__netif_rx_schedule(dev, napi);
+		return 1;
+	}
+	return 0;
+}
+
 /* same as netif_rx_complete, except that local_irq_save(flags)
  * has already been issued
  */

It looks like the comments for dev_put() and dev_hold() got reversed somehow.

---
 include/linux/netdevice.h |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index be5fe05..239ae68 100644
@@ -1030,7 +1030,7 @@ extern int		netdev_budget;
 extern void netdev_run_todo(void);
 
 /**
- *	dev_put - get reference to device
+ *	dev_put - release reference to device
  *	@dev: network device
  *
  * Hold reference to device to keep it from being freed.

@@ -1041,7 +1041,7 @@ static inline void dev_put(struct net_device *dev)
 }
 
 /**
- *	dev_hold - release reference to device
+ *	dev_hold - get reference to device
  *	@dev: network device
  *
  * Release reference to device to allow it to be freed.

From: Roland Dreier <[email protected]>
Date: Tue, 18 Sep 2007 11:04:22 -0700

Applied, thanks Roland.

From: Roland Dreier <[email protected]>
Date: Tue, 18 Sep 2007 10:58:37 -0700

Applied to net-2.6.24, thanks Roland.

Yes, I know, this is the one NAPI driver that hasn't been converted.

It's a complicated conversion because of how the driver and the data structures have been arranged (in short, a mess) which makes it insanely difficult to get from a queue instance back up to a network device or similar.

Further complicating things is that you need to setup a ppc32 cross-build environment to even build test a conversion, and I'm not comfortable doing the surgery until I can test build the thing.

And this may be hard to believe, but other things have been more pressing than setting up a ppc32 cross-build environment :-)

This is a hint of anyone looking for something to do that it'd be much appreciated for someone to tackle the ibm_emac conversion.

Thanks.

OK, I actually have a system with a ppc 440 SoC that uses this driver, so I'll try to get things to the stage where I can boot net-2.6.24 on it and see if I can get the driver working...

From: Roland Dreier <[email protected]>
Date: Tue, 18 Sep 2007 15:46:36 -0700

Thanks a lot Roland.