8 messages, 2 authors, 2003-11-17 · open the first message on its own page
From: Patrick McHardy <hidden>
Date: 2003-11-13 14:47:06
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
From: jamal <hidden>
Date: 2003-11-17 14:22:01
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;
From: Patrick McHardy <hidden>
Date: 2003-11-17 14:40:32
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
From: jamal <hidden>
Date: 2003-11-17 14:59:13
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:quoted
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
From: Patrick McHardy <hidden>
Date: 2003-11-17 16:13:51
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
From: jamal <hidden>
Date: 2003-11-17 17:30:10
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:quoted
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
From: Patrick McHardy <hidden>
Date: 2003-11-17 18:15:24
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:quoted
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:quoted
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
From: jamal <hidden>
Date: 2003-11-17 19:25:01
On Mon, 2003-11-17 at 13:15, Patrick McHardy wrote:
quoted
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