move skb->dev assignment into netdev_alloc_skb

PATCH In review Christoph Hellwig 5 participants 6 messages Aug 5, 2006 – Aug 15, 2006

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Christoph Hellwig

author

2006-08-05 13:01 UTC #0 lore raw

PATCH 1/1 move skb->dev assignment into netdev_alloc_skb

All caller of netdev_alloc_skb need to assign skb->dev shortly afterwards. Move it into common code.

I also had to fixup a little bit of the surrounding control flow in e1000 - it was just too convoluted.

Signed-off-by Christoph Hellwig <[email protected]>

drivers/net/e1000/e1000_main.c +6 ~~-11~~

Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================
@@ -3711,7 +3711,6 @@
 			    netdev_alloc_skb(netdev, length + NET_IP_ALIGN);
 			if (new_skb) {
 				skb_reserve(new_skb, NET_IP_ALIGN);
-				new_skb->dev = netdev;
 				memcpy(new_skb->data - NET_IP_ALIGN,
 				       skb->data - NET_IP_ALIGN,
 				       length + NET_IP_ALIGN);

@@ -3978,13 +3977,13 @@
 	buffer_info = &rx_ring->buffer_info[i];
 
 	while (cleaned_count--) {
-		if (!(skb = buffer_info->skb))
-			skb = netdev_alloc_skb(netdev, bufsz);
-		else {
+		skb = buffer_info->skb;
+		if (skb) {
 			skb_trim(skb, 0);
 			goto map_skb;
 		}
 
+		skb = netdev_alloc_skb(netdev, bufsz);
 		if (unlikely(!skb)) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;

@@ -4009,10 +4008,10 @@
 				dev_kfree_skb(skb);
 				dev_kfree_skb(oldskb);
 				break; /* while !buffer_info->skb */
-			} else {
-				/* Use new allocation */
-				dev_kfree_skb(oldskb);
 			}
+
+			/* Use new allocation */
+			dev_kfree_skb(oldskb);
 		}
 		/* Make buffer alignment 2 beyond a 16 byte boundary
 		 * this will result in a 16 byte aligned IP header after

@@ -4020,8 +4019,6 @@
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_buffer_len;
 map_skb:

@@ -4135,8 +4132,6 @@
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_ps_bsize0;
 		buffer_info->dma = pci_map_single(pdev, skb->data,

drivers/net/tg3.c +0 -2

Index: linux-2.6/drivers/net/tg3.c
===================================================================
@@ -3101,7 +3101,6 @@
 	if (skb == NULL)
 		return -ENOMEM;
 
-	skb->dev = tp->dev;
 	skb_reserve(skb, tp->rx_offset);
 
 	mapping = pci_map_single(tp->pdev, skb->data,

@@ -3274,7 +3273,6 @@
 			if (copy_skb == NULL)
 				goto drop_it_no_recycle;
 
-			copy_skb->dev = tp->dev;
 			skb_reserve(copy_skb, 2);
 			skb_put(copy_skb, len);
 			pci_dma_sync_single_for_cpu(tp->pdev, dma_addr, len, PCI_DMA_FROMDEVICE);

net/core/skbuff.c +3 -1

Index: linux-2.6/net/core/skbuff.c
===================================================================
@@ -268,8 +268,10 @@
 	struct sk_buff *skb;
 
 	skb = alloc_skb(length + NET_SKB_PAD, gfp_mask);
-	if (likely(skb))
+	if (likely(skb)) {
 		skb_reserve(skb, NET_SKB_PAD);
+		skb->dev = dev;
+	}
 	return skb;
 }

David Miller

maintainer

2006-08-07 23:10 UTC #1 lore raw

From: Christoph Hellwig <[email protected]>
Date: Sat, 5 Aug 2006 15:01:09 +0200

Quoted 7 lines

All caller of netdev_alloc_skb need to assign skb->dev shortly afterwards. Move it into common code.
I also had to fixup a little bit of the surrounding control flow in e1000 - it was just too convoluted.
Signed-off-by Christoph Hellwig <[email protected]>

Since the e1000 change is non-trivial I'm not going to bypass the driver author on it, sorry.

What I did do was put the netdev_alloc_skb() change into my tree, and since I'm co-author of the tg3 driver I'll apply that bit too.

The e1000 bit will need to go through the e1000 maintainers.

Thanks.

Jesse Brandeburg

2006-08-08 00:29 UTC #2 lore raw

Quoted 11 lines · David Miller

From: Christoph Hellwig <[email protected]>
Date: Sat, 5 Aug 2006 15:01:09 +0200
Quoted 7 lines
All caller of netdev_alloc_skb need to assign skb->dev shortly afterwards. Move it into common code.
I also had to fixup a little bit of the surrounding control flow in e1000 - it was just too convoluted.
Signed-off-by Christoph Hellwig <[email protected]>

Quoted 8 lines

Since the e1000 change is non-trivial I'm not going to bypass the driver author on it, sorry.
What I did do was put the netdev_alloc_skb() change into my tree, and since I'm co-author of the tg3 driver I'll apply that bit too.
The e1000 bit will need to go through the e1000 maintainers.

Thank you, I'll take a look at this tomorrow, as I need to digest the patch in context.

Jesse

Christoph Hellwig

author

2006-08-12 13:57 UTC #3 lore raw

Quoted 15 lines · Jesse Brandeburg

Quoted 14 lines · David Miller
From: Christoph Hellwig <[email protected]>
Date: Sat, 5 Aug 2006 15:01:09 +0200
Quoted 7 lines
All caller of netdev_alloc_skb need to assign skb->dev shortly afterwards. Move it into common code.
I also had to fixup a little bit of the surrounding control flow in e1000 - it was just too convoluted.
Signed-off-by Christoph Hellwig <[email protected]>
Since the e1000 change is non-trivial I'm not going to bypass the driver author on it, sorry.

Quoted 8 lines

Quoted 5 lines
What I did do was put the netdev_alloc_skb() change into my tree, and since I'm co-author of the tg3 driver I'll apply that bit too.
The e1000 bit will need to go through the e1000 maintainers.
Thank you, I'll take a look at this tomorrow, as I need to digest the patch in context.

Did you get a chance to look at it? For your conveniance here's just the e1000 bits, without the hunks Dave has commited already:

drivers/net/e1000/e1000_main.c +6 ~~-11~~

Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================
@@ -3711,7 +3711,6 @@
 			    netdev_alloc_skb(netdev, length + NET_IP_ALIGN);
 			if (new_skb) {
 				skb_reserve(new_skb, NET_IP_ALIGN);
-				new_skb->dev = netdev;
 				memcpy(new_skb->data - NET_IP_ALIGN,
 				       skb->data - NET_IP_ALIGN,
 				       length + NET_IP_ALIGN);

@@ -3978,13 +3977,13 @@
 	buffer_info = &rx_ring->buffer_info[i];
 
 	while (cleaned_count--) {
-		if (!(skb = buffer_info->skb))
-			skb = netdev_alloc_skb(netdev, bufsz);
-		else {
+		skb = buffer_info->skb;
+		if (skb) {
 			skb_trim(skb, 0);
 			goto map_skb;
 		}
 
+		skb = netdev_alloc_skb(netdev, bufsz);
 		if (unlikely(!skb)) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;

@@ -4009,10 +4008,10 @@
 				dev_kfree_skb(skb);
 				dev_kfree_skb(oldskb);
 				break; /* while !buffer_info->skb */
-			} else {
-				/* Use new allocation */
-				dev_kfree_skb(oldskb);
 			}
