Patchwork Gianfar: RX Recycle skb->len error

login
register
mail settings
Submitter ben@bigfootnetworks.com
Date March 22, 2010, 9:10 p.m.
Message ID <A6A1774AFD79E346AE6D49A33CB294530DC19F18@EX-BE-017-SFO.shared.themessagecenter.com>
Download mbox | patch
Permalink /patch/48307/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

ben@bigfootnetworks.com - March 22, 2010, 9:10 p.m.
> Yes, skb_unreserve() (or skb_reset_reserved() for naming consistency?)

> would be great.


Just a couple of notes:
	It's yucky, but skb_reserve(skb, -alignamount) works, if you know the alignamount (which I discuss a bit below).  If that's not intended, then should len be unsigned int?  skb_put, skb_push, and skb_pull all seem to be unsigned int.

	It seems in both these cases for gianfar, the amount of the alignment is not immediately available to the code the recognizes that the skb_reset_reserved() is required.  A bit larger rework appears to be needed.  

I took a shot at using a new heuristic for the rx_ring to prevent having to skb_reset_reserved() at all.  The idea is to guarantee that the elements in ring are reserve()-ed, and that if we encounter an error condition that yields the two-skb case, free the new skb, since it has not yet been reserve()-ed.  Looking forward to hearing from FS on the "right" way, though.

Ben Menchaca
Bigfoot Networks
David Miller - March 23, 2010, 3:30 a.m.
From: "Ben Menchaca (ben@bigfootnetworks.com)" <ben@bigfootnetworks.com>
Date: Mon, 22 Mar 2010 14:10:48 -0700

> 	It's yucky, but skb_reserve(skb, -alignamount) works,

I have no problem with people using that.

> 	It seems in both these cases for gianfar, the amount of the
> 	alignment is not immediately available to the code the
> 	recognizes that the skb_reset_reserved() is required.  A bit
> 	larger rework appears to be needed.

There's no need to make this so complicated.  Just remember the
value and then refer to it later, when needed.

struct gianfar_skb_cb {
       int	alignamount;
};

#define GIANFAR_CB(skb) ((struct gianfar_skb_cb *)((skb)->cb))

...
	GIANFAR_CB(skb)->alignamount = alignamount;
...

		skb_reserve(skb, -GIANFAR_CB(skb)->alignamount);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c

index b671555..82f8486 100644

--- a/drivers/net/gianfar.c

+++ b/drivers/net/gianfar.c

@@ -109,6 +109,7 @@  static int gfar_start_xmit(struct sk_buff *skb, struct net_device *dev);

 static void gfar_reset_task(struct work_struct *work);
 static void gfar_timeout(struct net_device *dev);
 static int gfar_close(struct net_device *dev);
+static void gfar_skb_reserve_aligned(struct sk_buff *skb);

 struct sk_buff *gfar_new_skb(struct net_device *dev);
 static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp,
 		struct sk_buff *skb);
@@ -214,6 +215,7 @@  static int gfar_init_bds(struct net_device *ndev)

 							ndev->name);
 					goto err_rxalloc_fail;
 				}
+				gfar_skb_reserve_aligned(ndev);

 				rx_queue->rx_skbuff[j] = skb;
 
 				gfar_new_rxbdp(rx_queue, rxbdp, skb);
@@ -2372,6 +2374,20 @@  static void gfar_new_rxbdp(struct gfar_priv_rx_q *rx_queue, struct rxbd8 *bdp,

 }
 
 
+static void gfar_skb_reserve_aligned(struct sk_buff *skb)

+{

+	unsigned int alignamount;

+

+        alignamount = RXBUF_ALIGNMENT -

+                (((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1));

+

+        /* We need the data buffer to be aligned properly.  We will reserve

+         * as many bytes as needed to align the data properly

+         */

+        skb_reserve(skb, alignamount);

+}

+

+

 struct sk_buff * gfar_new_skb(struct net_device *dev)
 {
 	unsigned int alignamount;
@@ -2383,17 +2399,6 @@  struct sk_buff * gfar_new_skb(struct net_device *dev)

 		skb = netdev_alloc_skb(dev,
 				priv->rx_buffer_size + RXBUF_ALIGNMENT);
 
-	if (!skb)

-		return NULL;

-

-	alignamount = RXBUF_ALIGNMENT -

-		(((unsigned long) skb->data) & (RXBUF_ALIGNMENT - 1));

-

-	/* We need the data buffer to be aligned properly.  We will reserve

-	 * as many bytes as needed to align the data properly

-	 */

-	skb_reserve(skb, alignamount);

-

 	return skb;
 }
 
@@ -2532,21 +2537,17 @@  int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)

 			if (unlikely(!newskb))
 				newskb = skb;
 			else if (skb) {
-				/*

-				 * We need to reset ->data to what it

-				 * was before gfar_new_skb() re-aligned

-				 * it to an RXBUF_ALIGNMENT boundary

-				 * before we put the skb back on the

-				 * recycle list.

-				 */

-				skb->data = skb->head + NET_SKB_PAD;

-				__skb_queue_head(&priv->rx_recycle, skb);

+				__skb_queue_head(&priv->rx_recycle, newskb);

+				newskb = skb;

+			} else {

+				gfar_skb_reserve_aligned(newskb);

 			}
 		} else {
 			/* Increment the number of packets */
 			rx_queue->stats.rx_packets++;
 			howmany++;
 
+			gfar_skb_reserve_aligned(newskb);

 			if (likely(skb)) {
 				pkt_len = bdp->length - ETH_FCS_LEN;
 				/* Remove the FCS from the packet length */