[PATCH] net: Disable queueing when carrier is lost (take 2)

[PATCH] net: Disable queueing when carrier is lost (take 2)

[Tommy Christensen]

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

Some network drivers call netif_stop_queue() when detecting loss of
carrier. This leads to packets being queued up at the qdisc level for
an unbound period of time. In order to prevent this effect, the core
networking stack will now seize to queue packets for any device, that
is operationally down (i.e. the queue is flushed and disabled).

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


[-- Attachment #2: carrier-4.patch --]
[-- Type: text/plain, Size: 2640 bytes --]

diff -ru linux-2.6.12-rc3/net/core/dev.c linux-2.6.12-work/net/core/dev.c
--- linux-2.6.12-rc3/net/core/dev.c	2005-04-06 16:43:37.000000000 +0200
+++ linux-2.6.12-work/net/core/dev.c	2005-05-02 23:16:40.329156594 +0200
@@ -2203,14 +2203,10 @@
 	unsigned flags;
 
 	flags = (dev->flags & ~(IFF_PROMISC |
-				IFF_ALLMULTI |
-				IFF_RUNNING)) | 
+				IFF_ALLMULTI)) |
 		(dev->gflags & (IFF_PROMISC |
 				IFF_ALLMULTI));
 
-	if (netif_running(dev) && netif_carrier_ok(dev))
-		flags |= IFF_RUNNING;
-
 	return flags;
 }
 
diff -ru linux-2.6.12-rc3/net/core/link_watch.c linux-2.6.12-work/net/core/link_watch.c
--- linux-2.6.12-rc3/net/core/link_watch.c	2005-03-04 09:55:42.000000000 +0100
+++ linux-2.6.12-work/net/core/link_watch.c	2005-05-02 22:40:59.773471515 +0200
@@ -16,6 +16,7 @@
 #include <linux/netdevice.h>
 #include <linux/if.h>
 #include <net/sock.h>
+#include <net/pkt_sched.h>
 #include <linux/rtnetlink.h>
 #include <linux/jiffies.h>
 #include <linux/spinlock.h>
@@ -74,6 +75,12 @@
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 		if (dev->flags & IFF_UP) {
+			if (netif_carrier_ok(dev)) {
+				WARN_ON(dev->qdisc_sleeping == &noop_qdisc);
+				dev_activate(dev);
+			} else
+				dev_deactivate(dev);
+
 			netdev_state_change(dev);
 		}
 
diff -ru linux-2.6.12-rc3/net/sched/sch_generic.c linux-2.6.12-work/net/sched/sch_generic.c
--- linux-2.6.12-rc3/net/sched/sch_generic.c	2005-03-04 09:55:44.000000000 +0100
+++ linux-2.6.12-work/net/sched/sch_generic.c	2005-05-02 23:07:49.367184929 +0200
@@ -514,6 +514,9 @@
 
 void dev_activate(struct net_device *dev)
 {
+	if (dev->flags & IFF_RUNNING)
+		return;
+
 	/* No queueing discipline is attached to device;
 	   create default one i.e. pfifo_fast for devices,
 	   which need queueing and noqueue_qdisc for
@@ -539,6 +542,11 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 
+	if (!netif_carrier_ok(dev))
+		/* Delay activation until next carrier-on event */
+		return;
+	dev->flags |= IFF_RUNNING;
+
 	spin_lock_bh(&dev->queue_lock);
 	rcu_assign_pointer(dev->qdisc, dev->qdisc_sleeping);
 	if (dev->qdisc != &noqueue_qdisc) {
@@ -552,15 +560,18 @@
 {
 	struct Qdisc *qdisc;
 
-	spin_lock_bh(&dev->queue_lock);
-	qdisc = dev->qdisc;
-	dev->qdisc = &noop_qdisc;
+	if (dev->flags & IFF_RUNNING) {
+		spin_lock_bh(&dev->queue_lock);
+		qdisc = dev->qdisc;
+		dev->qdisc = &noop_qdisc;
 
-	qdisc_reset(qdisc);
+		qdisc_reset(qdisc);
 
-	spin_unlock_bh(&dev->queue_lock);
+		spin_unlock_bh(&dev->queue_lock);
 
-	dev_watchdog_down(dev);
+		dev_watchdog_down(dev);
+	}
+	dev->flags &= ~IFF_RUNNING;
 
 	while (test_bit(__LINK_STATE_SCHED, &dev->state))
 		yield();

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Herbert Xu]

Hi Tommy:

On Tue, May 03, 2005 at 01:01:19AM +0200, Tommy Christensen wrote:
> Some network drivers call netif_stop_queue() when detecting loss of
> carrier. This leads to packets being queued up at the qdisc level for
> an unbound period of time. In order to prevent this effect, the core
> networking stack will now seize to queue packets for any device, that
> is operationally down (i.e. the queue is flushed and disabled).

This looks great.

> @@ -552,15 +560,18 @@
>  {
>  	struct Qdisc *qdisc;
>  
> -	spin_lock_bh(&dev->queue_lock);
> -	qdisc = dev->qdisc;
> -	dev->qdisc = &noop_qdisc;
> +	if (dev->flags & IFF_RUNNING) {
> +		spin_lock_bh(&dev->queue_lock);
> +		qdisc = dev->qdisc;
> +		dev->qdisc = &noop_qdisc;
>  
> -	qdisc_reset(qdisc);
> +		qdisc_reset(qdisc);
>  
> -	spin_unlock_bh(&dev->queue_lock);
> +		spin_unlock_bh(&dev->queue_lock);
>  
> -	dev_watchdog_down(dev);
> +		dev_watchdog_down(dev);
> +	}
> +	dev->flags &= ~IFF_RUNNING;
>  
>  	while (test_bit(__LINK_STATE_SCHED, &dev->state))
>  		yield();

Doing the wait when IFF_RUNNING is off isn't necessary though.  If
IFF_RUNNING isn't set, then either the device has never been activated
or we've already carried out those waits the last time we were in
dev_deactivate.

I understand your preference for defensive programming.  However, in
cases like this it's better to do the obvious thing because:

1) We don't cover up bugs that may come back to bite us elsewhere.
2) We don't give people misconceptions.  If they're unfamiliar with
the system they may infer things from this code that aren't necessarily
the case.

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

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Thomas Graf]

* Herbert Xu <20050503100306.GB29788@gondor.apana.org.au> 2005-05-03 20:03
> On Tue, May 03, 2005 at 01:01:19AM +0200, Tommy Christensen wrote:
> > Some network drivers call netif_stop_queue() when detecting loss of
> > carrier. This leads to packets being queued up at the qdisc level for
> > an unbound period of time. In order to prevent this effect, the core
> > networking stack will now seize to queue packets for any device, that
> > is operationally down (i.e. the queue is flushed and disabled).
> 
> This looks great.
> 
> > @@ -552,15 +560,18 @@
> >  {
> >  	struct Qdisc *qdisc;
> >  
> > -	spin_lock_bh(&dev->queue_lock);
> > -	qdisc = dev->qdisc;
> > -	dev->qdisc = &noop_qdisc;
> > +	if (dev->flags & IFF_RUNNING) {
> > +		spin_lock_bh(&dev->queue_lock);
> > +		qdisc = dev->qdisc;
> > +		dev->qdisc = &noop_qdisc;
> >  
> > -	qdisc_reset(qdisc);
> > +		qdisc_reset(qdisc);
> >  
> > -	spin_unlock_bh(&dev->queue_lock);
> > +		spin_unlock_bh(&dev->queue_lock);
> >  
> > -	dev_watchdog_down(dev);
> > +		dev_watchdog_down(dev);
> > +	}
> > +	dev->flags &= ~IFF_RUNNING;
> >  
> >  	while (test_bit(__LINK_STATE_SCHED, &dev->state))
> >  		yield();
> 
> Doing the wait when IFF_RUNNING is off isn't necessary though.  If
> IFF_RUNNING isn't set, then either the device has never been activated
> or we've already carried out those waits the last time we were in
> dev_deactivate.

I do like the patch, no question but IFF_RUNNING is still abused
by drivers and some subsystems. So I'm not sure how reliable the
above code will be without those cases fixed. I submitted a
patchset once to fix some of them, not sure about the status. Also,
what about those drivers that do not support or do not use
netif_carrier_(on|off)?

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Herbert Xu]

