[PATCH] move skb->dev assignment into netdev_alloc_skb

[PATCH] move skb->dev assignment into netdev_alloc_skb

[Christoph Hellwig]

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 <hch@lst.de>

Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000/e1000_main.c	2006-08-04 19:06:09.000000000 +0200
+++ linux-2.6/drivers/net/e1000/e1000_main.c	2006-08-05 14:18:32.000000000 +0200
@@ -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,
Index: linux-2.6/drivers/net/tg3.c
===================================================================
--- linux-2.6.orig/drivers/net/tg3.c	2006-08-04 19:06:11.000000000 +0200
+++ linux-2.6/drivers/net/tg3.c	2006-08-05 14:18:18.000000000 +0200
@@ -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);
Index: linux-2.6/net/core/skbuff.c
===================================================================
--- linux-2.6.orig/net/core/skbuff.c	2006-08-04 19:06:37.000000000 +0200
+++ linux-2.6/net/core/skbuff.c	2006-08-05 14:48:01.000000000 +0200
@@ -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;
 }
 

Re: [PATCH] move skb->dev assignment into netdev_alloc_skb

[David Miller]

From: Christoph Hellwig <hch@lst.de>
Date: Sat, 5 Aug 2006 15:01:09 +0200

> 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 <hch@lst.de>

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.

Re: [PATCH] move skb->dev assignment into netdev_alloc_skb

[Jesse Brandeburg]

On 8/7/06, David Miller <davem@davemloft.net> wrote:
> From: Christoph Hellwig <hch@lst.de>
> Date: Sat, 5 Aug 2006 15:01:09 +0200
>
> > 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 <hch@lst.de>
>
> 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

Re: [PATCH] move skb->dev assignment into netdev_alloc_skb

[Christoph Hellwig]

On Mon, Aug 07, 2006 at 05:29:54PM -0700, Jesse Brandeburg wrote:
> On 8/7/06, David Miller <davem@davemloft.net> wrote:
> >From: Christoph Hellwig <hch@lst.de>
> >Date: Sat, 5 Aug 2006 15:01:09 +0200
> >
> >> 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 <hch@lst.de>
> >
> >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.

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:


Index: linux-2.6/drivers/net/e1000/e1000_main.c
===================================================================
--- linux-2.6.orig/drivers/net/e1000/e1000_main.c	2006-08-04 19:06:09.000000000 +0200
+++ linux-2.6/drivers/net/e1000/e1000_main.c	2006-08-05 14:18:32.000000000 +0200
@@ -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,

Re: [PATCH] move skb->dev assignment into netdev_alloc_skb

[Brandeburg, Jesse]

On Sat, 12 Aug 2006, Christoph Hellwig wrote:
> > >> Signed-off-by: Christoph Hellwig <hch@lst.de>
> > >
> > >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.
> 
> 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 <hch@lst.de>
Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>

---

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

diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
index 627f224..66c0325 100644
--- a/drivers/net/e1000/e1000_main.c
+++ b/drivers/net/e1000/e1000_main.c
@@ -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,

Re: [PATCH] move skb->dev assignment into netdev_alloc_skb

[Auke Kok]

Brandeburg, Jesse wrote:
> On Sat, 12 Aug 2006, Christoph Hellwig wrote:
>>>>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>>>> 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.
>> 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


> 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 <hch@lst.de>
> Acked-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> 
> ---
> 
>  drivers/net/e1000/e1000_main.c |   19 +++++++------------
>  1 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/e1000/e1000_main.c b/drivers/net/e1000/e1000_main.c
> index 627f224..66c0325 100644
> --- a/drivers/net/e1000/e1000_main.c
> +++ b/drivers/net/e1000/e1000_main.c
> @@ -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,