>-----Original Message----- >From: Anton Vorontsov [mailto:avoront...@ru.mvista.com] >Sent: Wednesday, November 11, 2009 5:41 AM >To: David Miller >Cc: Fleming Andy-AFLEMING; Jon Loeliger; Kumar >Gopalpet-B05799; Lennert Buytenhek; Stephen Hemminger; >net...@vger.kernel.org; linuxppc-...@ozlabs.org >Subject: [PATCH 6/6] 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 <avoront...@ru.mvista.com> >--- > drivers/net/gianfar.c | 31 +++++++++++++++++++------------ > 1 files changed, 19 insertions(+), 12 deletions(-) > >diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c >index fde430a..16def13 100644 >--- a/drivers/net/gianfar.c >+++ b/drivers/net/gianfar.c >@@ -1928,14 +1928,11 @@ static int gfar_start_xmit(struct >sk_buff *skb, struct net_device *dev) > /* 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 @@ static int gfar_start_xmit(struct >sk_buff *skb, struct net_device *dev) > 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 @@ static int >gfar_clean_tx_ring(struct gfar_priv_tx_q *tx_queue) > 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 @@ static int gfar_clean_tx_ring(struct >gfar_priv_tx_q *tx_queue) > 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 @@ static int >gfar_poll(struct napi_struct *napi, int budget) > 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 @@ static int gfar_poll(struct napi_struct >*napi, int budget) > 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; >--
Anton, we tried some experiments too at our end, and removing the spinlocks did help improve the performance and recycling was effective although, I don't have exact numbers to specify. But overall I agree with you in removing the spinlocks from the gfar_poll context. -- Thanks Sandeep _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
