Patchwork ucc_geth: Add support for skb recycling

login
register
mail settings
Submitter Anton Vorontsov
Date July 7, 2009, 6:38 p.m.
Message ID <20090707183842.GA8425@oksana.dev.rtsoft.ru>
Download mbox | patch
Permalink /patch/29538/
State Not Applicable
Headers show

Comments

Anton Vorontsov - July 7, 2009, 6:38 p.m.
We can reclaim transmitted skbs to use in the receive path, so-called
skb recycling support.

Also reorder ucc_geth_poll() steps, so that we'll clean tx ring firstly,
thus maybe reclaim some skbs for rx.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 drivers/net/ucc_geth.c |   40 ++++++++++++++++++++++++++++------------
 drivers/net/ucc_geth.h |    2 ++
 2 files changed, 30 insertions(+), 12 deletions(-)
Rick Jones - July 7, 2009, 6:47 p.m.
Anton Vorontsov wrote:
> We can reclaim transmitted skbs to use in the receive path, so-called
> skb recycling support.
> 
> Also reorder ucc_geth_poll() steps, so that we'll clean tx ring firstly,
> thus maybe reclaim some skbs for rx.

Admittedly, all the world is not TCP, but a big chunk is, so are you likely to 
have reference counts go to zero on the tx queue for anything other than small 
standalone TCP ACK segments?

rick jones
Anton Vorontsov - July 7, 2009, 8:37 p.m.
On Tue, Jul 07, 2009 at 11:47:38AM -0700, Rick Jones wrote:
> Anton Vorontsov wrote:
> >We can reclaim transmitted skbs to use in the receive path, so-called
> >skb recycling support.
> >
> >Also reorder ucc_geth_poll() steps, so that we'll clean tx ring firstly,
> >thus maybe reclaim some skbs for rx.
> 
> Admittedly, all the world is not TCP, but a big chunk is, so are you
> likely to have reference counts go to zero on the tx queue for
> anything other than small standalone TCP ACK segments?

That's a generic question wrt skb recycling, right? Whether we can
always recycle transmitted skbs. No, sometimes (or mostly) we can't.

Initially, I was quite puzzled by this support... looking at how
gianfar driver works (it has the same support as of 0fd56bb5be6455d0),
I noticed that skb_recycle_check() always returns 0, and so we
don't recycle the skbs.

Though, things change when the kernel starts packets forwarding,
*then* skb recycling path actually triggers.

Lennert (skb recycling author) hints us that the gain is indeed
in forwarding/routing workload:

http://kerneltrap.org/mailarchive/linux-netdev/2008/9/28/3433514

Hope I understood everything correctly. :-)


Thanks!
Rick Jones - July 7, 2009, 8:46 p.m.
Anton Vorontsov wrote:
> On Tue, Jul 07, 2009 at 11:47:38AM -0700, Rick Jones wrote:
>>Admittedly, all the world is not TCP, but a big chunk is, so are you
>>likely to have reference counts go to zero on the tx queue for
>>anything other than small standalone TCP ACK segments?
> 
> 
> That's a generic question wrt skb recycling, right? Whether we can
> always recycle transmitted skbs. No, sometimes (or mostly) we can't.
> 
> Initially, I was quite puzzled by this support... looking at how
> gianfar driver works (it has the same support as of 0fd56bb5be6455d0),
> I noticed that skb_recycle_check() always returns 0, and so we
> don't recycle the skbs.
> 
> Though, things change when the kernel starts packets forwarding,
> *then* skb recycling path actually triggers.
> 
> Lennert (skb recycling author) hints us that the gain is indeed
> in forwarding/routing workload:
> 
> http://kerneltrap.org/mailarchive/linux-netdev/2008/9/28/3433514
 >
> 
> Hope I understood everything correctly. :-)

Given the text reads:

  This gives a nice increase in the maximum loss-free packet forwarding
  rate in routing workloads.

Your understanding is probably correct.  Might have been "nice" :) to get a 
definition of a "nice increase" though :)

rick jones
David Miller - July 8, 2009, 2:23 a.m.
From: Anton Vorontsov <avorontsov@ru.mvista.com>
Date: Tue, 7 Jul 2009 22:38:42 +0400

> We can reclaim transmitted skbs to use in the receive path, so-called
> skb recycling support.
> 
> Also reorder ucc_geth_poll() steps, so that we'll clean tx ring firstly,
> thus maybe reclaim some skbs for rx.
> 
> Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

Applied to net-next-2.6

Patch

