[PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Phil Sutter]

This series adds a new private net_device flag indicating that a device may
(and probably should) be used without a queueing discipline attached to it.
This is already common practice for many virtual device types like e.g.
loopback, VLAN (802.1Q) or bridges (802.1D). The reason for this is that these
devices lack an underlying layer which could impose back pressure and therefore
making a TX queue necessary to not slow down senders.

Up to now, drivers being aware of the above applying to them set
dev->tx_queue_len to zero to indicate no qdisc should be attached to the
interface they drive and the kernel reacts upon this by assigning the noop
qdisc instead of the default pfifo_fast. This implicit agreement though leads
to an inconvenient situation once a user tries to attach a real qdisc to these
devices, as the formerly special tx_queue_len value becomes a regular one,
limiting the queue to zero packets and thus prevents any TX from happening. To
overcome this, practically all qdisc implementations intercept and sanitize the
malicious value.

With this series applied, drivers may signal the lack of need for a qdisc
without having to tamper with tx_queue_len, making fallbacks in qdiscs and
caveats in userspace unnecessary.

Upon upstream acceptance, this series will be followed up by a set of patches
converting device drivers, adding a warning so out-of-tree driver authors get
aware of this change and dropping all special handling of tx_queue_len in
net/sched/.

Phil Sutter (2):
  net: declare new net_device priv_flag IFF_NO_QUEUE
  net: sch_generic: react upon IFF_NO_QUEUE flag

 include/linux/netdevice.h | 3 +++
 net/sched/sch_generic.c   | 6 ++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

-- 
2.1.2

[PATCH 1/2] net: declare new net_device priv_flag IFF_NO_QUEUE

[Phil Sutter]

