gianfar: Revive SKB recycling

Before calling gfar_clean_tx_ring() the driver grabs an irqsave spinlock, and then tries to recycle skbs. But since skb_recycle_check() returns 0 with IRQs disabled, we'll never recycle any skbs. It appears that gfar_clean_tx_ring() and gfar_start_xmit() are mostly idependent and can work in parallel, except when they modify num_txbdfree. So we can drop the lock from most sections and thus fix the skb recycling. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Acked-by: Kumar Gala <galak@kernel.crashing.org> Signed-off-by: David S. Miller <davem@davemloft.net>

gianfar: Revive SKB recycling
Before calling gfar_clean_tx_ring() the driver grabs an irqsave spinlock, and then tries to recycle skbs. But since skb_recycle_check() returns 0 with IRQs disabled, we'll never recycle any skbs. It appears that gfar_clean_tx_ring() and gfar_start_xmit() are mostly idependent and can work in parallel, except when they modify num_txbdfree. So we can drop the lock from most sections and thus fix the skb recycling. Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com> Acked-by: Kumar Gala <galak@kernel.crashing.org> Signed-off-by: David S. Miller <davem@davemloft.net>
Anton Vorontsov · David S. Miller
1 parent 836cf7faf8
Showing 1 changed file with 19 additions and 12 deletions Side-by-side Diff
drivers/net/gianfar.c
@@ -1928,14 +1928,11 @@
 	/* total number of fragments in the SKB */
 	nr_frags = skb_shinfo(skb)->nr_frags;
  
-	spin_lock_irqsave(&tx_queue->txlock, flags);
-
 	/* check if there is space to queue this packet */
 	if ((nr_frags+1) > tx_queue->num_txbdfree) {
 		/* no space, stop the queue */
 		netif_tx_stop_queue(txq);
 		dev->stats.tx_fifo_errors++;
-		spin_unlock_irqrestore(&tx_queue->txlock, flags);
 		return NETDEV_TX_BUSY;
 	}
  
@@ -1999,6 +1996,20 @@
 	lstatus |= BD_LFLAG(TXBD_CRC | TXBD_READY) | skb_headlen(skb);
  
 	/*
+	 * We can work in parallel with gfar_clean_tx_ring(), except
+	 * when modifying num_txbdfree. Note that we didn't grab the lock
+	 * when we were reading the num_txbdfree and checking for available
+	 * space, that's because outside of this function it can only grow,
+	 * and once we've got needed space, it cannot suddenly disappear.
+	 *
+	 * The lock also protects us from gfar_error(), which can modify
+	 * regs->tstat and thus retrigger the transfers, which is why we
+	 * also must grab the lock before setting ready bit for the first
+	 * to be transmitted BD.
+	 */
+	spin_lock_irqsave(&tx_queue->txlock, flags);
+
+	/*
 	 * The powerpc-specific eieio() is used, as wmb() has too strong
 	 * semantics (it requires synchronization between cacheable and
 	 * uncacheable mappings, which eieio doesn't provide and which we
@@ -2225,6 +2236,8 @@
 	skb_dirtytx = tx_queue->skb_dirtytx;
  
 	while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) {
+		unsigned long flags;
+
 		frags = skb_shinfo(skb)->nr_frags;
 		lbdp = skip_txbd(bdp, frags, base, tx_ring_size);
  
  
@@ -2269,7 +2282,9 @@
 			TX_RING_MOD_MASK(tx_ring_size);
  
 		howmany++;
+		spin_lock_irqsave(&tx_queue->txlock, flags);
 		tx_queue->num_txbdfree += frags + 1;
+		spin_unlock_irqrestore(&tx_queue->txlock, flags);
 	}
  
 	/* If we freed a buffer, we can restart transmission, if necessary */
@@ -2548,7 +2563,6 @@
 	int tx_cleaned = 0, i, left_over_budget = budget;
 	unsigned long serviced_queues = 0;
 	int num_queues = 0;
-	unsigned long flags;
  
 	num_queues = gfargrp->num_rx_queues;
 	budget_per_queue = budget/num_queues;
@@ -2568,14 +2582,7 @@
 			rx_queue = priv->rx_queue[i];
 			tx_queue = priv->tx_queue[rx_queue->qindex];
  
-			/* If we fail to get the lock,
-			 * don't bother with the TX BDs */
-			if (spin_trylock_irqsave(&tx_queue->txlock, flags)) {
-				tx_cleaned += gfar_clean_tx_ring(tx_queue);
-				spin_unlock_irqrestore(&tx_queue->txlock,
-							flags);
-			}
-
+			tx_cleaned += gfar_clean_tx_ring(tx_queue);
 			rx_cleaned_per_queue = gfar_clean_rx_ring(rx_queue,
 							budget_per_queue);
 			rx_cleaned += rx_cleaned_per_queue;