+
+			/* Use new allocation */
+			dev_kfree_skb(oldskb);
 		}
 		/* Make buffer alignment 2 beyond a 16 byte boundary
 		 * this will result in a 16 byte aligned IP header after

@@ -4020,8 +4019,6 @@
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_buffer_len;
 map_skb:

@@ -4135,8 +4132,6 @@
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_ps_bsize0;
 		buffer_info->dma = pci_map_single(pdev, skb->data,

Brandeburg, Jesse

Acked-by

2006-08-15 01:00 UTC #4 lore raw

Quoted 9 lines · Christoph Hellwig

Quoted 9 lines
Quoted 9 lines
Quoted 1 line
Signed-off-by Christoph Hellwig <[email protected]>
Since the e1000 change is non-trivial I'm not going to bypass the driver author on it, sorry.
What I did do was put the netdev_alloc_skb() change into my tree, and since I'm co-author of the tg3 driver I'll apply that bit too.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
The e1000 bit will need to go through the e1000 maintainers.
Thank you, I'll take a look at this tomorrow, as I need to digest the patch in context.
Did you get a chance to look at it? For your conveniance here's just the e1000 bits, without the hunks Dave has commited already:

I'm okay with this change. Auke will shortly be submitting a patch set for 2.6.19 and it should include this.

Thanks for the feedback Christoph!

Jesse

for reference this is the patch, but like i said auke will enqueue it in GIT and send it to Jeff for inclusion.

e1000: clean up skb allocation code, patch submitted by Christoph

Signed-off-by Christoph Hellwig <[email protected]>

Acked-by Jesse Brandeburg <[email protected]>

---

 drivers/net/e1000/e1000_main.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

drivers/net/e1000/e1000_main.c +7 ~~-12~~

index 627f224..66c0325 100644
@@ -36,7 +36,7 @@ static char e1000_driver_string[] = "Int
 #else
 #define DRIVERNAPI "-NAPI"
 #endif
-#define DRV_VERSION "7.1.9-k4"DRIVERNAPI
+#define DRV_VERSION "7.1.9-k6"DRIVERNAPI
 char e1000_driver_version[] = DRV_VERSION;
 static char e1000_copyright[] = "Copyright (c) 1999-2006 Intel Corporation.";

@@ -3711,7 +3711,6 @@ e1000_clean_rx_irq(struct e1000_adapter 
 			    netdev_alloc_skb(netdev, length + NET_IP_ALIGN);
 			if (new_skb) {
 				skb_reserve(new_skb, NET_IP_ALIGN);
-				new_skb->dev = netdev;
 				memcpy(new_skb->data - NET_IP_ALIGN,
 				       skb->data - NET_IP_ALIGN,
 				       length + NET_IP_ALIGN);

@@ -3978,13 +3977,13 @@ e1000_alloc_rx_buffers(struct e1000_adap
 	buffer_info = &rx_ring->buffer_info[i];
 
 	while (cleaned_count--) {
-		if (!(skb = buffer_info->skb))
-			skb = netdev_alloc_skb(netdev, bufsz);
-		else {
+		skb = buffer_info->skb;
+		if (skb) {
 			skb_trim(skb, 0);
 			goto map_skb;
 		}
 
+		skb = netdev_alloc_skb(netdev, bufsz);
 		if (unlikely(!skb)) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;

@@ -4009,10 +4008,10 @@ e1000_alloc_rx_buffers(struct e1000_adap
 				dev_kfree_skb(skb);
 				dev_kfree_skb(oldskb);
 				break; /* while !buffer_info->skb */
