Re: 2.6: QoS scheduling not working with IP-over-IP

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

[ moving from linux-kernel to correct mailing list... ]

Czesc Dawid,

On Tue, 10 Feb 2004 13:36:32 +0100 (CET)
Dawid Kuroczko <qnex@atlantis.knm.org.pl> wrote:

> There seems to be a bug in QoS scheduling code related to IP-over-IP
> tunnels (ipip.ko).  My setup is a bit unusual -- my default route
> goes through tunl1, so I want it shaped.  It worked fine with 2.4,
> but it doesn't with 2.6 (2.6.1 and 2.6.2, I made a transition quite
> recently).

Can you assign QoS schedulers and classifiers successfully to eth0
or some other non ipip device?

Does it work with 'tunl0' instead of 'tunl1'?

Stephen, I bet this problem got introduced when we moved ipip.c over
to dynamic netdevice allocation, can you take a look?

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

Actually this may be a result of the recent tweak to
the scheduler code that seems to be in 2.6.2. The fix seems
to fix the off by one in pfifo.

cheers,
jamal

On Wed, 2004-02-11 at 23:05, David S. Miller wrote:
> [ moving from linux-kernel to correct mailing list... ]
> 
> Czesc Dawid,
> 
> On Tue, 10 Feb 2004 13:36:32 +0100 (CET)
> Dawid Kuroczko <qnex@atlantis.knm.org.pl> wrote:
> 
> > There seems to be a bug in QoS scheduling code related to IP-over-IP
> > tunnels (ipip.ko).  My setup is a bit unusual -- my default route
> > goes through tunl1, so I want it shaped.  It worked fine with 2.4,
> > but it doesn't with 2.6 (2.6.1 and 2.6.2, I made a transition quite
> > recently).
> 
> Can you assign QoS schedulers and classifiers successfully to eth0
> or some other non ipip device?
> 
> Does it work with 'tunl0' instead of 'tunl1'?
> 
> Stephen, I bet this problem got introduced when we moved ipip.c over
> to dynamic netdevice allocation, can you take a look?
> 
> 

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

What the heck, heres a patch (maybe it ll teach that poster to post to
netdev instead of lk ;->). Not tested not even compiled but looks fine.
ipip was always broken; the pfifo off by one fix in 2.6 seems to have
exposed the bug.

-----
--- 262/net/ipv4/ipip.c 2004-01-09 02:00:13.000000000 -0500
+++ 262-mod/net/ipv4/ipip.c     2004-02-11 23:35:18.000000000 -0500
@@ -247,6 +247,7 @@
        SET_MODULE_OWNER(dev);
        dev->init = ipip_tunnel_init;
        dev->destructor = free_netdev;
+       dev->tx_queue_len = 100;
        nt->parms = *parms;
 
        if (register_netdevice(dev) < 0) {

-------

cheers,
jamal

On Wed, 2004-02-11 at 23:51, jamal wrote:
> Actually this may be a result of the recent tweak to
> the scheduler code that seems to be in 2.6.2. The fix seems
> to fix the off by one in pfifo.
> 
> cheers,
> jamal
> 
> On Wed, 2004-02-11 at 23:05, David S. Miller wrote:
> > [ moving from linux-kernel to correct mailing list... ]
> > 
> > Czesc Dawid,
> > 
> > On Tue, 10 Feb 2004 13:36:32 +0100 (CET)
> > Dawid Kuroczko <qnex@atlantis.knm.org.pl> wrote:
> > 
> > > There seems to be a bug in QoS scheduling code related to IP-over-IP
> > > tunnels (ipip.ko).  My setup is a bit unusual -- my default route
> > > goes through tunl1, so I want it shaped.  It worked fine with 2.4,
> > > but it doesn't with 2.6 (2.6.1 and 2.6.2, I made a transition quite
> > > recently).
> > 
> > Can you assign QoS schedulers and classifiers successfully to eth0
> > or some other non ipip device?
> > 
> > Does it work with 'tunl0' instead of 'tunl1'?
> > 
> > Stephen, I bet this problem got introduced when we moved ipip.c over
> > to dynamic netdevice allocation, can you take a look?
> > 
> > 
> 
> 
> 

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Wed, 2004-02-11 at 23:59, jamal wrote:
> What the heck, heres a patch (maybe it ll teach that poster to post to
> netdev instead of lk ;->). Not tested not even compiled but looks fine.
> ipip was always broken; the pfifo off by one fix in 2.6 seems to have
> exposed the bug.
> 

