qdisc and tx_queue_len cleanups for IFF_NO_QUEUE devices

The flag IFF_NO_QUEUE marks virtual device drivers that doesn't need a default qdisc attached, given they will be backed by physical device, that already have a qdisc attached for pushback.

It is still supported to attach a qdisc to a IFF_NO_QUEUE device, as this can be useful for difference policy reasons (e.g. bandwidth limiting containers). For this to work, the tx_queue_len need to have a sane value, because some qdiscs inherit/copy the tx_queue_len (namely, pfifo, bfifo, gred, htb, plug and sfb).

Commit a813104d9233 ("IFF_NO_QUEUE: Fix for drivers not calling ether_setup()") caught situations where some drivers didn't initialize tx_queue_len. The problem with the commit was choosing 1 as the fallback value.

A qdisc queue length of 1 causes more harm than good, because it creates hard to debug situations for userspace. It gives userspace a false sense of a working config after attaching a qdisc. As low volume traffic (that doesn't activate the qdisc policy) works, like ping, while traffic that e.g. needs shaping cannot reach the configured policy levels, given the queue length is too small.

Quoted 15 lines · Jesper Dangaard Brouer

The flag IFF_NO_QUEUE marks virtual device drivers that doesn't need a default qdisc attached, given they will be backed by physical device, that already have a qdisc attached for pushback.

It is still supported to attach a qdisc to a IFF_NO_QUEUE device, as this can be useful for difference policy reasons (e.g. bandwidth limiting containers). For this to work, the tx_queue_len need to have a sane value, because some qdiscs inherit/copy the tx_queue_len (namely, pfifo, bfifo, gred, htb, plug and sfb).

Commit a813104d9233 ("IFF_NO_QUEUE: Fix for drivers not calling ether_setup()") caught situations where some drivers didn't initialize tx_queue_len. The problem with the commit was choosing 1 as the fallback value.

Quoted 6 lines

A qdisc queue length of 1 causes more harm than good, because it creates hard to debug situations for userspace. It gives userspace a false sense of a working config after attaching a qdisc. As low volume traffic (that doesn't activate the qdisc policy) works, like ping, while traffic that e.g. needs shaping cannot reach the configured policy levels, given the queue length is too small.

Thanks for fixing this. I've run into this in the exact scenario you describe -- bandwith limiting containers. I'm pretty sure my vote doesn't count, but I'm in favor of this change.

net/sched/sch_api.c +0 -0

index 206dc24add3a..f337f1bdd1d4 100644
@@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 
 	sch->handle = handle;
 

Hi,

On Thu, Nov 03, 2016 at 02:56:11PM +0100, Jesper Dangaard Brouer wrote:
[...]

Quoted 8 lines

net/sched/sch_api.c +0 -0

index 206dc24add3a..f337f1bdd1d4 100644
@@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 
 	sch->handle = handle;
 

net/sched/sch_api.c static struct Qdisc *qdisc_create(struct net_device *dev, · line 972

Quoted 10 lines

net/sched/sch_api.c +10 -0

+	/* This exist to keep backward compatible with a userspace
+	 * loophole, what allowed userspace to get IFF_NO_QUEUE
+	 * facility on older kernels by setting tx_queue_len=0 (prior
+	 * to qdisc init), and then forgot to reinit tx_queue_len
+	 * before again attaching a qdisc.
+	 */
+	if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
+		dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
+		netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
+	}  

I wonder why this is limited to IFF_NO_QUEUE devices. Do you think there is a valid use case for physical ones?

Also, if we sanitize here, couldn't we then just get rid of the sanitization you're fixing in patch 2?

Apart from that, ACK to all the patches. Thanks for cleaning up my mess! :)

Quoted 12 lines · On Fri, 4 Nov 2016 10:35:26 +0100 Phil Sutter <[email protected]> wrote:

Hi,

On Thu, Nov 03, 2016 at 02:56:11PM +0100, Jesper Dangaard Brouer wrote:
[...]

Quoted 8 lines

net/sched/sch_api.c +0 -0

index 206dc24add3a..f337f1bdd1d4 100644
@@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 
 	sch->handle = handle;
 

Quoted 13 lines

net/sched/sch_api.c static struct Qdisc *qdisc_create(struct net_device *dev, · line 972

Quoted 10 lines

net/sched/sch_api.c +10 -0

+	/* This exist to keep backward compatible with a userspace
+	 * loophole, what allowed userspace to get IFF_NO_QUEUE
+	 * facility on older kernels by setting tx_queue_len=0 (prior
+	 * to qdisc init), and then forgot to reinit tx_queue_len
+	 * before again attaching a qdisc.
+	 */
+	if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
+		dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
+		netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
+	}  

I wonder why this is limited to IFF_NO_QUEUE devices. Do you think there is a valid use case for physical ones?

Hmmm, I cannot come up with a useful use-case for physical devices, but I cannot see why we should save users that had used the loophole on physical devices, as that is clearly a faulty config to begin with. See net_crit_ratelimited warning here:
https://github.com/torvalds/linux/blob/v4.9-rc3/net/core/dev.c#L3403

Quoted 2 lines

Also, if we sanitize here, couldn't we then just get rid of the sanitization you're fixing in patch 2?

Without patch 2, then some IFF_NO_QUEUE devices would have a visible tx_queue_len 0 (e.g. the ones not calling ether_setup()), and that would be inconsistent (visible from userspace).

Quoted 29 lines · Jesper Dangaard Brouer

Quoted 25 lines · On Fri, 4 Nov 2016 10:35:26 +0100 Phil Sutter <[email protected]> wrote:

Hi,

On Thu, Nov 03, 2016 at 02:56:11PM +0100, Jesper Dangaard Brouer wrote:
[...]

Quoted 8 lines

net/sched/sch_api.c +0 -0

index 206dc24add3a..f337f1bdd1d4 100644
@@ -960,6 +960,17 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
 
 	sch->handle = handle;
 

net/sched/sch_api.c static struct Qdisc *qdisc_create(struct net_device *dev, · line 972

Quoted 10 lines

net/sched/sch_api.c +10 -0

+	/* This exist to keep backward compatible with a userspace
+	 * loophole, what allowed userspace to get IFF_NO_QUEUE
+	 * facility on older kernels by setting tx_queue_len=0 (prior
+	 * to qdisc init), and then forgot to reinit tx_queue_len
+	 * before again attaching a qdisc.
+	 */
+	if ((dev->priv_flags & IFF_NO_QUEUE) && (dev->tx_queue_len == 0)) {
+		dev->tx_queue_len = DEFAULT_TX_QUEUE_LEN;
+		netdev_info(dev, "Caught tx_queue_len zero misconfig\n");
+	}    

I wonder why this is limited to IFF_NO_QUEUE devices. Do you think there is a valid use case for physical ones?

Quoted 5 lines

Hmmm, I cannot come up with a useful use-case for physical devices, but I cannot see why we should save users that had used the loophole on physical devices, as that is clearly a faulty config to begin with. See net_crit_ratelimited warning here:
[1] https://github.com/torvalds/linux/blob/v4.9-rc3/net/core/dev.c#L3403

I really feel like nit-picking again,

but what differs in between
loophole users of virtual devices (whose broken scripts stopped working) and loophole users of physical devices (whose broken scripts stopped working as well)?

It is a clear misconfiguration to attach a qdisc to a device with tx_queue_len zero, because some qdisc's (namely, pfifo, bfifo, gred, htb, plug and sfb) inherit/copy this value as their queue length.

Why should the kernel catch such a misconfiguration? Because prior to introducing the IFF_NO_QUEUE device flag, userspace found a loophole in the qdisc config system that allowed them to achieve the equivalent of IFF_NO_QUEUE, which is to remove the qdisc code path entirely from a device. The loophole on older kernels is setting tx_queue_len=0, *prior* to device qdisc init (the config time is significant, simply setting tx_queue_len=0 doesn't trigger the loophole).

This loophole is currently used by Docker[1] to get better performance and scalability out of the veth device. The Docker developers were warned[1] that they needed to adjust the tx_queue_len if ever attaching a qdisc. The OpenShift project didn't remember this warning and attached a qdisc, this were caught and fixed in[2].

[1] https://github.com/docker/libcontainer/pull/193
[2] https://github.com/openshift/origin/pull/11126

Instead of fixing every userspace program that used this loophole, and forgot to reset the tx_queue_len, prior to attaching a qdisc. Let's catch the misconfiguration on the kernel side.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
 net/sched/sch_api.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

net/sched/sch_api.c +0 -0

index 206dc24add3a..f337f1bdd1d4 100644

From what I fuzzily recall, HTB with txquelelen == 0 drops traffic

Just FYI:

I'm tangentially aware of internal Google code that:
- expects a bonding device running HTB with non-zero txqueuelen
- wants to remove HTB and get a noqueue interface (the normal default for bonding)

The code currently removes HTB, which gets us to mq, sets txqueuelen to 0, adds a pfifo, removes the pfifo, which gets us to noqueue.

After this patch this would ?possibly? break (adding pfifo, would change txqueuelen, so when we remove it we wouldn't end up with noqueue).

From what I fuzzily recall, HTB with txquelelen == 0 drops traffic hard, while pfifo continues to function, hence the ordering...

Obviously our code can be fixed, but I'm worried there's a more generic backwards compatibility problem here.

(note: this is mostly about 3.11 and 4.3 and might no longer be relevant with 4.10... maybe the new kernel's default qdisc selection logic doesn't depend on txqueuelen and checks the flag instead???)

This patchset is a cleanup for IFF_NO_QUEUE devices. It will hopefully help userspace get a more consistent behavior when attaching qdisc to such virtual devices.

From: Jesper Dangaard Brouer <[email protected]>
Date: Thu, 03 Nov 2016 14:55:56 +0100

Quoted 3 lines

This patchset is a cleanup for IFF_NO_QUEUE devices. It will hopefully help userspace get a more consistent behavior when attaching qdisc to such virtual devices.

I'm still thinking about this.

My reservation about this is basically since the one known offender in userspace acknowledged that what it was doing wrong, and fixed it quickly already, I see no reason to explicitly accomodate this.

This patchset is a cleanup for IFF_NO_QUEUE devices. It will hopefully help userspace get a more consistent behavior when attaching qdisc to such virtual devices.