[net-next PATCH 0/4] fixup IFF_NO_QUEUE conversion

[net-next PATCH 0/4] fixup IFF_NO_QUEUE conversion

[Phil Sutter]

This series serves two purposes:

On one hand it fixes a quite embarrassing bug around the warning I added for
drivers still setting tx_queue_len = 0 to achieve noqueue operation. It turned
out to be quite useless as due to using alloc_netdev(), many in-kernel drivers
fell into the trap by accident, as well. Instead this place serves pretty well
as a sanitizing point to set IFF_NO_QUEUE for drivers not initializing
tx_queue_len, which in turn allows to drop all special treatment of the latter
being zero since that can not happen anymore without IFF_NO_QUEUE being set.

On the other hand, it provides a better solution for Eric Dumazet's concern
regarding how to assign noqueue to an interface which does not default to it
already. In order to make this possible, noqueue is being registered so users
can 'tc qd add dev eth0 root noqueue'. In addition, it resolves the ugly
situation of 'tc qd show' not showing noqueue. Finally, the former changes
allow for some code cleanup.

Phil Sutter (4):
  net: fix IFF_NO_QUEUE for drivers using alloc_netdev
  net: sched: ignore tx_queue_len when assigning default qdisc
  net: sched: register noqueue qdisc
  net: sched: simplify attach_one_default_qdisc()

 include/net/sch_generic.h |  1 +
 net/core/dev.c            |  2 +-
 net/sched/sch_api.c       |  1 +
 net/sched/sch_generic.c   | 54 ++++++++++++++++++++---------------------------
 4 files changed, 26 insertions(+), 32 deletions(-)

-- 
2.1.2

[net-next PATCH 1/4] net: fix IFF_NO_QUEUE for drivers using alloc_netdev

[Phil Sutter]

Printing a warning in alloc_netdev_mqs() if tx_queue_len is zero and
IFF_NO_QUEUE not set is not appropriate since drivers may use one of the
alloc_netdev* macros instead of alloc_etherdev*, thereby not
intentionally leaving tx_queue_len uninitialized. Instead check here if
tx_queue_len is zero and set IFF_NO_QUEUE, so the value of tx_queue_len
can be ignored in net/sched_generic.c.

Fixes: 906470c ("net: warn if drivers set tx_queue_len = 0")
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index b1f3f48..68156ef 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6998,7 +6998,7 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	setup(dev);
 
 	if (!dev->tx_queue_len)
-		printk(KERN_WARNING "%s uses DEPRECATED zero tx_queue_len - convert driver to use IFF_NO_QUEUE instead.\n", name);
+		dev->priv_flags |= IFF_NO_QUEUE;
 
 	dev->num_tx_queues = txqs;
 	dev->real_num_tx_queues = txqs;
-- 
2.1.2

[net-next PATCH 2/4] net: sched: ignore tx_queue_len when assigning default qdisc

[Phil Sutter]

