diff mbox

[RFC] gianfar: fix undo of reserve()

Message ID A6A1774AFD79E346AE6D49A33CB294530DC19FC4@EX-BE-017-SFO.shared.themessagecenter.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

ben@bigfootnetworks.com March 24, 2010, 3:05 p.m. UTC 
From: Ben Menchaca <ben@bigfootnetworks.com>

Fix undo of reserve() before RX recycle

gfar_new_skb reserve()s space in the SKB to align it.  If an error occurs,
and the skb needs to be returned to the RX recycle queue, the current code
attempts to reset head, but did not reset tail.  This patch remembers the
alignment amount, and reverses the reserve() when needed.

Signed-off-by: Ben Menchaca <ben@bigfootnetworks.com>
---
 drivers/net/gianfar.c |    5 +++--
 drivers/net/gianfar.h |    6 ++++++
 2 files changed, 9 insertions(+), 2 deletions(-)

--

--
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

Comments

David Miller March 24, 2010, 6:47 p.m. UTC | #1 
From: "Ben Menchaca (ben@bigfootnetworks.com)" <ben@bigfootnetworks.com>
Date: Wed, 24 Mar 2010 08:05:02 -0700

> From: Ben Menchaca <ben@bigfootnetworks.com>
> 
> Fix undo of reserve() before RX recycle
> 
> gfar_new_skb reserve()s space in the SKB to align it.  If an error occurs,
> and the skb needs to be returned to the RX recycle queue, the current code
> attempts to reset head, but did not reset tail.  This patch remembers the
> alignment amount, and reverses the reserve() when needed.
> 
> Signed-off-by: Ben Menchaca <ben@bigfootnetworks.com>

So, is this tested at least a little bit?
--
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
ben@bigfootnetworks.com March 24, 2010, 8:13 p.m. UTC | #2 
> So, is this tested at least a little bit?
We run a 2.6.31 variant on our board, and this patch fixes the skb->len issue, and we are not observing any strangeness under the stress conditions after this patch, and our asserts are no longer firing.  2.6.31 was pre-multiq for gianfar, though, and that was a large change...I would definitely prefer getting some modern-ish kernel time on it.  If needed, I can probably move our board forward to -next for a bit.

For some history, and to see the original issue addressed, this appears to have been introduced shortly after recycling in 2.6.30, here:
commit d4a76f8a619b5d7dfd5a0f122666fee24bb3dcb9
gianfar: fix BUG under load after introduction of skb recycling

- Ben

--
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
David Miller March 24, 2010, 8:21 p.m. UTC | #3 
From: "Ben Menchaca (ben@bigfootnetworks.com)" <ben@bigfootnetworks.com>
Date: Wed, 24 Mar 2010 13:13:22 -0700

> I would definitely prefer getting some modern-ish kernel time on it.
> If needed, I can probably move our board forward to -next for a bit.

If anyone can test this on a current kernel that would be
enough for me.
--
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
David Miller March 27, 2010, 3:16 a.m. UTC | #4 
From: "Ben Menchaca (ben@bigfootnetworks.com)" <ben@bigfootnetworks.com>
Date: Wed, 24 Mar 2010 08:05:02 -0700

> From: Ben Menchaca <ben@bigfootnetworks.com>
> 
> Fix undo of reserve() before RX recycle
> 
> gfar_new_skb reserve()s space in the SKB to align it.  If an error occurs,
> and the skb needs to be returned to the RX recycle queue, the current code
> attempts to reset head, but did not reset tail.  This patch remembers the
> alignment amount, and reverses the reserve() when needed.
> 
> Signed-off-by: Ben Menchaca <ben@bigfootnetworks.com>

Applied, thanks.
--
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
diff mbox

Patch

diff --git a/drivers/net/gianfar.c b/drivers/net/gianfar.c
index b671555..669de02 100644
--- a/drivers/net/gianfar.c
+++ b/drivers/net/gianfar.c
@@ -2393,6 +2393,7 @@  struct sk_buff * gfar_new_skb(struct net_device *dev)
 	 * as many bytes as needed to align the data properly
 	 */
 	skb_reserve(skb, alignamount);
+	GFAR_CB(skb)->alignamount = alignamount;
 
 	return skb;
 }
@@ -2533,13 +2534,13 @@  int gfar_clean_rx_ring(struct gfar_priv_rx_q *rx_queue, int rx_work_limit)
 				newskb = skb;
 			else if (skb) {
 				/*
-				 * We need to reset ->data to what it
+				 * We need to un-reserve() the skb 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_reserve(skb, -GFAR_CB(skb)->alignamount);
 				__skb_queue_head(&priv->rx_recycle, skb);
 			}
 		} else {
diff --git a/drivers/net/gianfar.h b/drivers/net/gianfar.h
index 3d72dc4..06044dd 100644
--- a/drivers/net/gianfar.h
+++ b/drivers/net/gianfar.h
@@ -566,6 +566,12 @@  struct rxfcb {
 	u16	vlctl;	/* VLAN control word */
 };
 
+struct gianfar_skb_cb {
+	int alignamount;
+};
+
+#define GFAR_CB(skb) ((struct gianfar_skb_cb *)((skb)->cb))
+
 struct rmon_mib
 {
 	u32	tr64;	/* 0x.680 - Transmit and Receive 64-byte Frame Counter */