More complete patch attached for other broken tunnel devices.
Note the bug exists in 2.4 as well but is masquareded by a boundary bug
in pfifo.

cheers,
jamal


[-- Attachment #2: p1 --]
[-- Type: text/plain, Size: 874 bytes --]

--- 261/net/ipv4/ip_gre.c	2004-01-09 01:59:48.000000000 -0500
+++ 261-mod/net/ipv4/ip_gre.c	2004-02-11 23:57:30.000000000 -0500
@@ -276,6 +276,7 @@
 	  return NULL;
 
 	dev->init = ipgre_tunnel_init;
+	dev->tx_queue_len = 100;
 	nt = dev->priv;
 	nt->parms = *parms;
 
--- 261/net/ipv4/ipip.c	2004-01-09 02:00:13.000000000 -0500
+++ 261-mod/net/ipv4/ipip.c	2004-02-11 23:35:18.000000000 -0500
@@ -247,6 +247,7 @@
 	SET_MODULE_OWNER(dev);
 	dev->init = ipip_tunnel_init;
 	dev->destructor = free_netdev;
+	dev->tx_queue_len = 100;
 	nt->parms = *parms;
 
 	if (register_netdevice(dev) < 0) {
--- 261/net/ipv6/sit.c	2004-01-09 02:00:03.000000000 -0500
+++ 261-mod/net/ipv6/sit.c	2004-02-11 23:58:00.000000000 -0500
@@ -184,6 +184,7 @@
 
 	nt = dev->priv;
 	dev->init = ipip6_tunnel_init;
+	dev->tx_queue_len = 100;
 	nt->parms = *parms;
 
 	if (register_netdevice(dev) < 0) {

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On 12 Feb 2004 00:04:42 -0500
jamal <hadi@cyberus.ca> wrote:

> More complete patch attached for other broken tunnel devices.
> Note the bug exists in 2.4 as well but is masquareded by a boundary bug
> in pfifo.

OK, however, old behavior would be like setting tx_queue_len to '1'.
Do we wish to preserve this?

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

On Thu, 2004-02-12 at 00:15, David S. Miller wrote:
> On 12 Feb 2004 00:04:42 -0500
> jamal <hadi@cyberus.ca> wrote:
> 
> > More complete patch attached for other broken tunnel devices.
> > Note the bug exists in 2.4 as well but is masquareded by a boundary bug
> > in pfifo.
> 
> OK, however, old behavior would be like setting tx_queue_len to '1'.
> Do we wish to preserve this?

All the alloc_netdev() types of calls dont set this - so default is
zero. Maybe we could move it there. 100 seems to be what a lot of
10/100 ether devices set things to be.
Note that ifconfig and ip can set this value as well at runtime

cheers,
jamal

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

On Thu, 2004-02-12 at 00:25, jamal wrote:

> > OK, however, old behavior would be like setting tx_queue_len to '1'.
> > Do we wish to preserve this?

I think i misunderstood you - you are saying maybe it was not a bug
after all in pfifo, right? i.e the off by one is ok.
I suppose you could look at it that way; i am just baffled that
tunnels worked fine though with that size of default.
Note the poster didnt seem to change the tx_queue_len 

cheers,
jamal

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On 12 Feb 2004 00:43:59 -0500
jamal <hadi@cyberus.ca> wrote:

> On Thu, 2004-02-12 at 00:25, jamal wrote:
> 
> > > OK, however, old behavior would be like setting tx_queue_len to '1'.
> > > Do we wish to preserve this?
> 
> I think i misunderstood you - you are saying maybe it was not a bug
> after all in pfifo, right? i.e the off by one is ok.

No, you understood me the first time.

I am saying, the PFIFO fix was correct.  And therefore if we are to preserve
the tunnel device behavior precisely to what happened before the PFIFO fix
we must set the tx_queue_len to '1'.

What use is any other value?  Show me a code path where more than one packet
can occupy the queue of one of these tunnel devices? :-)

Packets get headers added and straight to transmit to next device.
->hard_start_xmit() routines of these drivers always consume SKB and
return 0.

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

On Thu, 2004-02-12 at 00:51, David S. Miller wrote:

>  Show me a code path where more than one packet
> can occupy the queue of one of these tunnel devices? :-)

ok, makes sense ;->

cheers,
jamal

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On 12 Feb 2004 01:09:36 -0500
jamal <hadi@cyberus.ca> wrote:

> On Thu, 2004-02-12 at 00:51, David S. Miller wrote:
> 
> >  Show me a code path where more than one packet
> > can occupy the queue of one of these tunnel devices? :-)
> 
> ok, makes sense ;->

