qdisc and down links (regression)

qdisc and down links (regression)

[Stephen Hemminger]

Before the mulitqueue changes in 2.6.27-rc it was possible to setup
queueing disciplines before the link came up (carrier active). This
no longer works. If link is down, the qdisc is the noop_qdisc and
any configuration changes don't seem to be shown.

This probably will break ISP's (and other users whose links aren't always
on) that use traffic shaping.

For example, try wondershaper or similar script before boot; all the
tc commands will fail.

Re: qdisc and down links (regression)

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Thu, 7 Aug 2008 14:30:36 -0700

> Before the mulitqueue changes in 2.6.27-rc it was possible to setup
> queueing disciplines before the link came up (carrier active). This
> no longer works. If link is down, the qdisc is the noop_qdisc and
> any configuration changes don't seem to be shown.

I'll see why this happens, it wasn't an intentional change.

Before the link comes up, we don't activate the qdisc.  We just
remember it in ->qdisc_sleeping.  I bet if you bring the link up the
configuration will become visible, or that is the area where the
unintentional error is occuring.

Re: qdisc and down links (regression)

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Thu, 07 Aug 2008 15:47:35 -0700 (PDT)

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Thu, 7 Aug 2008 14:30:36 -0700
> 
> > Before the mulitqueue changes in 2.6.27-rc it was possible to setup
> > queueing disciplines before the link came up (carrier active). This
> > no longer works. If link is down, the qdisc is the noop_qdisc and
> > any configuration changes don't seem to be shown.
> 
> I'll see why this happens, it wasn't an intentional change.

This should fix it, let me know if it doesn't:

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index 83b23b5..ba1d121 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -189,7 +189,7 @@ struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle)
 
 	for (i = 0; i < dev->num_tx_queues; i++) {
 		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
-		struct Qdisc *q, *txq_root = txq->qdisc;
+		struct Qdisc *q, *txq_root = txq->qdisc_sleeping;
 
 		if (!(txq_root->flags & TCQ_F_BUILTIN) &&
 		    txq_root->handle == handle)
@@ -793,7 +793,7 @@ qdisc_create(struct net_device *dev, struct netdev_queue *dev_queue,
 			}
 		}
 		if ((parent != TC_H_ROOT) && !(sch->flags & TCQ_F_INGRESS))
-			list_add_tail(&sch->list, &dev_queue->qdisc->list);
+			list_add_tail(&sch->list, &dev_queue->qdisc_sleeping->list);
 
 		return sch;
 	}
@@ -1236,11 +1236,11 @@ static int tc_dump_qdisc(struct sk_buff *skb, struct netlink_callback *cb)
 		q_idx = 0;
 
 		dev_queue = netdev_get_tx_queue(dev, 0);
-		if (tc_dump_qdisc_root(dev_queue->qdisc, skb, cb, &q_idx, s_q_idx) < 0)
+		if (tc_dump_qdisc_root(dev_queue->qdisc_sleeping, skb, cb, &q_idx, s_q_idx) < 0)
 			goto done;
 
 		dev_queue = &dev->rx_queue;
-		if (tc_dump_qdisc_root(dev_queue->qdisc, skb, cb, &q_idx, s_q_idx) < 0)
+		if (tc_dump_qdisc_root(dev_queue->qdisc_sleeping, skb, cb, &q_idx, s_q_idx) < 0)
 			goto done;
 
 cont:

Re: qdisc and down links (regression)

[Stephen Hemminger]

On Thu, 07 Aug 2008 18:50:24 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: David Miller <davem@davemloft.net>
> Date: Thu, 07 Aug 2008 15:47:35 -0700 (PDT)
> 
> > From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> > Date: Thu, 7 Aug 2008 14:30:36 -0700
> > 
> > > Before the mulitqueue changes in 2.6.27-rc it was possible to setup
> > > queueing disciplines before the link came up (carrier active). This
> > > no longer works. If link is down, the qdisc is the noop_qdisc and
> > > any configuration changes don't seem to be shown.
> > 
> > I'll see why this happens, it wasn't an intentional change.
> 
> This should fix it, let me know if it doesn't:

It fixes it for qdisc but not for ingress filters

# tc qdisc add dev eth0 handle ffff: ingress
# tc filter add dev eth0 parent ffff: protocol ip \
   prio 50 u32 match ip src 0.0.0.0/0 police rate 800kbit burst 10k drop flowid :1
RTNETLINK answers: Invalid argument
We have an error talking to the kernel

Another problem is that the ingress qdisc can no longer be deleted
successfully.

Re: qdisc and down links (regression)

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Fri, 8 Aug 2008 14:32:01 -0700

> On Thu, 07 Aug 2008 18:50:24 -0700 (PDT)
> David Miller <davem@davemloft.net> wrote:
> 
> > This should fix it, let me know if it doesn't:
> 
> It fixes it for qdisc but not for ingress filters
> 
> # tc qdisc add dev eth0 handle ffff: ingress
> # tc filter add dev eth0 parent ffff: protocol ip \
>    prio 50 u32 match ip src 0.0.0.0/0 police rate 800kbit burst 10k drop flowid :1
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel
> 
> Another problem is that the ingress qdisc can no longer be deleted
> successfully.

