[PATCH] net: Disable queueing when carrier is lost

[PATCH] net: Disable queueing when carrier is lost

[Tommy Christensen]

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

A while back we had a very long thread about the queueing behavior of
netdevices in a loss-of-carrier situation. This thread had the poetic
subject: [patch 4/10] s390: network driver.

The executive summary:

Certain network drivers call netif_stop_queue() when they detect a loss
of carrier. This has some unfortunate effects on the current networking
stack, since packets are now being queued up at the qdisc level for an
unbound period of time:
  *) the socket send buffer may be exhausted, preventing transmission
     to all devices (from this particular socket)
  *) resources are held "indefinitely": memory, socket/dst/module refcounts
  *) when carrier is re-established stale packets are sent out on the
     network, which could have undesirable effects

The best solution seems to be to simply disable the queueing at the qdisc
layer when this situation arises. That approach has been implemented in
the patch below. Hasso Tepper <hasso@estpak.ee> reports succesfull testing
of the patch with Quagga.

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

[-- Attachment #2: carrier.patch --]
[-- Type: text/plain, Size: 1655 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-04-26 22:19:22.939393488 +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,9 @@
 		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
 
 		if (dev->flags & IFF_UP) {
+			if (netif_carrier_ok(dev) &&
+			    dev->qdisc_sleeping != &noop_qdisc)
+				dev_activate(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-04-26 22:19:22.939393488 +0200
@@ -185,6 +185,7 @@
 static void dev_watchdog(unsigned long arg)
 {
 	struct net_device *dev = (struct net_device *)arg;
+	int check_carrier = 0;
 
 	spin_lock(&dev->xmit_lock);
 	if (dev->qdisc != &noop_qdisc) {
@@ -198,10 +199,23 @@
 			}
 			if (!mod_timer(&dev->watchdog_timer, jiffies + dev->watchdog_timeo))
 				dev_hold(dev);
-		}
+		} else
+			check_carrier = 1;
 	}
 	spin_unlock(&dev->xmit_lock);
 
+	if (check_carrier) {
+		spin_lock(&dev->queue_lock);
+		if (!netif_carrier_ok(dev) && netif_queue_stopped(dev)) {
+			struct Qdisc *qdisc;
+
+			qdisc = dev->qdisc;
+			dev->qdisc = &noop_qdisc;
+			qdisc_reset(qdisc);
+		}
+		spin_unlock(&dev->queue_lock);
+	}
+
 	dev_put(dev);
 }
 

Re: [PATCH] net: Disable queueing when carrier is lost

[Herbert Xu]

Tommy Christensen <tommy.christensen@tpack.net> wrote:
> 
> A while back we had a very long thread about the queueing behavior of
> netdevices in a loss-of-carrier situation. This thread had the poetic
> subject: [patch 4/10] s390: network driver.

I remember that thread :) Thanks for chasing this up.

> The executive summary:

Agreed.

> The best solution seems to be to simply disable the queueing at the qdisc
> layer when this situation arises. That approach has been implemented in
> the patch below. Hasso Tepper <hasso@estpak.ee> reports succesfull testing
> of the patch with Quagga.

The idea is good.  However, the implementation has a problem in it.
You're relying on the watchdog which may not be there.  The watchdog
is only present if tx_timeout is defined.

Also, this still doesn't help us if the driver itself has a TX queue
that is hoarding the packets.  So the driver itself needs to make sure
that when the carrier goes off that its TX queue is flushed too.

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

[Herbert Xu]

On Wed, Apr 27, 2005 at 08:52:59PM +0200, Tommy Christensen wrote:
> 
> My theory was this: Almost all drivers should be able to use the generic
> watchdog (and I believe most of them do). If the "TX stalled" supervision
> isn't appropriate for some particular driver, e.g. due to unorthodox use
> of netif_stop_queue, then I didn't want to force my addition on this
> driver either.

Not having a TX timeout handler doesn't mean that the driver is doing
something weird.  If you do a grep in drivers/net you'll find loads
of drivers that don't have TX timeout handlers but their handling of
stop_queue/start_queue is exactly the same as anybody else.

There's also another problem.  The thing that triggered the original
discussion is the fact that the socket send buffer was filled up.
Theoretically, it is possible to exhaust someone's socket buffer
without filling up a NIC's TX ring.  Assuming that the NIC does not
transmit at all when the carrier is off, the watchdog would not trigger
and your application will block anyway.

> Hooking into dev_watchdog() has the additional benefit of adding some
> latency, so that a short-break doesn't necessarily trigger the flushing.