Great, here is the patch I am testing now.

BTW, you missed fixing up the xxx0 device in each of these things.

===== net/ipv4/ip_gre.c 1.34 vs edited =====
--- 1.34/net/ipv4/ip_gre.c	Tue Oct 28 01:13:14 2003
+++ edited/net/ipv4/ip_gre.c	Wed Feb 11 22:17:28 2004
@@ -276,6 +276,7 @@
 	  return NULL;
 
 	dev->init = ipgre_tunnel_init;
+	dev->tx_queue_len = 1;
 	nt = dev->priv;
 	nt->parms = *parms;
 
@@ -1269,6 +1270,7 @@
 	}
 
 	ipgre_fb_tunnel_dev->init = ipgre_fb_tunnel_init;
+	ipgre_fb_tunnel_dev->tx_queue_len = 1;
 
 	if ((err = register_netdev(ipgre_fb_tunnel_dev)))
 		goto fail;
===== net/ipv4/ipip.c 1.37 vs edited =====
--- 1.37/net/ipv4/ipip.c	Sun Oct 19 00:12:04 2003
+++ edited/net/ipv4/ipip.c	Wed Feb 11 22:17:46 2004
@@ -246,6 +246,7 @@
 	nt = dev->priv;
 	SET_MODULE_OWNER(dev);
 	dev->init = ipip_tunnel_init;
+	dev->tx_queue_len = 1;
 	dev->destructor = free_netdev;
 	nt->parms = *parms;
 
@@ -892,6 +893,7 @@
 	}
 
 	ipip_fb_tunnel_dev->init = ipip_fb_tunnel_init;
+	ipip_fb_tunnel_dev->tx_queue_len = 1;
 
 	if ((err = register_netdev(ipip_fb_tunnel_dev)))
 	    goto fail;
===== net/ipv6/sit.c 1.33 vs edited =====
--- 1.33/net/ipv6/sit.c	Mon Jan 19 21:23:19 2004
+++ edited/net/ipv6/sit.c	Wed Feb 11 22:18:05 2004
@@ -184,6 +184,7 @@
 
 	nt = dev->priv;
 	dev->init = ipip6_tunnel_init;
+	dev->tx_queue_len = 1;
 	nt->parms = *parms;
 
 	if (register_netdevice(dev) < 0) {
@@ -832,6 +833,7 @@
 	}
 
 	ipip6_fb_tunnel_dev->init = ipip6_fb_tunnel_init;
+	ipip6_fb_tunnel_dev->tx_queue_len = 1;
 
 	if ((err =  register_netdev(ipip6_fb_tunnel_dev)))
 		goto fail;

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On Fri, 13 Feb 2004 23:51:02 +0100
Patrick McHardy <kaber@trash.net> wrote:

> while looking for other devices that may be affected I noticed
> this fix changes behaviour in dev_activate. dev_activate creates
> pfifo_fast queues for devices with tx_queue_len != 0 and noqueue
> qdiscs otherwise.

Thanks for spotting this.

> Although it is uncomfortable for some users, I don't know if there
> is anything to fix at all. Work-conserving qdiscs are useless for
> virtual devices, as you say there is maximum one packet queued at
> any time. For non-work-conserving qdiscs with default pfifo qdiscs
> as leaves, I'd say it is simply a configuration error not to increase
> tx_queue_len. Even with a length of 1 (or a off-by-one), you won't get
> good results, except when the traffic is already well-formed. All that
> is changed is that nonsensical configurations stop working.

Another way to view this is that configurations that, while nonsensical,
used to send packets properly no longer do.

However, I do agree with you, and we should not promote this kind of crap.
I'm going to revert the changeset that set tx_queue_len to 1 in the tunnels.

Thanks again Patrick.

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

On Sat, 2004-02-14 at 00:36, David S. Miller wrote:

> However, I do agree with you, and we should not promote this kind of crap.
> I'm going to revert the changeset that set tx_queue_len to 1 in the tunnels.

I concur. But you gotta admit if that was a bug, it was the fastest
fixed bug of the week ;->

cheers,
jamal

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

On second thought:
Dawid noticed that SFQ and RED appear immune to this problem
(i think any non classful qdisc would exhibit same behavior).
Maybe we should allow for this behavior for consistency.
The suggestion that Alexey made  of setting limit of 1 to default 
child fifo when tx_queue_len is 0 is the better appraoch instead of
touching these devices.

cheers,
jamal

jamal wrote:
> On Sat, 2004-02-14 at 00:36, David S. Miller wrote:
> 
> > However, I do agree with you, and we should not promote this kind of crap.
> > I'm going to revert the changeset that set tx_queue_len to 1 in the tunnels.
> 
> I concur. But you gotta admit if that was a bug, it was the fastest
> fixed bug of the week ;->
> 
> cheers,
> jamal
> 
> 
> 
> 
> 

Re: 2.6: QoS scheduling not working with IP-over-IP

[jamal]

[-- Attachment #1: Type: text/plain, Size: 954 bytes --]


And the closest to a sane patch in 30 secs that compiles is attached
to describe the idea.

cheers,
jamal

On Sat, 2004-02-14 at 08:06, jamal wrote:
> On second thought:
> Dawid noticed that SFQ and RED appear immune to this problem
> (i think any non classful qdisc would exhibit same behavior).
> Maybe we should allow for this behavior for consistency.
> The suggestion that Alexey made  of setting limit of 1 to default 
> child fifo when tx_queue_len is 0 is the better appraoch instead of
> touching these devices.
> 
> cheers,
> jamal
> 
> jamal wrote:
> > On Sat, 2004-02-14 at 00:36, David S. Miller wrote:
> > 
> > > However, I do agree with you, and we should not promote this kind of crap.
> > > I'm going to revert the changeset that set tx_queue_len to 1 in the tunnels.
> > 
> > I concur. But you gotta admit if that was a bug, it was the fastest
> > fixed bug of the week ;->
> > 
> > cheers,
> > jamal
> > 
> > 
> > 
> > 
> > 
> 
> 
> 

[-- Attachment #2: p2 --]
[-- Type: text/plain, Size: 1155 bytes --]

--- 261-mod/net/sched/sch_generic.c	2004/02/14 13:10:43	1.1
+++ 261-mod/net/sched/sch_generic.c	2004/02/14 13:42:18
@@ -448,22 +448,17 @@
 
 	if (dev->qdisc_sleeping == &noop_qdisc) {
 		struct Qdisc *qdisc;
-		if (dev->tx_queue_len) {
-			qdisc = qdisc_create_dflt(dev, &pfifo_fast_ops);
-			if (qdisc == NULL) {
-				printk(KERN_INFO "%s: activation failed\n", dev->name);
-				return;
-			}
-
-			write_lock(&qdisc_tree_lock);
-			qdisc->next = dev->qdisc_list;
-			dev->qdisc_list = qdisc;
-			write_unlock(&qdisc_tree_lock);
-
-		} else {
-			qdisc =  &noqueue_qdisc;
+		if (!dev->tx_queue_len) 
+			dev->tx_queue_len = 1;
+		qdisc = qdisc_create_dflt(dev, &pfifo_fast_ops);
+		if (qdisc == NULL) {
+			printk(KERN_INFO "%s: activation failed\n", dev->name);
+			return;
 		}
+
 		write_lock(&qdisc_tree_lock);
+		qdisc->next = dev->qdisc_list;
+		dev->qdisc_list = qdisc;
 		dev->qdisc_sleeping = qdisc;
 		write_unlock(&qdisc_tree_lock);
 	}
@@ -489,6 +484,8 @@
 	spin_unlock_bh(&dev->queue_lock);
 
 	dev_watchdog_down(dev);
+	if (dev->tx_queue_len == 1)
+		dev->tx_queue_len = 0;
 
 	while (test_bit(__LINK_STATE_SCHED, &dev->state))
 		yield();

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On 14 Feb 2004 08:56:21 -0500
jamal <hadi@cyberus.ca> wrote:

> And the closest to a sane patch in 30 secs that compiles is attached
> to describe the idea.

I don't know how wise this tx_queue_len fiddling is.

With this, if user modifies tx_queue_len himself, on ifdown this configuration
change is lost if user sets it explicitly to '1'.  See?  In fact you could
call it corruption :-)

I think Alexey maybe meant something different, achieving the ends by some other
means than directly fiddling with tx_queue_len.

Re: 2.6: QoS scheduling not working with IP-over-IP

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

David S. Miller wrote:

> I don't know how wise this tx_queue_len fiddling is.
> 
> With this, if user modifies tx_queue_len himself, on ifdown this configuration
> change is lost if user sets it explicitly to '1'.  See?  In fact you could
> call it corruption :-)
> 
> I think Alexey maybe meant something different, achieving the ends by some other
> means than directly fiddling with tx_queue_len.
> 

I guess he meant something like this, but I still think it encourages
broken configurations in combination with non-work-conserving
schedulers.

Regards,
Patrick

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1015 bytes --]

