[PATCH]: Adjust qlen when grafting in multiple qdiscs

[PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

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

Hi Dave,

multiple qdiscs fail to adjust sch->q.qlen after grafting when the
old qdisc is non-empty. This permanently damages the counter.
TBF additionally needs to adjust stats.backlog.

Best regards,
Patrick



[-- Attachment #2: 01-graft-qlen.diff --]
[-- Type: text/plain, Size: 2052 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1427  -> 1.1428 
#	 net/sched/sch_tbf.c	1.10    -> 1.11   
#	 net/sched/sch_cbq.c	1.13    -> 1.14   
#	net/sched/sch_dsmark.c	1.10    -> 1.11   
#	net/sched/sch_prio.c	1.9     -> 1.10   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/11/12	kaber@trash.net	1.1428
# Adjust qlen when grafting in multiple qdiscs
# --------------------------------------------
#
diff -Nru a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
--- a/net/sched/sch_cbq.c	Thu Nov 13 14:57:07 2003
+++ b/net/sched/sch_cbq.c	Thu Nov 13 14:57:07 2003
@@ -1670,6 +1670,7 @@
 		sch_tree_lock(sch);
 		*old = cl->q;
 		cl->q = new;
+		sch->q.qlen -= (*old)->q.qlen;
 		qdisc_reset(*old);
 		sch_tree_unlock(sch);
 
diff -Nru a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
--- a/net/sched/sch_dsmark.c	Thu Nov 13 14:57:07 2003
+++ b/net/sched/sch_dsmark.c	Thu Nov 13 14:57:07 2003
@@ -75,8 +75,10 @@
 		new = &noop_qdisc;
 	sch_tree_lock(sch);
 	*old = xchg(&p->q,new);
-	if (*old)
+	if (*old) {
+		sch->q.qlen -= (*old)->q.qlen;
 		qdisc_reset(*old);
+	}
 	sch_tree_unlock(sch); /* @@@ move up ? */
         return 0;
 }
diff -Nru a/net/sched/sch_prio.c b/net/sched/sch_prio.c
--- a/net/sched/sch_prio.c	Thu Nov 13 14:57:07 2003
+++ b/net/sched/sch_prio.c	Thu Nov 13 14:57:07 2003
@@ -266,6 +266,7 @@
 	sch_tree_lock(sch);
 	*old = q->queues[band];
 	q->queues[band] = new;
+	sch->q.qlen -= (*old)->q.qlen;
 	qdisc_reset(*old);
 	sch_tree_unlock(sch);
 
diff -Nru a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
--- a/net/sched/sch_tbf.c	Thu Nov 13 14:57:07 2003
+++ b/net/sched/sch_tbf.c	Thu Nov 13 14:57:07 2003
@@ -455,6 +455,8 @@
 	sch_tree_lock(sch);	
 	*old = xchg(&q->qdisc, new);
 	qdisc_reset(*old);
+	sch->q.qlen = 0;
+	sch->stats.backlog = 0;
 	sch_tree_unlock(sch);
 	
 	return 0;

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[jamal]

Did you test some of this stuff or just did a mass-edit? I havent paid
attention to all the details, but what would decrementing sch->q.qlen on
grafting mean on a dsmark? 

cheers,
jamal

On Thu, 2003-11-13 at 09:47, Patrick McHardy wrote:
> Hi Dave,
> 
> multiple qdiscs fail to adjust sch->q.qlen after grafting when the
> old qdisc is non-empty. This permanently damages the counter.
> TBF additionally needs to adjust stats.backlog.
> 
> Best regards,
> Patrick
> 
> 
> 
> ______________________________________________________________________
> 
> # This is a BitKeeper generated patch for the following project:
> # Project Name: Linux kernel tree
> # This patch format is intended for GNU patch command version 2.5 or higher.
> # This patch includes the following deltas:
> #	           ChangeSet	1.1427  -> 1.1428 
> #	 net/sched/sch_tbf.c	1.10    -> 1.11   
> #	 net/sched/sch_cbq.c	1.13    -> 1.14   
> #	net/sched/sch_dsmark.c	1.10    -> 1.11   
> #	net/sched/sch_prio.c	1.9     -> 1.10   
> #
> # The following is the BitKeeper ChangeSet Log
> # --------------------------------------------
> # 03/11/12	kaber@trash.net	1.1428
> # Adjust qlen when grafting in multiple qdiscs
> # --------------------------------------------
> #
> diff -Nru a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
> --- a/net/sched/sch_cbq.c	Thu Nov 13 14:57:07 2003
> +++ b/net/sched/sch_cbq.c	Thu Nov 13 14:57:07 2003
> @@ -1670,6 +1670,7 @@
>  		sch_tree_lock(sch);
>  		*old = cl->q;
>  		cl->q = new;
> +		sch->q.qlen -= (*old)->q.qlen;
>  		qdisc_reset(*old);
>  		sch_tree_unlock(sch);
>  
> diff -Nru a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
> --- a/net/sched/sch_dsmark.c	Thu Nov 13 14:57:07 2003
> +++ b/net/sched/sch_dsmark.c	Thu Nov 13 14:57:07 2003
> @@ -75,8 +75,10 @@
>  		new = &noop_qdisc;
>  	sch_tree_lock(sch);
>  	*old = xchg(&p->q,new);
> -	if (*old)
> +	if (*old) {
> +		sch->q.qlen -= (*old)->q.qlen;
>  		qdisc_reset(*old);
> +	}
>  	sch_tree_unlock(sch); /* @@@ move up ? */
>          return 0;
>  }
> diff -Nru a/net/sched/sch_prio.c b/net/sched/sch_prio.c
> --- a/net/sched/sch_prio.c	Thu Nov 13 14:57:07 2003
> +++ b/net/sched/sch_prio.c	Thu Nov 13 14:57:07 2003
> @@ -266,6 +266,7 @@
>  	sch_tree_lock(sch);
>  	*old = q->queues[band];
>  	q->queues[band] = new;
> +	sch->q.qlen -= (*old)->q.qlen;
>  	qdisc_reset(*old);
>  	sch_tree_unlock(sch);
>  
> diff -Nru a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
> --- a/net/sched/sch_tbf.c	Thu Nov 13 14:57:07 2003
> +++ b/net/sched/sch_tbf.c	Thu Nov 13 14:57:07 2003
> @@ -455,6 +455,8 @@
>  	sch_tree_lock(sch);	
>  	*old = xchg(&q->qdisc, new);
>  	qdisc_reset(*old);
> +	sch->q.qlen = 0;
> +	sch->stats.backlog = 0;
>  	sch_tree_unlock(sch);
>  	
>  	return 0;

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

Hi Jamal,

I've tested tbf and prio changes before and after. Unfortunately I've 
never used dsmark so I didn't test this part. I did try to make sure
my changes make sense, and I still don't see why this wouldn't be
correct. dsmark increments sch->q.qlen in enqueue, so it needs to
adjust it in dsmark_graft if the old queue is non-empty. If this is
wrong, please enlighten me so I can fix my patch.

Best regards,
Patrick


jamal wrote:

>Did you test some of this stuff or just did a mass-edit? I havent paid
>attention to all the details, but what would decrementing sch->q.qlen on
>grafting mean on a dsmark? 
>
>cheers,
>jamal
>
>  
>

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[jamal]

Patrick,

It doesnt make sense to have more than one queue in Dsmark (used as a
place holder/work queue while DSCP remarking). 
If reset() is already setting the length to 0 then it is not useful
to set it to anything else (it may actually become a -ve number).
I would suggest you use some of the examples in
iproute2/examples/diffserv to test things out (for all your changes -
the dsmark change was staring at me in the face - i didnt pay attention
to the rest).

cheers,
jamal

On Mon, 2003-11-17 at 09:40, Patrick McHardy wrote:
> Hi Jamal,
> 
> I've tested tbf and prio changes before and after. Unfortunately I've 
> never used dsmark so I didn't test this part. I did try to make sure
> my changes make sense, and I still don't see why this wouldn't be
> correct. dsmark increments sch->q.qlen in enqueue, so it needs to
> adjust it in dsmark_graft if the old queue is non-empty. If this is
> wrong, please enlighten me so I can fix my patch.
> 
> Best regards,
> Patrick
> 
> 
> jamal wrote:
> 
> >Did you test some of this stuff or just did a mass-edit? I havent paid
> >attention to all the details, but what would decrementing sch->q.qlen on
> >grafting mean on a dsmark? 
> >
> >cheers,
> >jamal
> >
> >  
> >
> 
> 
> 

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

Hi Jamal,

the patch was meant to fix the problem that when replacing (or deleting)
a leaf queue that still holds packets the packets are not subtracted
from the upper queue's q.qlen. dsmark_reset does set the length to 0 but
it is not called when grafting. I've now verified experimentally the
problem also exists in dsmark:

# tc qdisc add dev eth0 root handle 1: dsmark indices 64
# tc qdisc add dev eth0 parent 1: pfifo limit 100
# netperf -H 192.168.0.1
< queue is holding 38 packets at this moment >
# tc qdisc del dev eth0 parent 1: pfifo
# tc qdisc add dev eth0 parent 1: pfifo limit 100
# tc -s -d qdisc show dev eth0

qdisc pfifo 800d: limit 100p
 Sent 296729221 bytes 854680 pkts (dropped 0, overlimits 0)
 backlog 33p
                                                                                
qdisc dsmark 1: indices 0x0040
 Sent 510414473 bytes 1489414 pkts (dropped 40, overlimits 0)
 backlog 71p

While it may not be a common operation or not even make sense, it is a
valid one and IMHO should be handled correctly. It actually did happen
to me while playing with TBF. I missed dsmark only has a single queue
so sch->q.qlen = 0 would be ok in dsmark_graft. If you're ok with that,
I'm going to change the patch.

Best regards,
Patrick


jamal wrote:

>Patrick,
>
>It doesnt make sense to have more than one queue in Dsmark (used as a
>place holder/work queue while DSCP remarking). 
>If reset() is already setting the length to 0 then it is not useful
>to set it to anything else (it may actually become a -ve number).
>I would suggest you use some of the examples in
>iproute2/examples/diffserv to test things out (for all your changes -
>the dsmark change was staring at me in the face - i didnt pay attention
>to the rest).
>
>cheers,
>jamal
>  
>

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[jamal]

Patrick,

What happens if you add another type of qdisc example RED (after you
have deleted pfifo? Not that it makes a lot of sense to add anything
than a simple FIFO or even makes sense to add a leaf qdisc to begin
with).
My opinion is that since these interfaces (ex qdisc) exist they
exist to be (ab)used. 
Dsmark happens to work well with the single queue - maybe thats what
needs to be documented as opposed to saying it deviates from a purist
angle.

Lets hear what the author of dsmark says. Werner?

cheers,
jamal

PS: Again please note i didnt pay close attention to your other changes;
so test them accordingly.

On Mon, 2003-11-17 at 11:13, Patrick McHardy wrote:
> Hi Jamal,
> 
> the patch was meant to fix the problem that when replacing (or deleting)
> a leaf queue that still holds packets the packets are not subtracted
> from the upper queue's q.qlen. dsmark_reset does set the length to 0 but
> it is not called when grafting. I've now verified experimentally the
> problem also exists in dsmark:
> 
> # tc qdisc add dev eth0 root handle 1: dsmark indices 64
> # tc qdisc add dev eth0 parent 1: pfifo limit 100
> # netperf -H 192.168.0.1
> < queue is holding 38 packets at this moment >
> # tc qdisc del dev eth0 parent 1: pfifo
> # tc qdisc add dev eth0 parent 1: pfifo limit 100
> # tc -s -d qdisc show dev eth0
> 
> qdisc pfifo 800d: limit 100p
>  Sent 296729221 bytes 854680 pkts (dropped 0, overlimits 0)
>  backlog 33p
>                                                                                 
> qdisc dsmark 1: indices 0x0040
>  Sent 510414473 bytes 1489414 pkts (dropped 40, overlimits 0)
>  backlog 71p
> 
> While it may not be a common operation or not even make sense, it is a
> valid one and IMHO should be handled correctly. It actually did happen
> to me while playing with TBF. I missed dsmark only has a single queue
> so sch->q.qlen = 0 would be ok in dsmark_graft. If you're ok with that,
> I'm going to change the patch.
> 
> Best regards,
> Patrick
> 
> 
> jamal wrote:
> 
> >Patrick,
> >
> >It doesnt make sense to have more than one queue in Dsmark (used as a
> >place holder/work queue while DSCP remarking). 
> >If reset() is already setting the length to 0 then it is not useful
> >to set it to anything else (it may actually become a -ve number).
> >I would suggest you use some of the examples in
> >iproute2/examples/diffserv to test things out (for all your changes -
> >the dsmark change was staring at me in the face - i didnt pay attention
> >to the rest).
> >
> >cheers,
> >jamal
> >  
> >
> 
> 
> 

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

jamal wrote:

>Patrick,
>
>What happens if you add another type of qdisc example RED (after you
>have deleted pfifo? Not that it makes a lot of sense to add anything
>than a simple FIFO or even makes sense to add a leaf qdisc to begin
>with).
>  
>
Exactly the same. dsmark's counter is damaged the moment I delete the
pfifo qdisc holding packets.


>My opinion is that since these interfaces (ex qdisc) exist they
>exist to be (ab)used. 
>Dsmark happens to work well with the single queue - maybe thats what
>needs to be documented as opposed to saying it deviates from a purist
>angle.
>  
>
As I said I've never used dsmark, I changed it while fixing TBF (where
it is a problem, TBF was changed not long ago to support attaching inner
qdiscs), so anything is fine with me. I just became a purist defending
correctness of the patch ;)