...	...	@@ -1928,14 +1928,11 @@
1928	1928	/* total number of fragments in the SKB */
1929	1929	nr_frags = skb_shinfo(skb)->nr_frags;
1930	1930
1931		- spin_lock_irqsave(&tx_queue->txlock, flags);
1932		-
1933	1931	/* check if there is space to queue this packet */
1934	1932	if ((nr_frags+1) > tx_queue->num_txbdfree) {
1935	1933	/* no space, stop the queue */
1936	1934	netif_tx_stop_queue(txq);
1937	1935	dev->stats.tx_fifo_errors++;
1938		- spin_unlock_irqrestore(&tx_queue->txlock, flags);
1939	1936	return NETDEV_TX_BUSY;
1940	1937	}
1941	1938
...	...	@@ -1999,6 +1996,20 @@
1999	1996	lstatus \|= BD_LFLAG(TXBD_CRC \| TXBD_READY) \| skb_headlen(skb);
2000	1997
2001	1998	/*
	1999	+ * We can work in parallel with gfar_clean_tx_ring(), except
	2000	+ * when modifying num_txbdfree. Note that we didn't grab the lock
	2001	+ * when we were reading the num_txbdfree and checking for available
	2002	+ * space, that's because outside of this function it can only grow,
	2003	+ * and once we've got needed space, it cannot suddenly disappear.
	2004	+ *
	2005	+ * The lock also protects us from gfar_error(), which can modify
	2006	+ * regs->tstat and thus retrigger the transfers, which is why we
	2007	+ * also must grab the lock before setting ready bit for the first
	2008	+ * to be transmitted BD.
	2009	+ */
	2010	+ spin_lock_irqsave(&tx_queue->txlock, flags);
	2011	+
	2012	+ /*
2002	2013	* The powerpc-specific eieio() is used, as wmb() has too strong
2003	2014	* semantics (it requires synchronization between cacheable and
2004	2015	* uncacheable mappings, which eieio doesn't provide and which we
...	...	@@ -2225,6 +2236,8 @@
2225	2236	skb_dirtytx = tx_queue->skb_dirtytx;
2226	2237
2227	2238	while ((skb = tx_queue->tx_skbuff[skb_dirtytx])) {
	2239	+ unsigned long flags;
	2240	+
2228	2241	frags = skb_shinfo(skb)->nr_frags;
2229	2242	lbdp = skip_txbd(bdp, frags, base, tx_ring_size);
2230	2243
2231	2244
...	...	@@ -2269,7 +2282,9 @@
2269	2282	TX_RING_MOD_MASK(tx_ring_size);
2270	2283
2271	2284	howmany++;
	2285	+ spin_lock_irqsave(&tx_queue->txlock, flags);
2272	2286	tx_queue->num_txbdfree += frags + 1;
	2287	+ spin_unlock_irqrestore(&tx_queue->txlock, flags);
2273	2288	}
2274	2289
2275	2290	/* If we freed a buffer, we can restart transmission, if necessary */
...	...	@@ -2548,7 +2563,6 @@
2548	2563	int tx_cleaned = 0, i, left_over_budget = budget;
2549	2564	unsigned long serviced_queues = 0;
2550	2565	int num_queues = 0;
2551		- unsigned long flags;
2552	2566
2553	2567	num_queues = gfargrp->num_rx_queues;
2554	2568	budget_per_queue = budget/num_queues;
...	...	@@ -2568,14 +2582,7 @@
2568	2582	rx_queue = priv->rx_queue[i];
2569	2583	tx_queue = priv->tx_queue[rx_queue->qindex];
2570	2584
2571		- /* If we fail to get the lock,
2572		- * don't bother with the TX BDs */
2573		- if (spin_trylock_irqsave(&tx_queue->txlock, flags)) {
2574		- tx_cleaned += gfar_clean_tx_ring(tx_queue);
2575		- spin_unlock_irqrestore(&tx_queue->txlock,
2576		- flags);
2577		- }
2578		-
	2585	+ tx_cleaned += gfar_clean_tx_ring(tx_queue);
2579	2586	rx_cleaned_per_queue = gfar_clean_rx_ring(rx_queue,
2580	2587	budget_per_queue);
2581	2588	rx_cleaned += rx_cleaned_per_queue;