I don't think this is too important.  If your link is flapping constantly
then you've got a serious problem.  If it's just an isolated event then
whether we do the flush or not isn't going to have a significant impact
on the system.

Besides, someone might be watching from user-space and could have taken
much more drastic actions as a result of the carrier off message which is
certainly not delayed.

> ... unless the HW already takes care of this by draining the packets
> from the ring buffer, disregarding the link status.

Although this may be true for a lot of NICs, you can't bank on that.

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

[Tommy Christensen]

Herbert Xu wrote:
> On Wed, Apr 27, 2005 at 08:52:59PM +0200, Tommy Christensen wrote:
> 
>>My theory was this: Almost all drivers should be able to use the generic
>>watchdog (and I believe most of them do). If the "TX stalled" supervision
>>isn't appropriate for some particular driver, e.g. due to unorthodox use
>>of netif_stop_queue, then I didn't want to force my addition on this
>>driver either.
> 
> 
> Not having a TX timeout handler doesn't mean that the driver is doing
> something weird.  If you do a grep in drivers/net you'll find loads
> of drivers that don't have TX timeout handlers but their handling of
> stop_queue/start_queue is exactly the same as anybody else.

Hmm, maybe this is more common than I thought. But do any of these really
have a problem? I.e. do they call netif_stop_queue on link down?
That's the case I'm trying to address with the patch.

> There's also another problem.  The thing that triggered the original
> discussion is the fact that the socket send buffer was filled up.
> Theoretically, it is possible to exhaust someone's socket buffer
> without filling up a NIC's TX ring.  Assuming that the NIC does not
> transmit at all when the carrier is off, the watchdog would not trigger
> and your application will block anyway.

This is indeed possible, but hopefully you can agree that this would be
a driver bug. As stated above, I'm not trying to solve everything. We
have to assume some level of sanity of the drivers. E.g. for a NIC that
stalls the TX engine on carrier off, the driver would have to flush the
TX ring and either call netif_stop_queue or discard packets in their
hard_start_xmit function.  At present, even such well-behaving drivers
would hit the problem, because packets were piling up in the qdisc.

>>Hooking into dev_watchdog() has the additional benefit of adding some
>>latency, so that a short-break doesn't necessarily trigger the flushing.
> 
> 
> I don't think this is too important.  If your link is flapping constantly
> then you've got a serious problem.  If it's just an isolated event then
> whether we do the flush or not isn't going to have a significant impact
> on the system.
> 
> Besides, someone might be watching from user-space and could have taken
> much more drastic actions as a result of the carrier off message which is
> certainly not delayed.

Good point. So I shouldn't be too carefull.

>>... unless the HW already takes care of this by draining the packets
>>from the ring buffer, disregarding the link status.
> 
> 
> Although this may be true for a lot of NICs, you can't bank on that.
> 
> Cheers,

Thanks for your comments, Herbert.

Tommy

Re: [PATCH] net: Disable queueing when carrier is lost

[Herbert Xu]

On Thu, Apr 28, 2005 at 01:27:57AM +0200, Tommy Christensen wrote:
> 
> Hmm, maybe this is more common than I thought. But do any of these really
> have a problem? I.e. do they call netif_stop_queue on link down?
> That's the case I'm trying to address with the patch.

You're still assuming that the hardware will continue to drain packets
when the carrier goes off.  This may be true for a lot of NICs, but it
is certainly not universal.
 
> This is indeed possible, but hopefully you can agree that this would be
> a driver bug. As stated above, I'm not trying to solve everything. We
> have to assume some level of sanity of the drivers. E.g. for a NIC that
> stalls the TX engine on carrier off, the driver would have to flush the
> TX ring and either call netif_stop_queue or discard packets in their
> hard_start_xmit function.  At present, even such well-behaving drivers
> would hit the problem, because packets were piling up in the qdisc.

I am not saying that there is nothing to fix.  I'm simply stating
that doing it in the watchdog is not ideal for two reasons:

1) The watchdog is not always there.
2) The delay introduced by the watchdog is driver-dependent and
varies wildly.  For the purpose of shutting down the qdisc after
a carrier off we want everything to use the same delay (if we're
going to delay at all).

So instead of doing it in the watchdog, just do both actions in
the link_watch worker.

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

[Tommy Christensen]

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