Since alloc_netdev_mqs() sets IFF_NO_QUEUE for drivers not initializing
tx_queue_len, it is safe to assume that if tx_queue_len is zero,
dev->priv flags always contains IFF_NO_QUEUE.

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

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 942fea8..f501b74 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 && !(dev->priv_flags & IFF_NO_QUEUE)) {
+	if (!(dev->priv_flags & IFF_NO_QUEUE)) {
 		qdisc = qdisc_create_dflt(dev_queue,
 					  default_qdisc_ops, TC_H_ROOT);
 		if (!qdisc) {
@@ -756,7 +756,6 @@ 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 ||
 	    dev->priv_flags & IFF_NO_QUEUE) {
 		netdev_for_each_tx_queue(dev, attach_one_default_qdisc, NULL);
 		dev->qdisc = txq->qdisc_sleeping;
-- 
2.1.2

[net-next PATCH 3/4] net: sched: register noqueue qdisc

[Phil Sutter]

This way users can attach noqueue just like any other qdisc using tc
without having to mess with tx_queue_len first.

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

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 2eab08c..444faa8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -340,6 +340,7 @@ extern struct Qdisc noop_qdisc;
 extern struct Qdisc_ops noop_qdisc_ops;
 extern struct Qdisc_ops pfifo_fast_ops;
 extern struct Qdisc_ops mq_qdisc_ops;
+extern struct Qdisc_ops noqueue_qdisc_ops;
 extern const struct Qdisc_ops *default_qdisc_ops;
 
 struct Qdisc_class_common {
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index f06aa01..6a35551 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -1947,6 +1947,7 @@ static int __init pktsched_init(void)
 	register_qdisc(&bfifo_qdisc_ops);
 	register_qdisc(&pfifo_head_drop_qdisc_ops);
 	register_qdisc(&mq_qdisc_ops);
+	register_qdisc(&noqueue_qdisc_ops);
 
 	rtnl_register(PF_UNSPEC, RTM_NEWQDISC, tc_modify_qdisc, NULL, NULL);
 	rtnl_register(PF_UNSPEC, RTM_DELQDISC, tc_get_qdisc, NULL, NULL);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index f501b74..d5c7c0d 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -416,9 +416,19 @@ struct Qdisc noop_qdisc = {
 };
 EXPORT_SYMBOL(noop_qdisc);
 
-static struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
+static int noqueue_init(struct Qdisc *qdisc, struct nlattr *opt)
+{
+	/* register_qdisc() assigns a default of noop_enqueue if unset,
+	 * but __dev_queue_xmit() treats noqueue only as such
+	 * if this is NULL - so clear it here. */
+	qdisc->enqueue = NULL;
+	return 0;
+}
+
+struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
 	.id		=	"noqueue",
 	.priv_size	=	0,
+	.init		=	noqueue_init,
 	.enqueue	=	noop_enqueue,
 	.dequeue	=	noop_dequeue,
 	.peek		=	noop_dequeue,
-- 
2.1.2

[net-next PATCH 4/4] net: sched: simplify attach_one_default_qdisc()

[Phil Sutter]

Now that noqueue qdisc can be attached just like any other qdisc, no
special treatment is necessary anymore when attaching it as default
qdisc.

This change has the added benefit that 'tc qdisc show' prints noqueue
instead of nothing for devices defaulting to noqueue.

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

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index d5c7c0d..cb5d4ad 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -435,24 +435,6 @@ 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 = {
-	.enqueue	=	NULL,
-	.dequeue	=	noop_dequeue,
-	.flags		=	TCQ_F_BUILTIN,
-	.ops		=	&noqueue_qdisc_ops,
-	.list		=	LIST_HEAD_INIT(noqueue_qdisc.list),
-	.q.lock		=	__SPIN_LOCK_UNLOCKED(noqueue_qdisc.q.lock),
-	.dev_queue	=	&noqueue_netdev_queue,
-	.busylock	=	__SPIN_LOCK_UNLOCKED(noqueue_qdisc.busylock),
-};
-
-
 static const u8 prio2band[TC_PRIO_MAX + 1] = {
 	1, 2, 2, 2, 1, 2, 0, 0 , 1, 1, 1, 1, 1, 1, 1, 1
 };
@@ -743,18 +725,19 @@ static void attach_one_default_qdisc(struct net_device *dev,
 				     struct netdev_queue *dev_queue,
 				     void *_unused)
 {
-	struct Qdisc *qdisc = &noqueue_qdisc;
+	struct Qdisc *qdisc;
+	const struct Qdisc_ops *ops = default_qdisc_ops;
 
-	if (!(dev->priv_flags & IFF_NO_QUEUE)) {
-		qdisc = qdisc_create_dflt(dev_queue,
-					  default_qdisc_ops, TC_H_ROOT);
-		if (!qdisc) {
-			netdev_info(dev, "activation failed\n");
-			return;
-		}
-		if (!netif_is_multiqueue(dev))
-			qdisc->flags |= TCQ_F_ONETXQUEUE;
+	if (dev->priv_flags & IFF_NO_QUEUE)
+		ops = &noqueue_qdisc_ops;
+
+	qdisc = qdisc_create_dflt(dev_queue, ops, TC_H_ROOT);
+	if (!qdisc) {
+		netdev_info(dev, "activation failed\n");
+		return;
 	}
+	if (!netif_is_multiqueue(dev))
+		qdisc->flags |= TCQ_F_ONETXQUEUE;
 	dev_queue->qdisc_sleeping = qdisc;
 }
 
@@ -790,7 +773,7 @@ static void transition_one_qdisc(struct net_device *dev,
 		clear_bit(__QDISC_STATE_DEACTIVATED, &new_qdisc->state);
 
 	rcu_assign_pointer(dev_queue->qdisc, new_qdisc);
-	if (need_watchdog_p && new_qdisc != &noqueue_qdisc) {
+	if (need_watchdog_p) {
 		dev_queue->trans_start = 0;
 		*need_watchdog_p = 1;
 	}
-- 
2.1.2

Re: [net-next PATCH 0/4] fixup IFF_NO_QUEUE conversion

[David Miller]

From: Phil Sutter <phil@nwl.cc>
Date: Thu, 27 Aug 2015 21:21:35 +0200

> This series serves two purposes:
> 
> On one hand it fixes a quite embarrassing bug around the warning I added for
> drivers still setting tx_queue_len = 0 to achieve noqueue operation. It turned
> out to be quite useless as due to using alloc_netdev(), many in-kernel drivers
> fell into the trap by accident, as well. Instead this place serves pretty well
> as a sanitizing point to set IFF_NO_QUEUE for drivers not initializing
> tx_queue_len, which in turn allows to drop all special treatment of the latter
> being zero since that can not happen anymore without IFF_NO_QUEUE being set.
> 
> On the other hand, it provides a better solution for Eric Dumazet's concern
> regarding how to assign noqueue to an interface which does not default to it
> already. In order to make this possible, noqueue is being registered so users
> can 'tc qd add dev eth0 root noqueue'. In addition, it resolves the ugly
> situation of 'tc qd show' not showing noqueue. Finally, the former changes
> allow for some code cleanup.

Seems reasonable, series applied, thanks Phil.

Re: [net-next PATCH 3/4] net: sched: register noqueue qdisc

[Sergei Shtylyov]

Hello.

On 8/27/2015 10:21 PM, Phil Sutter wrote:

> This way users can attach noqueue just like any other qdisc using tc
> without having to mess with tx_queue_len first.
>
> Signed-off-by: Phil Sutter <phil@nwl.cc>
> ---
>   include/net/sch_generic.h |  1 +
>   net/sched/sch_api.c       |  1 +
>   net/sched/sch_generic.c   | 12 +++++++++++-
>   3 files changed, 13 insertions(+), 1 deletion(-)

[...]
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index f501b74..d5c7c0d 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -416,9 +416,19 @@ struct Qdisc noop_qdisc = {
>   };
>   EXPORT_SYMBOL(noop_qdisc);
>
> -static struct Qdisc_ops noqueue_qdisc_ops __read_mostly = {
> +static int noqueue_init(struct Qdisc *qdisc, struct nlattr *opt)
> +{
> +	/* register_qdisc() assigns a default of noop_enqueue if unset,
> +	 * but __dev_queue_xmit() treats noqueue only as such
> +	 * if this is NULL - so clear it here. */

    The multi-line comments in the networking code should follow this style:

/* bla
  * bla
  */

[...]

MBR, Sergei

Re: [net-next PATCH 0/4] fixup IFF_NO_QUEUE conversion

[Geert Uytterhoeven]

On Fri, Aug 28, 2015 at 2:15 AM, David Miller <davem@davemloft.net> wrote:
>> This series serves two purposes:
>>
>> On one hand it fixes a quite embarrassing bug around the warning I added for
>> drivers still setting tx_queue_len = 0 to achieve noqueue operation. It turned
>> out to be quite useless as due to using alloc_netdev(), many in-kernel drivers
>> fell into the trap by accident, as well. Instead this place serves pretty well
>> as a sanitizing point to set IFF_NO_QUEUE for drivers not initializing
>> tx_queue_len, which in turn allows to drop all special treatment of the latter
>> being zero since that can not happen anymore without IFF_NO_QUEUE being set.
>>
>> On the other hand, it provides a better solution for Eric Dumazet's concern
>> regarding how to assign noqueue to an interface which does not default to it
>> already. In order to make this possible, noqueue is being registered so users
>> can 'tc qd add dev eth0 root noqueue'. In addition, it resolves the ugly
>> situation of 'tc qd show' not showing noqueue. Finally, the former changes
>> allow for some code cleanup.
>
> Seems reasonable, series applied, thanks Phil.

Thanks, the warning is gone.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds