net: Disable queueing when carrier is lost

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:

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 <[email protected]> reports succesfull testing of the patch with Quagga.

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.

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

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

Quoted 5 lines · Tommy Christensen

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.

Quoted 2 lines

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.

Quoted 2 lines

... 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,

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.

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.

Quoted 7 lines · Tommy Christensen

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?

Review line 80

Quoted 7 lines · Tommy Christensen

(quoted diff) +3 -0

@@ -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?

Quoted 7 lines

diff -ru linux-2.6.12-rc3/net/sched/sch_generic.c linux-2.6.12-work/net/sched/sch_generic.c

net/sched/sch_generic.c +0 -0

@@ -539,6 +539,10 @@
 		write_unlock_bh(&qdisc_tree_lock);
 	}
 

net/sched/sch_generic.c line 544

Quoted 3 lines

net/sched/sch_generic.c +3 -0

+	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?

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.

Quoted 5 lines

net/sched/sch_generic.c line 544

Quoted 3 lines

net/sched/sch_generic.c +3 -0

+	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.

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.

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.

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.

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).

Quoted 3 lines

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?

Quoted 3 lines · Tommy Christensen

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.

Quoted 1 line

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.