On Thu, 2005-04-28 at 01:43, Herbert Xu wrote:
> On Thu, Apr 28, 2005 at 01:27:57AM +0200, Tommy Christensen wrote:
> > This is indeed possible, but hopefully you can agree that this would be
> > a driver bug. As stated above, I'm not trying to solve everything. We
> > have to assume some level of sanity of the drivers. E.g. for a NIC that
> > stalls the TX engine on carrier off, the driver would have to flush the
> > TX ring and either call netif_stop_queue or discard packets in their
> > hard_start_xmit function.  At present, even such well-behaving drivers
> > would hit the problem, because packets were piling up in the qdisc.
> 
> I am not saying that there is nothing to fix.  I'm simply stating
> that doing it in the watchdog is not ideal for two reasons:
> 
> 1) The watchdog is not always there.
> 2) The delay introduced by the watchdog is driver-dependent and
> varies wildly.  For the purpose of shutting down the qdisc after
> a carrier off we want everything to use the same delay (if we're
> going to delay at all).
> 
> So instead of doing it in the watchdog, just do both actions in
> the link_watch worker.

Originally, I was afraid it could get too trigger happy, so I left
this idea. Perhaps that's not such a big deal afterall.

How does this look?


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


[-- Attachment #2: Type: text/x-patch, Size: 1347 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-04-29 11:22:05.330262591 +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)) {
+				if (dev->qdisc_sleeping != &noop_qdisc)
+					dev_activate(dev);
+			} else if (netif_queue_stopped(dev))
+				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-04-29 11:22:05.420250195 +0200
@@ -539,6 +539,10 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 
+	if (!netif_carrier_ok(dev) && netif_queue_stopped(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

[Herbert Xu]

On Fri, Apr 29, 2005 at 11:51:48AM +0200, Tommy Christensen wrote:
> 
> Originally, I was afraid it could get too trigger happy, so I left
> this idea. Perhaps that's not such a big deal afterall.

Even if it does flap we've still got the built-in one second
flap-protection of link_watch.

> How does this look?

Much better :)

> @@ -74,6 +75,12 @@
>  		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
>  
>  		if (dev->flags & IFF_UP) {
> +			if (netif_carrier_ok(dev)) {
> +				if (dev->qdisc_sleeping != &noop_qdisc)
> +					dev_activate(dev);

I don't see how qdisc_sleeping can be noop_qdisc unless there is a bug.
So why not convert this to a BUG_ON?

> 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-04-29 11:22:05.420250195 +0200
> @@ -539,6 +539,10 @@
>  		write_unlock_bh(&qdisc_tree_lock);
>  	}
>  
> +	if (!netif_carrier_ok(dev) && netif_queue_stopped(dev))
> +		/* Delay activation until next carrier-on event */
> +		return;

How about moving this check into dev_open?

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

[Tommy Christensen]

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

On Fri, 2005-04-29 at 12:18, Herbert Xu wrote:
> On Fri, Apr 29, 2005 at 11:51:48AM +0200, Tommy Christensen wrote:
> > @@ -74,6 +75,12 @@
> >  		clear_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state);
> >  
> >  		if (dev->flags & IFF_UP) {
> > +			if (netif_carrier_ok(dev)) {
> > +				if (dev->qdisc_sleeping != &noop_qdisc)
> > +					dev_activate(dev);
> 
> I don't see how qdisc_sleeping can be noop_qdisc unless there is a bug.
> So why not convert this to a BUG_ON?

Calling dev->open instead of dev_open leads to this.  Probably that
shouldn't be allowed anyway.  But since it doesn't hurt us here, I'd
rather settle for a WARN_ON.

> > 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-04-29 11:22:05.420250195 +0200
> > @@ -539,6 +539,10 @@
> >  		write_unlock_bh(&qdisc_tree_lock);
> >  	}
> >  
> > +	if (!netif_carrier_ok(dev) && netif_queue_stopped(dev))
> > +		/* Delay activation until next carrier-on event */
> > +		return;
> 
> How about moving this check into dev_open?

But that would indeed leave qdisc_sleeping set to noop_qdisc, wouldn't
it?  Besides, I wanted to catch qdisc changes done by tc as well.


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


[-- Attachment #2: Type: text/x-patch, Size: 1351 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-04-29 14:19:07.356024339 +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 if (netif_queue_stopped(dev))
+				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-04-29 11:22:05.420250195 +0200
@@ -539,6 +539,10 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 
+	if (!netif_carrier_ok(dev) && netif_queue_stopped(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

[Herbert Xu]

On Fri, Apr 29, 2005 at 02:22:36PM +0200, Tommy Christensen wrote:
> 
> Calling dev->open instead of dev_open leads to this.  Probably that
> shouldn't be allowed anyway.  But since it doesn't hurt us here, I'd

That's definitely not allowed.

> rather settle for a WARN_ON.

Fine by me.

> > > +	if (!netif_carrier_ok(dev) && netif_queue_stopped(dev))
> > > +		/* Delay activation until next carrier-on event */
> > > +		return;
> > 
> > How about moving this check into dev_open?
> 
> But that would indeed leave qdisc_sleeping set to noop_qdisc, wouldn't
> it?  Besides, I wanted to catch qdisc changes done by tc as well.

You're right.  However, the netif_queue_stopped check doesn't make sense.
If the queue isn't stopped, you'd be leaving the qdisc open which means
that it can fill up later on.  Remember the problem is not just with NIC
drivers stopping the queue explicitly, some NICs simply won't process the
TX rings when the carrier is off.

The other concern I have is that this code can call dev_activate
or dev_deactivate twice in a row.  As far as I can tell this is
harmless for the moment but it would be nice if we can avoid it.

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

[Herbert Xu]

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

Hi Tommy:

What if we take a more direct approach and just drop the packets before
it hits hard_start_xmit? This also makes it easy for the drivers to do
TX ring flushing if it's necessary since we're guaranteeing (for drivers
that are not lockless anyway) that once carrier goes off while the xmit
lock is held hard_start_xmit will not be called again.

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: 1132 bytes --]

--- k/net/core/dev.c  (mode:100644)
+++ l/net/core/dev.c  (mode:100644)
@@ -1312,6 +1312,11 @@ int dev_queue_xmit(struct sk_buff *skb)
 
 			HARD_TX_LOCK(dev, cpu);
 
+			if (unlikely(!netif_carrier_ok(dev))) {
+				HARD_TX_UNLOCK(dev);
+				goto out_no_carrier;
+			}
+
 			if (!netif_queue_stopped(dev)) {
 				if (netdev_nit)
 					dev_queue_xmit_nit(skb, dev);
@@ -1335,6 +1340,7 @@ int dev_queue_xmit(struct sk_buff *skb)
 		}
 	}
 
+out_no_carrier:
 	rc = -ENETDOWN;
 	local_bh_enable();
 
--- k/net/sched/sch_generic.c  (mode:100644)
+++ l/net/sched/sch_generic.c  (mode:100644)
@@ -134,6 +134,11 @@ int qdisc_restart(struct net_device *dev
 			/* And release queue */
 			spin_unlock(&dev->queue_lock);
 
+			if (unlikely(!netif_carrier_ok(dev))) {
+				kfree_skb(skb);
+				goto out_no_carrier;
+			}
+
 			if (!netif_queue_stopped(dev)) {
 				int ret;
 				if (netdev_nit)
@@ -141,6 +146,7 @@ int qdisc_restart(struct net_device *dev
 
 				ret = dev->hard_start_xmit(skb, dev);
 				if (ret == NETDEV_TX_OK) { 
+out_no_carrier:
 					if (!nolock) {
 						dev->xmit_lock_owner = -1;
 						spin_unlock(&dev->xmit_lock);

Re: [PATCH] net: Disable queueing when carrier is lost

[Tommy Christensen]

Herbert Xu wrote:
> You're right.  However, the netif_queue_stopped check doesn't make sense.
> If the queue isn't stopped, you'd be leaving the qdisc open which means
> that it can fill up later on.  Remember the problem is not just with NIC
> drivers stopping the queue explicitly, some NICs simply won't process the
> TX rings when the carrier is off.

I know. I've given this a lot of thought already - and failed to come
up with a solution for everything.

Regarding the sort of NICs you mention, I came to the conclusion that
we shouldn't try to fix this in the stack. They indicate that they can
take more packets (by NOT calling netif_stop_queue), so we should obey
this and leave it to the drivers to handle things, e.g. by dropping the
packets if needed. Otherwise we deprive the drivers the option of
maintaining their tx_carrier_errors statistics, for instance. And,
theoretically, a driver could even depend on having its hard_start_xmit
invoked in order to kickstart its TX engine.

As stated earlier: if a driver holds on to packets "indefinitely", it
is buggy. This should be fixed, rather than the core having to jump
through loops trying to remedy things.

So, I am reluctant to drop this check for netif_queue_stopped. Doing so
would needlessly impact the drivers that work fine already (or at least
should be handling this situation).

> The other concern I have is that this code can call dev_activate
> or dev_deactivate twice in a row.  As far as I can tell this is
> harmless for the moment but it would be nice if we can avoid it.

Agreed, this isn't ideal. However, if this is the only downside, it
is something that I'd be happy to live with. I don't see any easy
way to avoid it.

Ideas, anyone?


-Tommy

Re: [PATCH] net: Disable queueing when carrier is lost

[Tommy Christensen]

Herbert Xu wrote:
> What if we take a more direct approach and just drop the packets before
> it hits hard_start_xmit? This also makes it easy for the drivers to do
> TX ring flushing if it's necessary since we're guaranteeing (for drivers
> that are not lockless anyway) that once carrier goes off while the xmit
> lock is held hard_start_xmit will not be called again.

Something like this has been considered already. It's simple and obvious,
but has it's drawbacks as well:

  - it adds a slight overhead to the TX path (some people are rather
    sensitive about this)
  - it impacts all sorts of drivers, including those that don't need fixing
  - it doesn't work as intended! qdisc_run checks for queue_stopped before
    calling qdisc_restart

-Tommy

Re: [PATCH] net: Disable queueing when carrier is lost

[Herbert Xu]

Hi Tommy:

I agree with you now that your patch is the best way to go.

On Sat, Apr 30, 2005 at 02:57:12PM +0200, Tommy Christensen wrote:
>
> Regarding the sort of NICs you mention, I came to the conclusion that
> we shouldn't try to fix this in the stack. They indicate that they can
> take more packets (by NOT calling netif_stop_queue), so we should obey
> this and leave it to the drivers to handle things, e.g. by dropping the
> packets if needed. Otherwise we deprive the drivers the option of
> maintaining their tx_carrier_errors statistics, for instance. And,
> theoretically, a driver could even depend on having its hard_start_xmit
> invoked in order to kickstart its TX engine.

We should document this so that NIC drivers are told about this.  Any
NICs that can't process their TX rings while the carrier is off needs
to stop their queue just before calling netif_carrier_off.
 
> So, I am reluctant to drop this check for netif_queue_stopped. Doing so
> would needlessly impact the drivers that work fine already (or at least
> should be handling this situation).

I don't see how those drivers are worse off without this check.  When
the carrier is off it doesn't really matter whether we're sending them
packets or not.  Besides, you didn't put this check in the link_watch
code path which would affect them a lot more than this path.

> >The other concern I have is that this code can call dev_activate
> >or dev_deactivate twice in a row.  As far as I can tell this is
> >harmless for the moment but it would be nice if we can avoid it.
> 
> Agreed, this isn't ideal. However, if this is the only downside, it
> is something that I'd be happy to live with. I don't see any easy
> way to avoid it.
> 
> Ideas, anyone?

You mean how we can avoid the double call? Here is one way.  Move
the setting of IF_RUNNING out of dev_get_flags and into dev_activate.
This would guarantee that IF_RUNNING is set iff the device has a qdisc.
Assuming that you remove the aformentioned netif_queue_stopped check,
the device has a qdisc iff the carrier is on.  So this preserves the
meaning of IF_RUNNING.

Now you can avoid the double call by returning early in dev_activate if
IF_RUNNING is set, and in dev_deactivate if IF_RUNNING is not set.

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

[Tommy Christensen]

Herbert Xu wrote:
> On Sat, Apr 30, 2005 at 02:57:12PM +0200, Tommy Christensen wrote:
>>So, I am reluctant to drop this check for netif_queue_stopped. Doing so
>>would needlessly impact the drivers that work fine already (or at least
>>should be handling this situation).
> 
> I don't see how those drivers are worse off without this check.  When
> the carrier is off it doesn't really matter whether we're sending them
> packets or not.  Besides, you didn't put this check in the link_watch
> code path which would affect them a lot more than this path.

My reasoning is reversed. You're saying:
   "What could possibly happen, if we do this?",
whereas I'm saying:
   "This has no benefit, so why risk it?".

I'm just not confident that some odd driver won't choke on this change.

As I'm probably just being too cautious (and your approach does seem
more consistent), I'm sending a new patch following your ideas.
And then Dave has something to choose from ;-)

>>Ideas, anyone?
> 
> You mean how we can avoid the double call? Here is one way.  Move
> the setting of IF_RUNNING out of dev_get_flags and into dev_activate.
> This would guarantee that IF_RUNNING is set iff the device has a qdisc.
> Assuming that you remove the aformentioned netif_queue_stopped check,
> the device has a qdisc iff the carrier is on.  So this preserves the
> meaning of IF_RUNNING.
> 
> Now you can avoid the double call by returning early in dev_activate if
> IF_RUNNING is set, and in dev_deactivate if IF_RUNNING is not set.

Implemented in the new patch, thanks.


-Tommy