[net-next PATCH v2 0/3] net: sched: allow switching qdisc to noqueue intuitively

[net-next PATCH v2 0/3] net: sched: allow switching qdisc to noqueue intuitively

[Phil Sutter]

This patch series improves the integration of the noqueue qdisc to become the
fallback queueing if no other is attached to an interface. Before it was rather
an add-on, a simpler alternative to a FIFO if no congestion is expected or
possible. It has become the default qdisc for virtual interfaces, and could be
attached by this mechanism only (through removing the root qdisc after having
set tx_queue_len to zero for interfaces not defaulting to noqueue otherwise).

This series does not change the default qdisc chosen for new interfaces, but
upon removal of the root qdisc from an interface, the kernel won't fall back to
the default but to noqueue instead.

Changes since v1:
- Leave qdisc_create_dflt() alone as it is used in sch_htb.c as well. Instead
  allocate the handle in attach_default_qdiscs() and
  attach_one_default_qdisc().

Phil Sutter (3):
  net: sched: make noqueue_qdisc non-static
  net: sched: allocate a handle to default qdiscs
  net: sched: fall back to noqueue when removing root qdisc

 include/net/sch_generic.h |  2 ++
 net/sched/sch_api.c       |  5 +++--
 net/sched/sch_generic.c   | 18 ++++++++++++++----
 3 files changed, 19 insertions(+), 6 deletions(-)

-- 
2.1.2

[net-next PATCH v2 1/3] net: sched: make noqueue_qdisc non-static

[Phil Sutter]

This needs to be referenced from within net/sched/sched_api.c later.

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

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 2eab08c..4495193 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -337,6 +337,7 @@ static inline void sch_tree_unlock(const struct Qdisc *q)
 #define tcf_tree_unlock(tp)	sch_tree_unlock((tp)->q)
 
 extern struct Qdisc noop_qdisc;
+extern struct Qdisc noqueue_qdisc;
 extern struct Qdisc_ops noop_qdisc_ops;
 extern struct Qdisc_ops pfifo_fast_ops;
 extern struct Qdisc_ops mq_qdisc_ops;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 942fea8..1fb65f9 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -425,13 +425,12 @@ static struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
 	.owner		=	THIS_MODULE,
 };
 
-static struct Qdisc noqueue_qdisc;
 static struct netdev_queue noqueue_netdev_queue = {
 	.qdisc		=	&noqueue_qdisc,
 	.qdisc_sleeping	=	&noqueue_qdisc,
 };
 
-static struct Qdisc noqueue_qdisc = {
+struct Qdisc noqueue_qdisc = {
 	.enqueue	=	NULL,
 	.dequeue	=	noop_dequeue,
 	.flags		=	TCQ_F_BUILTIN,
-- 
2.1.2

[net-next PATCH v2 2/3] net: sched: allocate a handle to default qdiscs

[Phil Sutter]

Since tc_get_qdisc() does not allow to remove a qdisc with zero handle,
a handle needs to be allocated to default qdiscs (currently pfifo_fast
or mq) in order to allow removing them.

Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 include/net/sch_generic.h |  1 +
 net/sched/sch_api.c       |  3 ++-
 net/sched/sch_generic.c   | 11 +++++++++++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 4495193..2bfc898 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -391,6 +391,7 @@ void dev_deactivate(struct net_device *dev);
 void dev_deactivate_many(struct list_head *head);
 struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
 			      struct Qdisc *qdisc);
+u32 qdisc_alloc_handle(struct net_device *dev);
 void qdisc_reset(struct Qdisc *qdisc);
 void qdisc_destroy(struct Qdisc *qdisc);
 void qdisc_tree_decrease_qlen(struct Qdisc *qdisc, unsigned int n);
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f06aa01..224374c 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -723,7 +723,7 @@ EXPORT_SYMBOL(qdisc_class_hash_remove);
 /* Allocate an unique handle from space managed by kernel
  * Possible range is [8000-FFFF]:0000 (0x8000 values)
  */
-static u32 qdisc_alloc_handle(struct net_device *dev)
+u32 qdisc_alloc_handle(struct net_device *dev)
 {
 	int i = 0x8000;
 	static u32 autohandle = TC_H_MAKE(0x80000000U, 0);
@@ -739,6 +739,7 @@ static u32 qdisc_alloc_handle(struct net_device *dev)
 
 	return 0;
 }
+EXPORT_SYMBOL(qdisc_alloc_handle);
 
 void qdisc_tree_decrease_qlen(struct Qdisc *sch, unsigned int n)
 {
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 1fb65f9..68df721 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -741,6 +741,11 @@ static void attach_one_default_qdisc(struct net_device *dev,
 			netdev_info(dev, "activation failed\n");
 			return;
 		}
+#ifdef CONFIG_NET_SCHED
+		qdisc->handle = qdisc_alloc_handle(dev);
+		if (!qdisc->handle)
+			netdev_info(dev, "qdisc handle allocation failed\n");
+#endif
 		if (!netif_is_multiqueue(dev))
 			qdisc->flags |= TCQ_F_ONETXQUEUE;
 	}
@@ -763,6 +768,12 @@ static void attach_default_qdiscs(struct net_device *dev)
 	} else {
 		qdisc = qdisc_create_dflt(txq, &mq_qdisc_ops, TC_H_ROOT);
 		if (qdisc) {
+#ifdef CONFIG_NET_SCHED
+			qdisc->handle = qdisc_alloc_handle(dev);
+			if (!qdisc->handle)
+				netdev_info(dev,
+				            "qdisc handle allocation failed\n");
+#endif
 			dev->qdisc = qdisc;
 			qdisc->ops->attach(qdisc);
 		}
-- 
2.1.2

[net-next PATCH v2 3/3] net: sched: fall back to noqueue when removing root qdisc

[Phil Sutter]

When removing the root qdisc, the interface should fall back to noqueue
as the 'real' minimal qdisc instead of the default one. Therefore
dev_graft_qdisc() has to be adjusted to assign noqueue if NULL was
passed as new qdisc, and qdisc_graft() needs to assign noqueue to
dev->qdisc instead of noop to prevent dev_activate() from attaching
default qdiscs to the interface.

Note that it is also necessary to have dev_graft_qdisc() set
dev_queue->qdisc to the new qdisc instead of (unconditionally) noop. I
don't know why this was there at all (originates from pre-git time), but
it seems wrong to me.  It could be worked around by droping the extra
check for noqueue in transition_one_qdisc(), maybe with unintended
side-effects.

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

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 224374c..3b2cf30 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -839,7 +839,7 @@ skip:
 					   dev->qdisc, new);
 			if (new && !new->ops->attach)
 				atomic_inc(&new->refcnt);
-			dev->qdisc = new ? : &noop_qdisc;
+			dev->qdisc = new ? : &noqueue_qdisc;
 
 			if (new && new->ops->attach)
 				new->ops->attach(new);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 68df721..ecc369b 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -718,9 +718,9 @@ struct Qdisc *dev_graft_qdisc(struct netdev_queue *dev_queue,
 
 	/* ... and graft new one */
 	if (qdisc == NULL)
-		qdisc = &noop_qdisc;
+		qdisc = &noqueue_qdisc;
 	dev_queue->qdisc_sleeping = qdisc;
-	rcu_assign_pointer(dev_queue->qdisc, &noop_qdisc);
+	rcu_assign_pointer(dev_queue->qdisc, qdisc);
 
 	spin_unlock_bh(root_lock);
 
-- 
2.1.2

Re: [net-next PATCH v2 3/3] net: sched: fall back to noqueue when removing root qdisc

[Jesper Dangaard Brouer]

On Sat, 22 Aug 2015 02:20:56 +0200
Phil Sutter <phil@nwl.cc> wrote:

> When removing the root qdisc, the interface should fall back to noqueue
> as the 'real' minimal qdisc instead of the default one. 

I worry this behavior could break existing scripts.

I prefer the idea of allowing tc command to assign noqueue (to any
device).  This makes the action explicit for the user, instead of being
a side-effect of removing a qdisc. (and does not break backward compat)

-- 
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: [net-next PATCH v2 3/3] net: sched: fall back to noqueue when removing root qdisc

[Jesper Dangaard Brouer]

On Sun, 23 Aug 2015 20:44:42 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Sat, 22 Aug 2015 02:20:56 +0200
> Phil Sutter <phil@nwl.cc> wrote:
> 
> > When removing the root qdisc, the interface should fall back to noqueue
> > as the 'real' minimal qdisc instead of the default one. 
> 
> I worry this behavior could break existing scripts.

You would break OpenWRT package "qos-scripts", specifically:
 https://github.com/openwrt-mirror/openwrt/blob/master/package/network/config/qos-scripts/files/usr/bin/qos-stop

Which cleans-up/clear the qdisc setup by removing the root qdisc,
assuming and depending on the default qdisc is re-assigned.


> I prefer the idea of allowing tc command to assign noqueue (to any
> device).  This makes the action explicit for the user, instead of being
> a side-effect of removing a qdisc. (and does not break backward compat)


-- 
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: [net-next PATCH v2 3/3] net: sched: fall back to noqueue when removing root qdisc

[Phil Sutter]

Hi Jesper,

On Sun, Aug 23, 2015 at 08:53:57PM +0200, Jesper Dangaard Brouer wrote:
> On Sun, 23 Aug 2015 20:44:42 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> 
> > On Sat, 22 Aug 2015 02:20:56 +0200
> > Phil Sutter <phil@nwl.cc> wrote:
> > 
> > > When removing the root qdisc, the interface should fall back to noqueue
> > > as the 'real' minimal qdisc instead of the default one. 
> > 
> > I worry this behavior could break existing scripts.
> 
> You would break OpenWRT package "qos-scripts", specifically:
>  https://github.com/openwrt-mirror/openwrt/blob/master/package/network/config/qos-scripts/files/usr/bin/qos-stop

Thanks for pointing this out!

> Which cleans-up/clear the qdisc setup by removing the root qdisc,
> assuming and depending on the default qdisc is re-assigned.

OK. Since the premise of the whole thing is to not break existing
scripts, this sadly tears down my approach.

> > I prefer the idea of allowing tc command to assign noqueue (to any
> > device).  This makes the action explicit for the user, instead of being
> > a side-effect of removing a qdisc. (and does not break backward compat)

I will give this another go. What I didn't like was that after attaching
noqueue, tc would output nothing when asked to show the attached qdisc -
which is of debatable correctness at least. But maybe that's just a user
space problem I could address separately.

Cheers, Phil