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

[net-next PATCH 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.

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   | 12 ++++++++----
 3 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.1.2

[net-next PATCH 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 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   | 5 +++++
 3 files changed, 8 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..ab614ee 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -634,6 +634,11 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
 	if (IS_ERR(sch))
 		goto errout;
 	sch->parent = parentid;
+#ifdef CONFIG_NET_SCHED
+	sch->handle = qdisc_alloc_handle(dev_queue->dev);
+	if (!sch->handle)
+		goto errout;
+#endif
 
 	if (!ops->init || ops->init(sch, NULL) == 0)
 		return sch;
-- 
2.1.2

[net-next PATCH 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 ab614ee..556de30 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -723,9 +723,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 2/3] net: sched: allocate a handle to default qdiscs

[Eric Dumazet]

On Fri, 2015-08-21 at 17:58 +0200, Phil Sutter wrote:
> 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   | 5 +++++
>  3 files changed, 8 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..ab614ee 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -634,6 +634,11 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
>  	if (IS_ERR(sch))
>  		goto errout;
>  	sch->parent = parentid;
> +#ifdef CONFIG_NET_SCHED
> +	sch->handle = qdisc_alloc_handle(dev_queue->dev);
> +	if (!sch->handle)
> +		goto errout;
> +#endif
>  
>  	if (!ops->init || ops->init(sch, NULL) == 0)
>  		return sch;

This might break HTB setups with more than 32768 classes ?

The pfifo qdisc that gets attached had no handle.

qdisc_alloc_handle() has a limited range.

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

[Phil Sutter]

On Fri, Aug 21, 2015 at 09:14:58AM -0700, Eric Dumazet wrote:
[...]
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 1fb65f9..ab614ee 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -634,6 +634,11 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue,
> >  	if (IS_ERR(sch))
> >  		goto errout;
> >  	sch->parent = parentid;
> > +#ifdef CONFIG_NET_SCHED
> > +	sch->handle = qdisc_alloc_handle(dev_queue->dev);
> > +	if (!sch->handle)
> > +		goto errout;
> > +#endif
> >  
> >  	if (!ops->init || ops->init(sch, NULL) == 0)
> >  		return sch;
> 
> This might break HTB setups with more than 32768 classes ?

Urgh. Thanks for noticing this!

> The pfifo qdisc that gets attached had no handle.

Yes, looks like I need to leave qdisc_create_dflt() alone. It is
possible, by doing the above twice in sch_generic.c (once in
attach_one_default_qdisc(), and in attach_default_qdiscs() as well).

> qdisc_alloc_handle() has a limited range.

Yes, I noticed this. Handles aren't reused in a running system either,
which might contribute to this problem in other situations.

V2 will follow, thanks again.

Cheers, Phil