Thanks, I'll take a look at this.

Re: qdisc and down links (regression)

[Stephen Hemminger]

This is the wondershaper script that I used for testing only.
---------------------------------------------------------------
#!/bin/bash
# Wonder Shaper
# please read the README before filling out these values 
#
# Set the following values to somewhat less than your actual download
# and uplink speed. In kilobits. Also set the device that is to be shaped.

DOWNLINK=800
UPLINK=220
DEV=eth0

# low priority OUTGOING traffic - you can leave this blank if you want
# low priority source netmasks
NOPRIOHOSTSRC=

# low priority destination netmasks
NOPRIOHOSTDST=

# low priority source ports
NOPRIOPORTSRC=

# low priority destination ports
NOPRIOPORTDST=

if [ "$1" = "status" ]
then
	tc -s qdisc ls dev $DEV
	tc -s class ls dev $DEV
	exit
fi


# clean existing down- and uplink qdiscs, hide errors
tc qdisc del dev $DEV root    2> /dev/null > /dev/null
tc qdisc del dev $DEV ingress 2> /dev/null > /dev/null

if [ "$1" = "stop" ] 
then 
	exit
fi


###### uplink

# install root HTB, point default traffic to 1:20:

tc qdisc add dev $DEV root handle 1: htb default 20

# shape everything at $UPLINK speed - this prevents huge queues in your
# DSL modem which destroy latency:

tc class add dev $DEV parent 1: classid 1:1 htb rate ${UPLINK}kbit burst 6k

# high prio class 1:10:

tc class add dev $DEV parent 1:1 classid 1:10 htb rate ${UPLINK}kbit \
   burst 6k prio 1

# bulk & default class 1:20 - gets slightly less traffic, 
# and a lower priority:

tc class add dev $DEV parent 1:1 classid 1:20 htb rate $[9*$UPLINK/10]kbit \
   burst 6k prio 2

tc class add dev $DEV parent 1:1 classid 1:30 htb rate $[8*$UPLINK/10]kbit \
   burst 6k prio 2

# all get Stochastic Fairness:
tc qdisc add dev $DEV parent 1:10 handle 10: sfq perturb 10
tc qdisc add dev $DEV parent 1:20 handle 20: sfq perturb 10
tc qdisc add dev $DEV parent 1:30 handle 30: sfq perturb 10

# TOS Minimum Delay (ssh, NOT scp) in 1:10:

tc filter add dev $DEV parent 1:0 protocol ip prio 10 u32 \
      match ip tos 0x10 0xff  flowid 1:10

# ICMP (ip protocol 1) in the interactive class 1:10 so we 
# can do measurements & impress our friends:
tc filter add dev $DEV parent 1:0 protocol ip prio 10 u32 \
        match ip protocol 1 0xff flowid 1:10

# To speed up downloads while an upload is going on, put ACK packets in
# the interactive class:

tc filter add dev $DEV parent 1: protocol ip prio 10 u32 \
   match ip protocol 6 0xff \
   match u8 0x05 0x0f at 0 \
   match u16 0x0000 0xffc0 at 2 \
   match u8 0x10 0xff at 33 \
   flowid 1:10

# rest is 'non-interactive' ie 'bulk' and ends up in 1:20

# some traffic however suffers a worse fate
for a in $NOPRIOPORTDST
do
	tc filter add dev $DEV parent 1: protocol ip prio 14 u32 \
	   match ip dport $a 0xffff flowid 1:30
done

for a in $NOPRIOPORTSRC
do
 	tc filter add dev $DEV parent 1: protocol ip prio 15 u32 \
	   match ip sport $a 0xffff flowid 1:30
done

for a in $NOPRIOHOSTSRC
do
 	tc filter add dev $DEV parent 1: protocol ip prio 16 u32 \
	   match ip src $a flowid 1:30
done

for a in $NOPRIOHOSTDST
do
 	tc filter add dev $DEV parent 1: protocol ip prio 17 u32 \
	   match ip dst $a flowid 1:30
done

# rest is 'non-interactive' ie 'bulk' and ends up in 1:20

tc filter add dev $DEV parent 1: protocol ip prio 18 u32 \
   match ip dst 0.0.0.0/0 flowid 1:20


########## downlink #############
# slow downloads down to somewhat less than the real speed  to prevent 
# queuing at our ISP. Tune to see how high you can set it.
# ISPs tend to have *huge* queues to make sure big downloads are fast
#
# attach ingress policer:

tc qdisc add dev $DEV handle ffff: ingress

# filter *everything* to it (0.0.0.0/0), drop everything that's
# coming in too fast:

tc filter add dev $DEV parent ffff: protocol ip prio 50 u32 match ip src \
   0.0.0.0/0 police rate ${DOWNLINK}kbit burst 10k drop flowid :1

Re: qdisc and down links (regression)

[David Miller]

From: Stephen Hemminger <stephen.hemminger@vyatta.com>
Date: Fri, 8 Aug 2008 14:32:01 -0700

> It fixes it for qdisc but not for ingress filters
> 
> # tc qdisc add dev eth0 handle ffff: ingress
> # tc filter add dev eth0 parent ffff: protocol ip \
>    prio 50 u32 match ip src 0.0.0.0/0 police rate 800kbit burst 10k drop flowid :1
> RTNETLINK answers: Invalid argument
> We have an error talking to the kernel
> 
> Another problem is that the ingress qdisc can no longer be deleted
> successfully.

These problems should be fixed by the following patch which
I've just pushed to net-2.6:

pkt_sched: Fix ingress deletion and filter attachment.

Based upon bug reports by Stephen Hemminger.

We still had some cases using ->qdisc instead of ->qdisc_sleeping.

Also, qdisc_lookup() should return ingress qdiscs.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 net/sched/sch_api.c |   36 +++++++++++++++++++++++-------------
 1 files changed, 23 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index ba1d121..bbf149d 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -183,6 +183,21 @@ EXPORT_SYMBOL(unregister_qdisc);
    (root qdisc, all its children, children of children etc.)
  */
 
+struct Qdisc *qdisc_match_from_root(struct Qdisc *root, u32 handle)
+{
+	struct Qdisc *q;
+
+	if (!(root->flags & TCQ_F_BUILTIN) &&
+	    root->handle == handle)
+		return root;
+
+	list_for_each_entry(q, &root->list, list) {
+		if (q->handle == handle)
+			return q;
+	}
+	return NULL;
+}
+
 struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle)
 {
 	unsigned int i;
@@ -191,16 +206,11 @@ struct Qdisc *qdisc_lookup(struct net_device *dev, u32 handle)
 		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
 		struct Qdisc *q, *txq_root = txq->qdisc_sleeping;
 
-		if (!(txq_root->flags & TCQ_F_BUILTIN) &&
-		    txq_root->handle == handle)
-			return txq_root;
-
-		list_for_each_entry(q, &txq_root->list, list) {
-			if (q->handle == handle)
-				return q;
-		}
+		q = qdisc_match_from_root(txq_root, handle);
+		if (q)
+			return q;
 	}
-	return NULL;
+	return qdisc_match_from_root(dev->rx_queue.qdisc_sleeping, handle);
 }
 
 static struct Qdisc *qdisc_leaf(struct Qdisc *p, u32 classid)
@@ -908,7 +918,7 @@ static int tc_get_qdisc(struct sk_buff *skb, struct nlmsghdr *n, void *arg)
 					return -ENOENT;
 				q = qdisc_leaf(p, clid);
 			} else { /* ingress */
-				q = dev->rx_queue.qdisc;
+				q = dev->rx_queue.qdisc_sleeping;
 			}
 		} else {
 			struct netdev_queue *dev_queue;
@@ -978,7 +988,7 @@ replay:
 					return -ENOENT;
 				q = qdisc_leaf(p, clid);
 			} else { /*ingress */
-				q = dev->rx_queue.qdisc;
+				q = dev->rx_queue.qdisc_sleeping;
 			}
 		} else {
 			struct netdev_queue *dev_queue;
@@ -1529,11 +1539,11 @@ static int tc_dump_tclass(struct sk_buff *skb, struct netlink_callback *cb)
 	t = 0;
 
 	dev_queue = netdev_get_tx_queue(dev, 0);
-	if (tc_dump_tclass_root(dev_queue->qdisc, skb, tcm, cb, &t, s_t) < 0)
+	if (tc_dump_tclass_root(dev_queue->qdisc_sleeping, skb, tcm, cb, &t, s_t) < 0)
 		goto done;
 
 	dev_queue = &dev->rx_queue;
-	if (tc_dump_tclass_root(dev_queue->qdisc, skb, tcm, cb, &t, s_t) < 0)
+	if (tc_dump_tclass_root(dev_queue->qdisc_sleeping, skb, tcm, cb, &t, s_t) < 0)
 		goto done;
 
 done:
-- 
1.5.6.5.GIT

Re: qdisc and down links (regression)

[Stephen Hemminger]

On Fri, 08 Aug 2008 23:24:27 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Stephen Hemminger <stephen.hemminger@vyatta.com>
> Date: Fri, 8 Aug 2008 14:32:01 -0700
> 
> > It fixes it for qdisc but not for ingress filters
> > 
> > # tc qdisc add dev eth0 handle ffff: ingress
> > # tc filter add dev eth0 parent ffff: protocol ip \
> >    prio 50 u32 match ip src 0.0.0.0/0 police rate 800kbit burst 10k drop flowid :1
> > RTNETLINK answers: Invalid argument
> > We have an error talking to the kernel
> > 
> > Another problem is that the ingress qdisc can no longer be deleted
> > successfully.
> 
> These problems should be fixed by the following patch which
> I've just pushed to net-2.6:
> 
> pkt_sched: Fix ingress deletion and filter attachment.
> 
> Based upon bug reports by Stephen Hemminger.
> 
> We still had some cases using ->qdisc instead of ->qdisc_sleeping.
> 
> Also, qdisc_lookup() should return ingress qdiscs.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>

Thanks, all fixed.