Best regards,
Patrick

>Lets hear what the author of dsmark says. Werner?
>
>cheers,
>jamal
>
>PS: Again please note i didnt pay close attention to your other changes;
>so test them accordingly.
>
>On Mon, 2003-11-17 at 11:13, Patrick McHardy wrote:
>  
>
>>Hi Jamal,
>>
>>the patch was meant to fix the problem that when replacing (or deleting)
>>a leaf queue that still holds packets the packets are not subtracted
>>from the upper queue's q.qlen. dsmark_reset does set the length to 0 but
>>it is not called when grafting. I've now verified experimentally the
>>problem also exists in dsmark:
>>
>># tc qdisc add dev eth0 root handle 1: dsmark indices 64
>># tc qdisc add dev eth0 parent 1: pfifo limit 100
>># netperf -H 192.168.0.1
>>< queue is holding 38 packets at this moment >
>># tc qdisc del dev eth0 parent 1: pfifo
>># tc qdisc add dev eth0 parent 1: pfifo limit 100
>># tc -s -d qdisc show dev eth0
>>
>>qdisc pfifo 800d: limit 100p
>> Sent 296729221 bytes 854680 pkts (dropped 0, overlimits 0)
>> backlog 33p
>>                                                                                
>>qdisc dsmark 1: indices 0x0040
>> Sent 510414473 bytes 1489414 pkts (dropped 40, overlimits 0)
>> backlog 71p
>>
>>While it may not be a common operation or not even make sense, it is a
>>valid one and IMHO should be handled correctly. It actually did happen
>>to me while playing with TBF. I missed dsmark only has a single queue
>>so sch->q.qlen = 0 would be ok in dsmark_graft. If you're ok with that,
>>I'm going to change the patch.
>>
>>Best regards,
>>Patrick
>>
>>
>>jamal wrote:
>>
>>    
>>
>>>Patrick,
>>>
>>>It doesnt make sense to have more than one queue in Dsmark (used as a
>>>place holder/work queue while DSCP remarking). 
>>>If reset() is already setting the length to 0 then it is not useful
>>>to set it to anything else (it may actually become a -ve number).
>>>I would suggest you use some of the examples in
>>>iproute2/examples/diffserv to test things out (for all your changes -
>>>the dsmark change was staring at me in the face - i didnt pay attention
>>>to the rest).
>>>
>>>cheers,
>>>jamal
>>> 
>>>
>>>      
>>>
>>
>>    
>>

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[jamal]

