[PATCH] Revert "net: sched: drop all special handling of tx_queue_len == 0"

This reverts commit 348e3435cbefa815bd56a5205c1412b5afe7b92e.
It breaks HTB classful qdiscs on the loopback interface.

It has been broken since kernel v4.2. The offending commit has
been identified by bissection of the issue with the following
test-case. It appears that the loopback interface does indeed
still have tx_queue_len == 0.

Reverting the commit on a v4.4.1 kernel fixes the issue.

When the problem occurs, no network traffic (at all), not
even an ICMP ping, can go through the loopback interface.

The following bash script reproduces the issue:

SESSIOND_CTRL_PORT=5342
SESSIOND_DATA_PORT=5343
DEFAULT_IF="lo"

function set_bw_limit
{
        limit=$1
        ctrlportlimit=$(($limit/10))
        [ $ctrlportlimit = 0 ] && ctrlportlimit=1
        dataportlimit=$((9*${ctrlportlimit}))

        tc qdisc add dev $DEFAULT_IF root handle 1: htb default 15

        tc class add dev $DEFAULT_IF parent 1: classid 1:1 htb rate ${limit}kbit ceil ${limit}kbit
        tc class add dev $DEFAULT_IF parent 1:1 classid 1:10 htb rate ${ctrlportlimit}kbit ceil ${limit}kbit prio 1
        tc class add dev $DEFAULT_IF parent 1:1 classid 1:11 htb rate ${dataportlimit}kbit ceil ${limit}kbit prio 2

        tc filter add dev $DEFAULT_IF parent 1: protocol ip u32 match ip dport $SESSIOND_CTRL_PORT 0xffff flowid 1:10
        tc filter add dev $DEFAULT_IF parent 1: protocol ip u32 match ip dport $SESSIOND_DATA_PORT 0xffff flowid 1:11

        echo "Set bandwidth limits to ${limit}kbits, ${ctrlportlimit} for control and ${dataportlimit} for data"
}

function reset_bw_limit
{
        tc qdisc del dev $DEFAULT_IF root
        echo "Reset bandwith limits"
}

trap reset_bw_limit SIGINT SIGTERM

set_bw_limit 3200
sleep 1
ping localhost

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Reported-by: Jonathan Rajotte-Julien <redacted>
CC: Phil Sutter <phil@nwl.cc>
CC: Jamal Hadi Salim <jhs@mojatatu.com>
CC: David S. Miller <davem@davemloft.net>
CC: netdev@vger.kernel.org
CC: stable@vger.kernel.org # 4.2+
---
 net/sched/sch_fifo.c | 2 +-
 net/sched/sch_gred.c | 8 +++++---
 net/sched/sch_htb.c  | 6 ++++--
 net/sched/sch_plug.c | 8 ++++++--
 net/sched/sch_sfb.c  | 2 +-
 5 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/net/sched/sch_fifo.c b/net/sched/sch_fifo.c
index 2177eac..2e2398c 100644
--- a/net/sched/sch_fifo.c
+++ b/net/sched/sch_fifo.c

@@ -54,7 +54,7 @@ static int fifo_init(struct Qdisc *sch, struct nlattr *opt)
 	bool is_bfifo = sch->ops == &bfifo_qdisc_ops;
 
 	if (opt == NULL) {
-		u32 limit = qdisc_dev(sch)->tx_queue_len;
+		u32 limit = qdisc_dev(sch)->tx_queue_len ? : 1;
 
 		if (is_bfifo)
 			limit *= psched_mtu(qdisc_dev(sch));

diff --git a/net/sched/sch_gred.c b/net/sched/sch_gred.c
index 8010510..abb9f2f 100644
--- a/net/sched/sch_gred.c
+++ b/net/sched/sch_gred.c

@@ -512,9 +512,11 @@ static int gred_init(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_GRED_LIMIT])
 		sch->limit = nla_get_u32(tb[TCA_GRED_LIMIT]);
-	else
-		sch->limit = qdisc_dev(sch)->tx_queue_len
-		             * psched_mtu(qdisc_dev(sch));
+	else {
+		u32 qlen = qdisc_dev(sch)->tx_queue_len ? : 1;
+
+		sch->limit = qlen * psched_mtu(qdisc_dev(sch));
+	}
 
 	return gred_change_table_def(sch, tb[TCA_GRED_DPS]);
 }

diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 15ccd7f..366ba83 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c

@@ -1048,9 +1048,11 @@ static int htb_init(struct Qdisc *sch, struct nlattr *opt)
 
 	if (tb[TCA_HTB_DIRECT_QLEN])
 		q->direct_qlen = nla_get_u32(tb[TCA_HTB_DIRECT_QLEN]);
-	else
+	else {
 		q->direct_qlen = qdisc_dev(sch)->tx_queue_len;
-
+		if (q->direct_qlen < 2)	/* some devices have zero tx_queue_len */
+			q->direct_qlen = 2;
+	}
 	if ((q->rate2quantum = gopt->rate2quantum) < 1)
 		q->rate2quantum = 1;
 	q->defcls = gopt->defcls;

diff --git a/net/sched/sch_plug.c b/net/sched/sch_plug.c
index 5abfe44..ade9445 100644
--- a/net/sched/sch_plug.c
+++ b/net/sched/sch_plug.c

@@ -130,8 +130,12 @@ static int plug_init(struct Qdisc *sch, struct nlattr *opt)
 	q->unplug_indefinite = false;
 
 	if (opt == NULL) {
-		q->limit = qdisc_dev(sch)->tx_queue_len
-		           * psched_mtu(qdisc_dev(sch));
+		/* We will set a default limit of 100 pkts (~150kB)
+		 * in case tx_queue_len is not available. The
+		 * default value is completely arbitrary.
+		 */
+		u32 pkt_limit = qdisc_dev(sch)->tx_queue_len ? : 100;
+		q->limit = pkt_limit * psched_mtu(qdisc_dev(sch));
 	} else {
 		struct tc_plug_qopt *ctl = nla_data(opt);
 

diff --git a/net/sched/sch_sfb.c b/net/sched/sch_sfb.c
index 5bbb633..d5c1c1d 100644
--- a/net/sched/sch_sfb.c
+++ b/net/sched/sch_sfb.c

@@ -502,7 +502,7 @@ static int sfb_change(struct Qdisc *sch, struct nlattr *opt)
 
 	limit = ctl->limit;
 	if (limit == 0)
-		limit = qdisc_dev(sch)->tx_queue_len;
+		limit = max_t(u32, qdisc_dev(sch)->tx_queue_len, 1);
 
 	child = fifo_create_dflt(sch, &pfifo_qdisc_ops, limit);
 	if (IS_ERR(child))

-- 
1.9.1

Hi,

On Tue, Feb 16, 2016 at 07:56:23PM -0500, Mathieu Desnoyers wrote:

This reverts commit 348e3435cbefa815bd56a5205c1412b5afe7b92e.
It breaks HTB classful qdiscs on the loopback interface.

It has been broken since kernel v4.2. The offending commit has
been identified by bissection of the issue with the following
test-case. It appears that the loopback interface does indeed
still have tx_queue_len == 0.

Reverting the commit on a v4.4.1 kernel fixes the issue.

Indeed, this is ugly. Affected are all drivers not calling ether_setup()
in their setup callback, and therefore not initializing tx_queue_len.

As the commit to be reverted shows, there is no common fallback value
for tx_queue_len - most qdisc implementations used 1, but HTB fell back
to 2 and PLUG to 100. But as the removed comment in sch_plug.c stated:

| /* We will set a default limit of 100 pkts (~150kB)
|  * in case tx_queue_len is not available. The
|  * default value is completely arbitrary.
|  */

It seems not to be overly important to fallback to the exact value each
qdisc had before. Therefore I guess the following change should
appropriately fix the issue at hand:

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -7440,8 +7440,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
        dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
        setup(dev);
 
-       if (!dev->tx_queue_len)
+       if (!dev->tx_queue_len) {
                dev->priv_flags |= IFF_NO_QUEUE;
+               dev->tx_queue_len = 1;
+       }
 
        dev->num_tx_queues = txqs;
        dev->real_num_tx_queues = txqs;

Unless there is concern, I will formally submit this later.

Thanks, Phil

----- On Feb 17, 2016, at 7:47 AM, Phil Sutter phil@nwl.cc wrote:

Works for me!

Tested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Thanks,

Mathieu

Thanks, Phil

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

On Wed, Feb 17, 2016 at 01:57:42PM +0000, Mathieu Desnoyers wrote:

----- On Feb 17, 2016, at 7:47 AM, Phil Sutter phil@nwl.cc wrote:

quoted

Hi,

On Tue, Feb 16, 2016 at 07:56:23PM -0500, Mathieu Desnoyers wrote:

quoted

This reverts commit 348e3435cbefa815bd56a5205c1412b5afe7b92e.
It breaks HTB classful qdiscs on the loopback interface.

It has been broken since kernel v4.2. The offending commit has
been identified by bissection of the issue with the following
test-case. It appears that the loopback interface does indeed
still have tx_queue_len == 0.

Reverting the commit on a v4.4.1 kernel fixes the issue.

Indeed, this is ugly. Affected are all drivers not calling ether_setup()
in their setup callback, and therefore not initializing tx_queue_len.

As the commit to be reverted shows, there is no common fallback value
for tx_queue_len - most qdisc implementations used 1, but HTB fell back
to 2 and PLUG to 100. But as the removed comment in sch_plug.c stated:

| /* We will set a default limit of 100 pkts (~150kB)
|  * in case tx_queue_len is not available. The
|  * default value is completely arbitrary.
|  */

It seems not to be overly important to fallback to the exact value each
qdisc had before. Therefore I guess the following change should
appropriately fix the issue at hand:

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -7440,8 +7440,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv,

const char *name,
       dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
       setup(dev);

-       if (!dev->tx_queue_len)
+       if (!dev->tx_queue_len) {
               dev->priv_flags |= IFF_NO_QUEUE;
+               dev->tx_queue_len = 1;
+       }

       dev->num_tx_queues = txqs;
       dev->real_num_tx_queues = txqs;

Unless there is concern, I will formally submit this later.

Works for me!

Tested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

Cool, thanks for testing!

Cheers, Phil

My implementation around IFF_NO_QUEUE driver flag assumed that leaving
tx_queue_len untouched (specifically: not setting it to zero) by drivers
would make it possible to assign a regular qdisc to them without having
to worry about setting tx_queue_len to a useful value. This was only
partially true: I overlooked that some drivers don't call ether_setup()
and therefore not initialize tx_queue_len to the default value of 1000.
Consequently, removing the workarounds in place for that case in qdisc
implementations which cared about it (namely, pfifo, bfifo, gred, htb,
plug and sfb) leads to problems with these specific interface types and
qdiscs.

Luckily, there's already a sanitization point for drivers setting
tx_queue_len to zero, which can be reused to assign the fallback value
most qdisc implementations used, which is 1.

Fixes: 348e3435cbefa ("net: sched: drop all special handling of tx_queue_len == 0")
Tested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>
---
 net/core/dev.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 3f4071a84a03f..75383f40e7ced 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -7440,8 +7440,10 @@ struct net_device *alloc_netdev_mqs(int sizeof_priv, const char *name,
 	dev->priv_flags = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM;
 	setup(dev);
 
-	if (!dev->tx_queue_len)
+	if (!dev->tx_queue_len) {
 		dev->priv_flags |= IFF_NO_QUEUE;
+		dev->tx_queue_len = 1;
+	}
 
 	dev->num_tx_queues = txqs;
 	dev->real_num_tx_queues = txqs;

-- 
2.7.0

From: Phil Sutter <phil@nwl.cc>
Date: Wed, 17 Feb 2016 15:37:43 +0100

My implementation around IFF_NO_QUEUE driver flag assumed that leaving
tx_queue_len untouched (specifically: not setting it to zero) by drivers
would make it possible to assign a regular qdisc to them without having
to worry about setting tx_queue_len to a useful value. This was only
partially true: I overlooked that some drivers don't call ether_setup()
and therefore not initialize tx_queue_len to the default value of 1000.
Consequently, removing the workarounds in place for that case in qdisc
implementations which cared about it (namely, pfifo, bfifo, gred, htb,
plug and sfb) leads to problems with these specific interface types and
qdiscs.

Luckily, there's already a sanitization point for drivers setting
tx_queue_len to zero, which can be reused to assign the fallback value
most qdisc implementations used, which is 1.

Fixes: 348e3435cbefa ("net: sched: drop all special handling of tx_queue_len == 0")
Tested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Signed-off-by: Phil Sutter <phil@nwl.cc>

Applied and queued up for -stable, thanks.