[NET] Move local_bh_disable back in dev_queue_xmit

[NET] Move local_bh_disable back in dev_queue_xmit

[Herbert Xu]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

Hi Ingo:

Your recent fix to dev_queue_xmit moved the local_bh_disable to
include stuff like skb_linearize and skb_checksum_help.  These
are potentially expensive operations.  Since they don't need to
run with preempt off, we could simply move the local_bh_enable
up instead.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: p --]
[-- Type: text/plain, Size: 1009 bytes --]

===== net/core/dev.c 1.171 vs edited =====
--- 1.171/net/core/dev.c	2004-11-02 12:40:59 +11:00
+++ edited/net/core/dev.c	2004-11-03 09:31:09 +11:00
@@ -1261,11 +1261,6 @@
 	struct Qdisc *q;
 	int rc = -ENOMEM;
 
-	/* Disable soft irqs for various locks below. Also 
-	 * stops preemption for RCU. 
-	 */
-	local_bh_disable(); 
-
 	if (skb_shinfo(skb)->frag_list &&
 	    !(dev->features & NETIF_F_FRAGLIST) &&
 	    __skb_linearize(skb, GFP_ATOMIC))
@@ -1290,6 +1285,11 @@
 	      	if (skb_checksum_help(skb, 0))
 	      		goto out_kfree_skb;
 
+	/* Disable soft irqs for various locks below. Also 
+	 * stops preemption for RCU. 
+	 */
+	local_bh_disable(); 
+
 	/* Updates of qdisc are serialized by queue_lock. 
 	 * The struct Qdisc which is pointed to by qdisc is now a 
 	 * rcu structure - it may be accessed without acquiring 
@@ -1363,10 +1363,10 @@
 	}
 out_enetdown:
 	rc = -ENETDOWN;
-out_kfree_skb:
-	kfree_skb(skb);
 out:
 	local_bh_enable();
+out_kfree_skb:
+	kfree_skb(skb);
 	return rc;
 }
 

Re: [NET] Move local_bh_disable back in dev_queue_xmit

[Ingo Molnar]

sure - perfectly fine to me.

	Ingo

* Herbert Xu <herbert@gondor.apana.org.au> wrote:

> Hi Ingo:
> 
> Your recent fix to dev_queue_xmit moved the local_bh_disable to
> include stuff like skb_linearize and skb_checksum_help.  These
> are potentially expensive operations.  Since they don't need to
> run with preempt off, we could simply move the local_bh_enable
> up instead.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Cheers,
> -- 
> Visit Openswan at http://www.openswan.org/
> Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

> ===== net/core/dev.c 1.171 vs edited =====
> --- 1.171/net/core/dev.c	2004-11-02 12:40:59 +11:00
> +++ edited/net/core/dev.c	2004-11-03 09:31:09 +11:00
> @@ -1261,11 +1261,6 @@
>  	struct Qdisc *q;
>  	int rc = -ENOMEM;
>  
> -	/* Disable soft irqs for various locks below. Also 
> -	 * stops preemption for RCU. 
> -	 */
> -	local_bh_disable(); 
> -
>  	if (skb_shinfo(skb)->frag_list &&
>  	    !(dev->features & NETIF_F_FRAGLIST) &&
>  	    __skb_linearize(skb, GFP_ATOMIC))
> @@ -1290,6 +1285,11 @@
>  	      	if (skb_checksum_help(skb, 0))
>  	      		goto out_kfree_skb;
>  
> +	/* Disable soft irqs for various locks below. Also 
> +	 * stops preemption for RCU. 
> +	 */
> +	local_bh_disable(); 
> +
>  	/* Updates of qdisc are serialized by queue_lock. 
>  	 * The struct Qdisc which is pointed to by qdisc is now a 
>  	 * rcu structure - it may be accessed without acquiring 
> @@ -1363,10 +1363,10 @@
>  	}
>  out_enetdown:
>  	rc = -ENETDOWN;
> -out_kfree_skb:
> -	kfree_skb(skb);
>  out:
>  	local_bh_enable();
> +out_kfree_skb:
> +	kfree_skb(skb);
>  	return rc;
>  }
>  

Re: [NET] Move local_bh_disable back in dev_queue_xmit

[Tommy Christensen]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Herbert Xu wrote:
> Hi Ingo:
> 
> Your recent fix to dev_queue_xmit moved the local_bh_disable to
> include stuff like skb_linearize and skb_checksum_help.  These
> are potentially expensive operations.  Since they don't need to
> run with preempt off, we could simply move the local_bh_enable
> up instead.

No, this breaks the normal return path.

How about this instead?

Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>


[-- Attachment #2: dev.c.patch --]
[-- Type: text/plain, Size: 1291 bytes --]

--- linux-2.6.10-rc1-bk/net/core/dev.c	Wed Nov  3 15:10:43 2004
+++ linux-2.6.10-rc1-work/net/core/dev.c	Wed Nov  3 15:31:22 2004
@@ -1261,11 +1261,6 @@
 	struct Qdisc *q;
 	int rc = -ENOMEM;
 
-	/* Disable soft irqs for various locks below. Also 
-	 * stops preemption for RCU. 
-	 */
-	local_bh_disable(); 
-
 	if (skb_shinfo(skb)->frag_list &&
 	    !(dev->features & NETIF_F_FRAGLIST) &&
 	    __skb_linearize(skb, GFP_ATOMIC))
@@ -1290,6 +1285,11 @@
 	      	if (skb_checksum_help(skb, 0))
 	      		goto out_kfree_skb;
 
+	/* Disable soft irqs for various locks below. Also 
+	 * stops preemption for RCU. 
+	 */
+	local_bh_disable(); 
+
 	/* Updates of qdisc are serialized by queue_lock. 
 	 * The struct Qdisc which is pointed to by qdisc is now a 
 	 * rcu structure - it may be accessed without acquiring 
@@ -1352,7 +1352,6 @@
 			if (net_ratelimit())
 				printk(KERN_CRIT "Virtual device %s asks to "
 				       "queue packet!\n", dev->name);
-			goto out_enetdown;
 		} else {
 			/* Recursion is detected! It is possible,
 			 * unfortunately */
@@ -1361,10 +1360,13 @@
 				       "%s, fix it urgently!\n", dev->name);
 		}
 	}
-out_enetdown:
+
 	rc = -ENETDOWN;
+	local_bh_enable();
+
 out_kfree_skb:
 	kfree_skb(skb);
+	return rc;
 out:
 	local_bh_enable();
 	return rc;

Re: [NET] Move local_bh_disable back in dev_queue_xmit

[David S. Miller]

On Wed, 03 Nov 2004 15:48:49 +0100
Tommy Christensen <tommy.christensen@tpack.net> wrote:

> Herbert Xu wrote:
> > Hi Ingo:
> > 
> > Your recent fix to dev_queue_xmit moved the local_bh_disable to
> > include stuff like skb_linearize and skb_checksum_help.  These
> > are potentially expensive operations.  Since they don't need to
> > run with preempt off, we could simply move the local_bh_enable
> > up instead.
> 
> No, this breaks the normal return path.
> 
> How about this instead?

Yep, this one is better.  Patch applied, thanks everyone.

Re: [NET] Move local_bh_disable back in dev_queue_xmit

[Herbert Xu]

Tommy Christensen <tommy.christensen@tpack.net> wrote:
>
> No, this breaks the normal return path.
> 
> How about this instead?

Thanks, your patch looks much better.
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt