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

[Patrick McHardy <kaber@xxxxxxxxx>]

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
>