On Tue, May 03, 2005 at 01:18:44PM +0200, Thomas Graf wrote:
> 
> I do like the patch, no question but IFF_RUNNING is still abused
> by drivers and some subsystems. So I'm not sure how reliable the
> above code will be without those cases fixed. I submitted a

Ouch, you're right.  They either need to be fixed or we'll just
have to go back to the simpler version that may call
dev_activate/dev_deactivate repeatedly.

> patchset once to fix some of them, not sure about the status. Also,
> what about those drivers that do not support or do not use
> netif_carrier_(on|off)?

They simply won't benefit from this patch.  This is OK since they're
no worse off than where they were before.  It might also prove to be
an incentive for someone to finally fix them :)

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

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Tommy Christensen]

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

Herbert Xu wrote:
> Doing the wait when IFF_RUNNING is off isn't necessary though.  If
> IFF_RUNNING isn't set, then either the device has never been activated
> or we've already carried out those waits the last time we were in
> dev_deactivate.

Right. But it still depends on what the synchronization is meant to
protect us from. It isn't clear to me, whether it's
  o packets in flight
  o qdisc references
  o device references
so yes, I tried to play it safe.

Anyway, given the comments from Thomas, I've pulled out the IFF_RUNNING
stuff. We can add this later, when the other uses have been sorted out.

> I understand your preference for defensive programming.  However, in
> cases like this it's better to do the obvious thing because:
> 
> 1) We don't cover up bugs that may come back to bite us elsewhere.
> 2) We don't give people misconceptions.  If they're unfamiliar with
> the system they may infer things from this code that aren't necessarily
> the case.

I totally agree with this.

-Tommy