===== sch_fifo.c 1.7 vs edited =====
--- 1.7/net/sched/sch_fifo.c	Wed Nov 19 02:37:34 2003
+++ edited/sch_fifo.c	Sun Feb 15 11:06:08 2004
@@ -141,10 +141,12 @@
 	struct fifo_sched_data *q = (void*)sch->data;
 
 	if (opt == NULL) {
+		unsigned int limit = sch->dev->tx_queue_len || 1;
+
 		if (sch->ops == &bfifo_qdisc_ops)
-			q->limit = sch->dev->tx_queue_len*sch->dev->mtu;
+			q->limit = limit*sch->dev->mtu;
 		else	
-			q->limit = sch->dev->tx_queue_len;
+			q->limit = limit;
 	} else {
 		struct tc_fifo_qopt *ctl = RTA_DATA(opt);
 		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
===== sch_gred.c 1.13 vs edited =====
--- 1.13/net/sched/sch_gred.c	Wed Nov 19 02:37:34 2003
+++ edited/sch_gred.c	Sun Feb 15 11:09:42 2004
@@ -110,7 +110,7 @@
 	unsigned long	qave=0;	
 	int i=0;
 
-	if (!t->initd && skb_queue_len(&sch->q) < sch->dev->tx_queue_len) {
+	if (!t->initd && skb_queue_len(&sch->q) < (sch->dev->tx_queue_len || 1)) {
 		D2PRINTK("NO GRED Queues setup yet! Enqueued anyway\n");
 		goto do_enqueue;
 	}

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On Sun, 15 Feb 2004 11:30:05 +0100
Patrick McHardy <kaber@trash.net> wrote:

> I guess he meant something like this, but I still think it encourages
> broken configurations in combination with non-work-conserving
> schedulers.

Patrick, "(x || 1)" is not the same as "(x ? x : 1)"
:-)

Please make a new patch.

Re: 2.6: QoS scheduling not working with IP-over-IP

[Patrick McHardy]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

David S. Miller wrote:
> On Sun, 15 Feb 2004 11:30:05 +0100
> Patrick McHardy <kaber@trash.net> wrote:
> 
> 
>>I guess he meant something like this, but I still think it encourages
>>broken configurations in combination with non-work-conserving
>>schedulers.
> 
> 
> Patrick, "(x || 1)" is not the same as "(x ? x : 1)"
> :-)

Doh! I must have been programming in perl before .. ;)

> 
> Please make a new patch.
> 

Attached.

Regards
Patrick

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1057 bytes --]

===== net/sched/sch_fifo.c 1.7 vs edited =====
--- 1.7/net/sched/sch_fifo.c	Wed Nov 19 02:37:34 2003
+++ edited/net/sched/sch_fifo.c	Wed Feb 18 12:06:06 2004
@@ -141,10 +141,12 @@
 	struct fifo_sched_data *q = (void*)sch->data;
 
 	if (opt == NULL) {
+		unsigned int limit = sch->dev->tx_queue_len ? : 1;
+
 		if (sch->ops == &bfifo_qdisc_ops)
-			q->limit = sch->dev->tx_queue_len*sch->dev->mtu;
+			q->limit = limit*sch->dev->mtu;
 		else	
-			q->limit = sch->dev->tx_queue_len;
+			q->limit = limit;
 	} else {
 		struct tc_fifo_qopt *ctl = RTA_DATA(opt);
 		if (opt->rta_len < RTA_LENGTH(sizeof(*ctl)))
===== net/sched/sch_gred.c 1.13 vs edited =====
--- 1.13/net/sched/sch_gred.c	Wed Nov 19 02:37:34 2003
+++ edited/net/sched/sch_gred.c	Wed Feb 18 12:06:56 2004
@@ -110,7 +110,7 @@
 	unsigned long	qave=0;	
 	int i=0;
 
-	if (!t->initd && skb_queue_len(&sch->q) < sch->dev->tx_queue_len) {
+	if (!t->initd && skb_queue_len(&sch->q) < (sch->dev->tx_queue_len ? : 1)) {
 		D2PRINTK("NO GRED Queues setup yet! Enqueued anyway\n");
 		goto do_enqueue;
 	}

Re: 2.6: QoS scheduling not working with IP-over-IP

[David S. Miller]

On Wed, 18 Feb 2004 12:21:22 +0100
Patrick McHardy <kaber@trash.net> wrote:

> David S. Miller wrote:
> > Patrick, "(x || 1)" is not the same as "(x ? x : 1)"
> > :-)
> 
> Doh! I must have been programming in perl before .. ;)

New patch applied, thanks Patrick.