On Mon, 2003-11-17 at 13:15, Patrick McHardy wrote:

> >My opinion is that since these interfaces (ex qdisc) exist they
> >exist to be (ab)used. 
> >Dsmark happens to work well with the single queue - maybe thats what
> >needs to be documented as opposed to saying it deviates from a purist
> >angle.
> >  
> >
> As I said I've never used dsmark, I changed it while fixing TBF (where
> it is a problem, TBF was changed not long ago to support attaching inner
> qdiscs), so anything is fine with me. I just became a purist defending
> correctness of the patch ;)

Now that we have invoked Werner - lets wait for his response then ;->
Sorry, didnt meant to drag this into 5-6 emails thread.
I closely looked at your other changes on that patch and they looked
fine.

cheers,
jamal

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[Werner Almesberger]

jamal wrote:
> My opinion is that since these interfaces (ex qdisc) exist they
> exist to be (ab)used. 

Stated in such a general way, that sounds dangerous ;-)

> Dsmark happens to work well with the single queue - maybe thats what
> needs to be documented as opposed to saying it deviates from a purist
> angle.

tcio, section 2, towards the end:

| Typically, each class ``owns'' one queue, but it is in principle
| also possible that several classes share the same queue or even
| that a single queue is used by all classes of the respective
| queuing discipline.

