: Adjust qlen when grafting in multiple qdiscs

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 [email protected] 1.1428
# Adjust qlen when grafting in multiple qdiscs # -------------------------------------------- #
diff -Nru a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c

net/sched/sch_cbq.c +1 -0

@@ -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

net/sched/sch_dsmark.c +3 -1

@@ -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

net/sched/sch_prio.c +1 -0

@@ -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

net/sched/sch_tbf.c +2 -0

@@ -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;

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

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

Quoted 9 lines · 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

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

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

Quoted 15 lines · 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

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.

Quoted 57 lines · 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

Quoted 16 lines · 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

Quoted 7 lines

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 ;)

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.

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

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.