[-- Attachment #2: carrier-5.patch --]
[-- Type: text/plain, Size: 1293 bytes --]

diff -ru linux-2.6.12-rc3/net/core/link_watch.c linux-2.6.12-work/net/core/link_watch.c
--- linux-2.6.12-rc3/net/core/link_watch.c	2005-03-04 09:55:42.000000000 +0100
+++ linux-2.6.12-work/net/core/link_watch.c	2005-05-02 22:40:59.000000000 +0200
@@ -16,6 +16,7 @@
 #include <linux/netdevice.h>
 #include <linux/if.h>
 #include <net/sock.h>
+#include <net/pkt_sched.h>
 #include <linux/rtnetlink.h>
 #include <linux/jiffies.h>
 #include <linux/spinlock.h>
@@ -74,6 +75,12 @@
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 		if (dev->flags & IFF_UP) {
+			if (netif_carrier_ok(dev)) {
+				WARN_ON(dev->qdisc_sleeping == &noop_qdisc);
+				dev_activate(dev);
+			} else
+				dev_deactivate(dev);
+
 			netdev_state_change(dev);
 		}
 
diff -ru linux-2.6.12-rc3/net/sched/sch_generic.c linux-2.6.12-work/net/sched/sch_generic.c
--- linux-2.6.12-rc3/net/sched/sch_generic.c	2005-03-04 09:55:44.000000000 +0100
+++ linux-2.6.12-work/net/sched/sch_generic.c	2005-05-04 00:24:55.558105856 +0200
@@ -539,6 +539,10 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 
+	if (!netif_carrier_ok(dev))
+		/* Delay activation until next carrier-on event */
+		return;
+
 	spin_lock_bh(&dev->queue_lock);
 	rcu_assign_pointer(dev->qdisc, dev->qdisc_sleeping);
 	if (dev->qdisc != &noqueue_qdisc) {

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Herbert Xu]

On Wed, May 04, 2005 at 12:32:54AM +0200, Tommy Christensen wrote:
> 
> Anyway, given the comments from Thomas, I've pulled out the IFF_RUNNING
> stuff. We can add this later, when the other uses have been sorted out.

Thanks Tommy.  This version looks good to me.  BTW, you probably
should add a Signed-off-by line.

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

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[David S. Miller]

On Wed, 4 May 2005 09:10:23 +1000
Herbert Xu <herbert@gondor.apana.org.au> wrote:

> On Wed, May 04, 2005 at 12:32:54AM +0200, Tommy Christensen wrote:
> > 
> > Anyway, given the comments from Thomas, I've pulled out the IFF_RUNNING
> > stuff. We can add this later, when the other uses have been sorted out.
> 
> Thanks Tommy.  This version looks good to me.  BTW, you probably
> should add a Signed-off-by line.

Yes, please provide this and I'll happily apply this patch.
Thanks.

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[Tommy Christensen]

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

Herbert Xu wrote:
> Thanks Tommy.  This version looks good to me.  BTW, you probably
> should add a Signed-off-by line.

Sigh. I remembered on all but the final patch ...
Thanks for helping out.

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

[-- Attachment #2: carrier-5.patch --]
[-- Type: text/plain, Size: 1293 bytes --]

diff -ru linux-2.6.12-rc3/net/core/link_watch.c linux-2.6.12-work/net/core/link_watch.c
--- linux-2.6.12-rc3/net/core/link_watch.c	2005-03-04 09:55:42.000000000 +0100
+++ linux-2.6.12-work/net/core/link_watch.c	2005-05-02 22:40:59.000000000 +0200
@@ -16,6 +16,7 @@
 #include <linux/netdevice.h>
 #include <linux/if.h>
 #include <net/sock.h>
+#include <net/pkt_sched.h>
 #include <linux/rtnetlink.h>
 #include <linux/jiffies.h>
 #include <linux/spinlock.h>
@@ -74,6 +75,12 @@
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 		if (dev->flags & IFF_UP) {
+			if (netif_carrier_ok(dev)) {
+				WARN_ON(dev->qdisc_sleeping == &noop_qdisc);
+				dev_activate(dev);
+			} else
+				dev_deactivate(dev);
+
 			netdev_state_change(dev);
 		}
 
diff -ru linux-2.6.12-rc3/net/sched/sch_generic.c linux-2.6.12-work/net/sched/sch_generic.c
--- linux-2.6.12-rc3/net/sched/sch_generic.c	2005-03-04 09:55:44.000000000 +0100
+++ linux-2.6.12-work/net/sched/sch_generic.c	2005-05-04 00:24:55.558105856 +0200
@@ -539,6 +539,10 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 
+	if (!netif_carrier_ok(dev))
+		/* Delay activation until next carrier-on event */
+		return;
+
 	spin_lock_bh(&dev->queue_lock);
 	rcu_assign_pointer(dev->qdisc, dev->qdisc_sleeping);
 	if (dev->qdisc != &noqueue_qdisc) {

Re: [PATCH] net: Disable queueing when carrier is lost (take 2)

[David S. Miller]

On Wed, 04 May 2005 01:28:24 +0200
Tommy Christensen <tommy.christensen@tpack.net> wrote:

> Herbert Xu wrote:
> > Thanks Tommy.  This version looks good to me.  BTW, you probably
> > should add a Signed-off-by line.
> 
> Sigh. I remembered on all but the final patch ...
> Thanks for helping out.
> 
> Signed-off-by: Tommy S. Christensen <tommy.christensen@tpack.net>

Applied, thanks Tommy.