Now, I've of course only documented the status quo, and one could
argue whether this actually makes sense. But then, it works well
for dsmark, and doesn't seem to break anything else.

I'm a bit confused about what the problem is here (or if there is
one at all): if dsmark_graft is changed to adjust the statistics
(which makes perfect sense to me), I don't see a reason why either
subtracting the queue length of the former inner qdisc, or simply
setting dsmark's queue lenghth to zero wouldn't work.

Patrick, was the experiment "I've now verified experimentally the
problem also exists in dsmark" done with or without your
"sch->q.qlen -= (*old)->q.qlen;" patch ? This looks just like the
behaviour I'd expect your patch to fix, no ?

- Werner

-- 
  _________________________________________________________________________
 / Werner Almesberger, Buenos Aires, Argentina     werner@almesberger.net /
/_http://www.almesberger.net/____________________________________________/

Re: [PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

Werner Almesberger wrote:

>Patrick, was the experiment "I've now verified experimentally the
>problem also exists in dsmark" done with or without your
>"sch->q.qlen -= (*old)->q.qlen;" patch ? This looks just like the
>behaviour I'd expect your patch to fix, no ?
>  
>
Correct, it was done without the patch and the patch is supposed to fix
it. I'm going to send a new version which just sets q.qlen to 0 later.

Best regards,
Patrick


>- Werner
>
>  
>

[UPDATED PATCH]: Adjust qlen when grafting in multiple qdiscs

[Patrick McHardy]

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

Hi Dave,

this patch fixes multiple qdiscs to adjust sch->q.qlen when
replacing a non-empty inner qdisc. The only change to the
first patch is in dsmark_graft: this patch directly sets
sch->q.qlen to 0 instead of subtracting the length of the
inner qdisc.

Best regards,
Patrick



[-- Attachment #2: graft-adjust-qlen.diff --]
[-- Type: text/plain, Size: 1974 bytes --]

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1441  -> 1.1442 
#	 net/sched/sch_tbf.c	1.10    -> 1.11   
#	 net/sched/sch_cbq.c	1.13    -> 1.14   
#	net/sched/sch_dsmark.c	1.10    -> 1.11   
#	net/sched/sch_prio.c	1.9     -> 1.10   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/11/18	kaber@trash.net	1.1442
# Adjust qlen when grafting in multiple qdiscs
# --------------------------------------------
#
diff -Nru a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
--- a/net/sched/sch_cbq.c	Tue Nov 18 01:18:14 2003
+++ b/net/sched/sch_cbq.c	Tue Nov 18 01:18:14 2003
@@ -1670,6 +1670,7 @@
 		sch_tree_lock(sch);
 		*old = cl->q;
 		cl->q = new;
+		sch->q.qlen -= (*old)->q.qlen;
 		qdisc_reset(*old);
 		sch_tree_unlock(sch);
 
diff -Nru a/net/sched/sch_dsmark.c b/net/sched/sch_dsmark.c
--- a/net/sched/sch_dsmark.c	Tue Nov 18 01:18:14 2003
+++ b/net/sched/sch_dsmark.c	Tue Nov 18 01:18:14 2003
@@ -77,6 +77,7 @@
 	*old = xchg(&p->q,new);
 	if (*old)
 		qdisc_reset(*old);
+	sch->q.qlen = 0;
 	sch_tree_unlock(sch); /* @@@ move up ? */
         return 0;
 }
diff -Nru a/net/sched/sch_prio.c b/net/sched/sch_prio.c
--- a/net/sched/sch_prio.c	Tue Nov 18 01:18:14 2003
+++ b/net/sched/sch_prio.c	Tue Nov 18 01:18:14 2003
@@ -266,6 +266,7 @@
 	sch_tree_lock(sch);
 	*old = q->queues[band];
 	q->queues[band] = new;
+	sch->q.qlen -= (*old)->q.qlen;
 	qdisc_reset(*old);
 	sch_tree_unlock(sch);
 
diff -Nru a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
--- a/net/sched/sch_tbf.c	Tue Nov 18 01:18:14 2003
+++ b/net/sched/sch_tbf.c	Tue Nov 18 01:18:14 2003
@@ -455,6 +455,8 @@
 	sch_tree_lock(sch);	
 	*old = xchg(&q->qdisc, new);
 	qdisc_reset(*old);
+	sch->q.qlen = 0;
+	sch->stats.backlog = 0;
 	sch_tree_unlock(sch);
 	
 	return 0;

Re: [UPDATED PATCH]: Adjust qlen when grafting in multiple qdiscs

[David S. Miller]

On Tue, 18 Nov 2003 01:34:56 +0100
Patrick McHardy <kaber@trash.net> wrote:

> this patch fixes multiple qdiscs to adjust sch->q.qlen when
> replacing a non-empty inner qdisc. The only change to the
> first patch is in dsmark_graft: this patch directly sets
> sch->q.qlen to 0 instead of subtracting the length of the
> inner qdisc.

Applied, thanks Patrick.

I'll stick this into my 2.4.24-pre1 queue as well.

Thanks again.