diff --git a/drivers/net/ucc_geth.c b/drivers/net/ucc_geth.c
index 40c6eba..beaa329 100644
--- a/drivers/net/ucc_geth.c
+++ b/drivers/net/ucc_geth.c
@@ -209,9 +209,10 @@  static struct sk_buff *get_new_skb(struct ucc_geth_private *ugeth,
 {
 	struct sk_buff *skb = NULL;
 
-	skb = dev_alloc_skb(ugeth->ug_info->uf_info.max_rx_buf_length +
-				  UCC_GETH_RX_DATA_BUF_ALIGNMENT);
-
+	skb = __skb_dequeue(&ugeth->rx_recycle);
+	if (!skb)
+		skb = dev_alloc_skb(ugeth->ug_info->uf_info.max_rx_buf_length +
+				    UCC_GETH_RX_DATA_BUF_ALIGNMENT);
 	if (skb == NULL)
 		return NULL;
 
@@ -1986,6 +1987,8 @@  static void ucc_geth_memclean(struct ucc_geth_private *ugeth)
 		iounmap(ugeth->ug_regs);
 		ugeth->ug_regs = NULL;
 	}
+
+	skb_queue_purge(&ugeth->rx_recycle);
 }
 
 static void ucc_geth_set_multi(struct net_device *dev)
@@ -2202,6 +2205,8 @@  static int ucc_struct_init(struct ucc_geth_private *ugeth)
 		return -ENOMEM;
 	}
 
+	skb_queue_head_init(&ugeth->rx_recycle);
+
 	return 0;
 }
 
@@ -3208,8 +3213,10 @@  static int ucc_geth_rx(struct ucc_geth_private *ugeth, u8 rxQ, int rx_work_limit
 			if (netif_msg_rx_err(ugeth))
 				ugeth_err("%s, %d: ERROR!!! skb - 0x%08x",
 					   __func__, __LINE__, (u32) skb);
-			if (skb)
-				dev_kfree_skb_any(skb);
+			if (skb) {
+				skb->data = skb->head + NET_SKB_PAD;
+				__skb_queue_head(&ugeth->rx_recycle, skb);
+			}
 
 			ugeth->rx_skbuff[rxQ][ugeth->skb_currx[rxQ]] = NULL;
 			dev->stats.rx_dropped++;
@@ -3267,6 +3274,8 @@  static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 
 	/* Normal processing. */
 	while ((bd_status & T_R) == 0) {
+		struct sk_buff *skb;
+
 		/* BD contains already transmitted buffer.   */
 		/* Handle the transmitted buffer and release */
 		/* the BD to be used with the current frame  */
@@ -3276,9 +3285,16 @@  static int ucc_geth_tx(struct net_device *dev, u8 txQ)
 
 		dev->stats.tx_packets++;
 
-		/* Free the sk buffer associated with this TxBD */
-		dev_kfree_skb(ugeth->
-				  tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]]);
+		skb = ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]];
+
+		if (skb_queue_len(&ugeth->rx_recycle) < RX_BD_RING_LEN &&
+			     skb_recycle_check(skb,
+				    ugeth->ug_info->uf_info.max_rx_buf_length +
+				    UCC_GETH_RX_DATA_BUF_ALIGNMENT))
+			__skb_queue_head(&ugeth->rx_recycle, skb);
+		else
+			dev_kfree_skb(skb);
+
 		ugeth->tx_skbuff[txQ][ugeth->skb_dirtytx[txQ]] = NULL;
 		ugeth->skb_dirtytx[txQ] =
 		    (ugeth->skb_dirtytx[txQ] +
@@ -3307,16 +3323,16 @@  static int ucc_geth_poll(struct napi_struct *napi, int budget)
 
 	ug_info = ugeth->ug_info;
 
-	howmany = 0;
-	for (i = 0; i < ug_info->numQueuesRx; i++)
-		howmany += ucc_geth_rx(ugeth, i, budget - howmany);
-
 	/* Tx event processing */
 	spin_lock(&ugeth->lock);
 	for (i = 0; i < ug_info->numQueuesTx; i++)
 		ucc_geth_tx(ugeth->ndev, i);
 	spin_unlock(&ugeth->lock);
 
+	howmany = 0;
+	for (i = 0; i < ug_info->numQueuesRx; i++)
+		howmany += ucc_geth_rx(ugeth, i, budget - howmany);
+
 	if (howmany < budget) {
 		napi_complete(napi);
 		setbits32(ugeth->uccf->p_uccm, UCCE_RX_EVENTS | UCCE_TX_EVENTS);
diff --git a/drivers/net/ucc_geth.h b/drivers/net/ucc_geth.h
index 195ab26..cfb31af 100644
--- a/drivers/net/ucc_geth.h
+++ b/drivers/net/ucc_geth.h
@@ -1212,6 +1212,8 @@  struct ucc_geth_private {
 	/* index of the first skb which hasn't been transmitted yet. */
 	u16 skb_dirtytx[NUM_TX_QUEUES];
 
+	struct sk_buff_head rx_recycle;
+
 	struct ugeth_mii_info *mii_info;
 	struct phy_device *phydev;
 	phy_interface_t phy_interface;