This private net_device flag can be set by drivers to inform that a
device runs fine without a qdisc attached. This was formerly done by
setting tx_queue_len to zero.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/linux/netdevice.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 607b5f4..7ed6fb0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1262,6 +1262,7 @@ struct net_device_ops {
  * @IFF_LIVE_ADDR_CHANGE: device supports hardware address
  *	change when it's running
  * @IFF_MACVLAN: Macvlan device
+ * @IFF_NO_QUEUE: device can run without qdisc attached
  */
 enum netdev_priv_flags {
 	IFF_802_1Q_VLAN			= 1<<0,
@@ -1289,6 +1290,7 @@ enum netdev_priv_flags {
 	IFF_XMIT_DST_RELEASE_PERM	= 1<<22,
 	IFF_IPVLAN_MASTER		= 1<<23,
 	IFF_IPVLAN_SLAVE		= 1<<24,
+	IFF_NO_QUEUE			= 1<<25,
 };
 
 #define IFF_802_1Q_VLAN			IFF_802_1Q_VLAN
@@ -1316,6 +1318,7 @@ enum netdev_priv_flags {
 #define IFF_XMIT_DST_RELEASE_PERM	IFF_XMIT_DST_RELEASE_PERM
 #define IFF_IPVLAN_MASTER		IFF_IPVLAN_MASTER
 #define IFF_IPVLAN_SLAVE		IFF_IPVLAN_SLAVE
+#define IFF_NO_QUEUE			IFF_NO_QUEUE
 
 /**
  *	struct net_device - The DEVICE structure.
-- 
2.1.2

[PATCH 2/2] net: sch_generic: react upon IFF_NO_QUEUE flag

[Phil Sutter]

Handle IFF_NO_QUEUE as alternative to tx_queue_len being zero.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/sched/sch_generic.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 6efca30..942fea8 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -735,7 +735,7 @@ static void attach_one_default_qdisc(struct net_device *dev,
 {
 	struct Qdisc *qdisc = &noqueue_qdisc;
 
-	if (dev->tx_queue_len) {
+	if (dev->tx_queue_len && !(dev->priv_flags & IFF_NO_QUEUE)) {
 		qdisc = qdisc_create_dflt(dev_queue,
 					  default_qdisc_ops, TC_H_ROOT);
 		if (!qdisc) {
@@ -755,7 +755,9 @@ static void attach_default_qdiscs(struct net_device *dev)
 
 	txq = netdev_get_tx_queue(dev, 0);
 
-	if (!netif_is_multiqueue(dev) || dev->tx_queue_len == 0) {
+	if (!netif_is_multiqueue(dev) ||
+	    dev->tx_queue_len == 0 ||
+	    dev->priv_flags & IFF_NO_QUEUE) {
 		netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL);
 		dev->qdisc = txq->qdisc_sleeping;
 		atomic_inc(&dev->qdisc->refcnt);
-- 
2.1.2

Re: [PATCH 2/2] net: sch_generic: react upon IFF_NO_QUEUE flag

[Jesper Dangaard Brouer]

On Thu, 13 Aug 2015 19:01:07 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Handle IFF_NO_QUEUE as alternative to tx_queue_len being zero.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Re: [PATCH 1/2] net: declare new net_device priv_flag IFF_NO_QUEUE

[Jesper Dangaard Brouer]

On Thu, 13 Aug 2015 19:01:06 +0200
Phil Sutter <phil@nwl.cc> wrote:

> This private net_device flag can be set by drivers to inform that a
> device runs fine without a qdisc attached. This was formerly done by
> setting tx_queue_len to zero.
> 
> Signed-off-by: Phil Sutter <phil@nwl.cc>

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Stephen Hemminger]

On Thu, 13 Aug 2015 19:01:05 +0200
Phil Sutter <phil@nwl.cc> wrote:

> Up to now, drivers being aware of the above applying to them set
> dev->tx_queue_len to zero to indicate no qdisc should be attached to the
> interface they drive and the kernel reacts upon this by assigning the noop
> qdisc instead of the default pfifo_fast. This implicit agreement though leads
> to an inconvenient situation once a user tries to attach a real qdisc to these
> devices, as the formerly special tx_queue_len value becomes a regular one,

So this is a workaround for user ignorance by introducing kernel API complexity.
Before user sets qdisc, why don't they set tx queue length?

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Jesper Dangaard Brouer]

On Thu, 13 Aug 2015 10:49:50 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Thu, 13 Aug 2015 19:01:05 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > Up to now, drivers being aware of the above applying to them set
> > dev->tx_queue_len to zero to indicate no qdisc should be attached to the
> > interface they drive and the kernel reacts upon this by assigning the noop
> > qdisc instead of the default pfifo_fast. This implicit agreement though leads
> > to an inconvenient situation once a user tries to attach a real qdisc to these
> > devices, as the formerly special tx_queue_len value becomes a regular one,
> 
> So this is a workaround for user ignorance by introducing kernel API complexity.
> Before user sets qdisc, why don't they set tx queue length?

Please don't insist on keeping this broke interface... how should users
know that BEFORE adding a qdisc they MUST change the _device_ tx queue
length (not zero).  Getting "back" to the original state, they MUST
change the device tx queue len back to zero BEFORE deleting the qdisc,
such that when assigning the default queue qdisc the system detects
this device can work without a qdisc.  Changing the tx queue len to
zero after the qdisc is deleted will have not effect. 

Listen to the description, that interface is broken. The kernel really
needs to hide these details from userspace.

It even allows you to misconfigure the kernel, by tricking the kernel
into assigning noqueue to physical devices that really need it.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Stephen Hemminger]

On Thu, 13 Aug 2015 20:40:37 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Thu, 13 Aug 2015 10:49:50 -0700
> Stephen Hemminger <stephen@networkplumber.org> wrote:
> 
> > On Thu, 13 Aug 2015 19:01:05 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> > 
> > > Up to now, drivers being aware of the above applying to them set
> > > dev->tx_queue_len to zero to indicate no qdisc should be attached to the
> > > interface they drive and the kernel reacts upon this by assigning the noop
> > > qdisc instead of the default pfifo_fast. This implicit agreement though leads
> > > to an inconvenient situation once a user tries to attach a real qdisc to these
> > > devices, as the formerly special tx_queue_len value becomes a regular one,
> > 
> > So this is a workaround for user ignorance by introducing kernel API complexity.
> > Before user sets qdisc, why don't they set tx queue length?
> 
> Please don't insist on keeping this broke interface... how should users
> know that BEFORE adding a qdisc they MUST change the _device_ tx queue
> length (not zero).

Before setting any qdisc, they should set queue length anyway.

>  Getting "back" to the original state, they MUST
> change the device tx queue len back to zero BEFORE deleting the qdisc,
> such that when assigning the default queue qdisc the system detects
> this device can work without a qdisc.  Changing the tx queue len to
> zero after the qdisc is deleted will have not effect. 
> 
> Listen to the description, that interface is broken. The kernel really
> needs to hide these details from userspace.
> 
> It even allows you to misconfigure the kernel, by tricking the kernel
> into assigning noqueue to physical devices that really need it.

But adding a flag risks breaking external scripts.

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Phil Sutter]

On Thu, Aug 13, 2015 at 12:11:57PM -0700, Stephen Hemminger wrote:
> On Thu, 13 Aug 2015 20:40:37 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> 
> > On Thu, 13 Aug 2015 10:49:50 -0700
> > Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > > On Thu, 13 Aug 2015 19:01:05 +0200
> > > Phil Sutter <phil@nwl.cc> wrote:
> > > 
> > > > Up to now, drivers being aware of the above applying to them set
> > > > dev->tx_queue_len to zero to indicate no qdisc should be attached to the
> > > > interface they drive and the kernel reacts upon this by assigning the noop
> > > > qdisc instead of the default pfifo_fast. This implicit agreement though leads
> > > > to an inconvenient situation once a user tries to attach a real qdisc to these
> > > > devices, as the formerly special tx_queue_len value becomes a regular one,
> > > 
> > > So this is a workaround for user ignorance by introducing kernel API complexity.
> > > Before user sets qdisc, why don't they set tx queue length?
> > 
> > Please don't insist on keeping this broke interface... how should users
> > know that BEFORE adding a qdisc they MUST change the _device_ tx queue
> > length (not zero).
> 
> Before setting any qdisc, they should set queue length anyway.

Probably, yes. But if they don't, it depends on the interface driver
whether they're screwed or not. In my opinion, this inconsistency alone
is worth fixing.

> >  Getting "back" to the original state, they MUST
> > change the device tx queue len back to zero BEFORE deleting the qdisc,
> > such that when assigning the default queue qdisc the system detects
> > this device can work without a qdisc.  Changing the tx queue len to
> > zero after the qdisc is deleted will have not effect. 
> > 
> > Listen to the description, that interface is broken. The kernel really
> > needs to hide these details from userspace.
> > 
> > It even allows you to misconfigure the kernel, by tricking the kernel
> > into assigning noqueue to physical devices that really need it.
> 
> But adding a flag risks breaking external scripts.

Could you please elaborate on this? As far as I can tell, introducing a
separate flag is the only solution *not* breaking existing scripts. So
if you see the rub, I would like to know where exactly it is.

Cheers, Phil

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Jesper Dangaard Brouer]

On Fri, 14 Aug 2015 10:41:53 +0200 Phil Sutter <phil@nwl.cc> wrote:

> On Thu, Aug 13, 2015 at 12:11:57PM -0700, Stephen Hemminger wrote:
[...]
> > 
> > But adding a flag risks breaking external scripts.
> 
> Could you please elaborate on this? As far as I can tell, introducing a
> separate flag is the only solution *not* breaking existing scripts. So
> if you see the rub, I would like to know where exactly it is.

I agree with Phil.  AFAIC see this approach does not break existing
scripts.

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Eric Dumazet]

On Mon, 2015-08-17 at 08:51 +0200, Jesper Dangaard Brouer wrote:
> On Fri, 14 Aug 2015 10:41:53 +0200 Phil Sutter <phil@nwl.cc> wrote:
> 
> > On Thu, Aug 13, 2015 at 12:11:57PM -0700, Stephen Hemminger wrote:
> [...]
> > > 
> > > But adding a flag risks breaking external scripts.
> > 
> > Could you please elaborate on this? As far as I can tell, introducing a
> > separate flag is the only solution *not* breaking existing scripts. So
> > if you see the rub, I would like to know where exactly it is.
> 
> I agree with Phil.  AFAIC see this approach does not break existing
> scripts.
> 
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> 

But, what is the long term plan ?

If long term plan is to change veth txqueuelen to 0, we said no.
(because it is too late and this change will break some setups)

If not, this flag wont help the case you want to optimize anyway.

(ie : veth with no qdisc)

So really, _new_ user scripts should either: make sure a qdisc is not
there, or is there.

Relying on a new flag wont help.

There is no point adding this flag as such if we do not take proper
decisions.

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[Jesper Dangaard Brouer]

On Mon, 17 Aug 2015 06:44:51 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Mon, 2015-08-17 at 08:51 +0200, Jesper Dangaard Brouer wrote:
> > On Fri, 14 Aug 2015 10:41:53 +0200 Phil Sutter <phil@nwl.cc> wrote:
> > 
> > > On Thu, Aug 13, 2015 at 12:11:57PM -0700, Stephen Hemminger wrote:
> > [...]
> > > > 
> > > > But adding a flag risks breaking external scripts.
> > > 
> > > Could you please elaborate on this? As far as I can tell, introducing a
> > > separate flag is the only solution *not* breaking existing scripts. So
> > > if you see the rub, I would like to know where exactly it is.
> > 
> > I agree with Phil.  AFAIC see this approach does not break existing
> > scripts.
> > 
> > Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > 
> 
> But, what is the long term plan ?
> 
> If long term plan is to change veth txqueuelen to 0, we said no.
> (because it is too late and this change will break some setups)

No, veth keep txqueuelen at the default 1000.  We simply add the flag
IFF_NO_QUEUE to veth. 

That result in (1) veth gets no-qdisc when using the default qdisc, and
(2) if anyone assigns another qdisc it still works.  This is backward
compatible with existing scripts.

Long term plan is to change e.g. vlan to be marked with IFF_NO_QUEUE
and don't set txqueuelen to zero (automatic falls back to default
setting).  This avoids any gotchas for users assigning a qdisc. This is
also backward compatible, with users who know to change the txqueuelen
before assigning a qdisc to a vlan.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 0/2] net: introduce IFF_NO_QUEUE as successor of zero tx_queue_len

[David Miller]

From: Phil Sutter <phil@nwl.cc>
Date: Thu, 13 Aug 2015 19:01:05 +0200

> This series adds a new private net_device flag indicating that a device may
> (and probably should) be used without a queueing discipline attached to it.
> This is already common practice for many virtual device types like e.g.
> loopback, VLAN (802.1Q) or bridges (802.1D). The reason for this is that these
> devices lack an underlying layer which could impose back pressure and therefore
> making a TX queue necessary to not slow down senders.
> 
> Up to now, drivers being aware of the above applying to them set
> dev->tx_queue_len to zero to indicate no qdisc should be attached to the
> interface they drive and the kernel reacts upon this by assigning the noop
> qdisc instead of the default pfifo_fast. This implicit agreement though leads
> to an inconvenient situation once a user tries to attach a real qdisc to these
> devices, as the formerly special tx_queue_len value becomes a regular one,
> limiting the queue to zero packets and thus prevents any TX from happening. To
> overcome this, practically all qdisc implementations intercept and sanitize the
> malicious value.
> 
> With this series applied, drivers may signal the lack of need for a qdisc
> without having to tamper with tx_queue_len, making fallbacks in qdiscs and
> caveats in userspace unnecessary.
> 
> Upon upstream acceptance, this series will be followed up by a set of patches
> converting device drivers, adding a warning so out-of-tree driver authors get
> aware of this change and dropping all special handling of tx_queue_len in
> net/sched/.

Series applied, thanks.

Since the VRF changes went in right before this, I had to bump the IFF_NO_QUEUE
value to use bit 26 instead of bit 25.