-			} else {
-				/* Use new allocation */
-				dev_kfree_skb(oldskb);
 			}
+
+			/* Use new allocation */
+			dev_kfree_skb(oldskb);
 		}
 		/* Make buffer alignment 2 beyond a 16 byte boundary
 		 * this will result in a 16 byte aligned IP header after

@@ -4020,8 +4019,6 @@ e1000_alloc_rx_buffers(struct e1000_adap
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_buffer_len;
 map_skb:

@@ -4135,8 +4132,6 @@ e1000_alloc_rx_buffers_ps(struct e1000_a
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_ps_bsize0;
 		buffer_info->dma = pci_map_single(pdev, skb->data,

Auke Kok

2006-08-15 14:39 UTC #5 lore raw

Quoted 9 lines · Jesse

Quoted 8 lines · Christoph Hellwig
Quoted 8 lines
Quoted 8 lines
Quoted 1 line
Signed-off-by Christoph Hellwig <[email protected]>
Since the e1000 change is non-trivial I'm not going to bypass the driver author on it, sorry.
What I did do was put the netdev_alloc_skb() change into my tree, and since I'm co-author of the tg3 driver I'll apply that bit too.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
The e1000 bit will need to go through the e1000 maintainers.
Thank you, I'll take a look at this tomorrow, as I need to digest the patch in context.
Did you get a chance to look at it? For your conveniance here's just the e1000 bits, without the hunks Dave has commited already:
I'm okay with this change. Auke will shortly be submitting a patch set for 2.6.19 and it should include this.

Great, I was already waiting for this change to drip in but hadn't checked for it. I've queued it and it will be included in this weeks' patch series git pull request.

Cheers,

Auke

Quoted 86 lines

Thanks for the feedback Christoph!

Jesse

for reference this is the patch, but like i said auke will enqueue it in GIT and send it to Jeff for inclusion.

e1000: clean up skb allocation code, patch submitted by Christoph

Signed-off-by Christoph Hellwig <[email protected]>

Acked-by Jesse Brandeburg <[email protected]>

---

 drivers/net/e1000/e1000_main.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

drivers/net/e1000/e1000_main.c +7 ~~-12~~

index 627f224..66c0325 100644
@@ -36,7 +36,7 @@ static char e1000_driver_string[] = "Int
 #else
 #define DRIVERNAPI "-NAPI"
 #endif
-#define DRV_VERSION "7.1.9-k4"DRIVERNAPI
+#define DRV_VERSION "7.1.9-k6"DRIVERNAPI
 char e1000_driver_version[] = DRV_VERSION;
 static char e1000_copyright[] = "Copyright (c) 1999-2006 Intel Corporation.";

@@ -3711,7 +3711,6 @@ e1000_clean_rx_irq(struct e1000_adapter 
 			    netdev_alloc_skb(netdev, length + NET_IP_ALIGN);
 			if (new_skb) {
 				skb_reserve(new_skb, NET_IP_ALIGN);
-				new_skb->dev = netdev;
 				memcpy(new_skb->data - NET_IP_ALIGN,
 				       skb->data - NET_IP_ALIGN,
 				       length + NET_IP_ALIGN);

@@ -3978,13 +3977,13 @@ e1000_alloc_rx_buffers(struct e1000_adap
 	buffer_info = &rx_ring->buffer_info[i];
 
 	while (cleaned_count--) {
-		if (!(skb = buffer_info->skb))
-			skb = netdev_alloc_skb(netdev, bufsz);
-		else {
+		skb = buffer_info->skb;
+		if (skb) {
 			skb_trim(skb, 0);
 			goto map_skb;
 		}
 
+		skb = netdev_alloc_skb(netdev, bufsz);
 		if (unlikely(!skb)) {
 			/* Better luck next round */
 			adapter->alloc_rx_buff_failed++;

@@ -4009,10 +4008,10 @@ e1000_alloc_rx_buffers(struct e1000_adap
 				dev_kfree_skb(skb);
 				dev_kfree_skb(oldskb);
 				break; /* while !buffer_info->skb */
-			} else {
-				/* Use new allocation */
-				dev_kfree_skb(oldskb);
 			}
+
+			/* Use new allocation */
+			dev_kfree_skb(oldskb);
 		}
 		/* Make buffer alignment 2 beyond a 16 byte boundary
 		 * this will result in a 16 byte aligned IP header after

@@ -4020,8 +4019,6 @@ e1000_alloc_rx_buffers(struct e1000_adap
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_buffer_len;
 map_skb:

@@ -4135,8 +4132,6 @@ e1000_alloc_rx_buffers_ps(struct e1000_a
 		 */
 		skb_reserve(skb, NET_IP_ALIGN);
 
-		skb->dev = netdev;
-
 		buffer_info->skb = skb;
 		buffer_info->length = adapter->rx_ps_bsize0;
 		buffer_info->dma = pci_map_single(pdev, skb->data,