Re: [Codel] [PATCH] codel take 2

[Eric Dumazet <eric.dumazet@gmail.com>]

On Fri, 2012-05-04 at 00:29 -0700, Dave Täht wrote:
> I took most of the suggestions from the code review.
> 
> This attempthas some debugging code left in it. It looks logically correct,
> and does exercise the various substates, does drop packets, and
> does control delay. On a 2mbit htb setup like this:
> 
> tc qdisc add dev eth0 root handle 1: est 1sec 8sec htb default 1
> tc class add dev eth0 parent 1: classid 1:1 est 1sec 8sec htb \
> rate 2000kibit mtu 1500 quantum 1514
> tc qdisc add dev eth0 parent 1:1 handle 10: est 1sec 4sec
>         codel target 5ms interval 100ms depth 1000
> 

The usual keyword for tc qdisc is "limit 1000", not "depth 1000"

(CoDel has no depth mentioned at all, so I suspect this can be changed)

> talking to some servers a few ms away via netperf,
> 
> I see it holding delays below 30ms.
> 
> This does not mean that I have the constants or units perfect.
> 
> Also for giggles I setup codel as a sub qdisc of qfq, and it worked.
> 
> Lastly I ran several dozen GB through it, and pfifo fast on a fast
> 86_64 box and saw roughly comparable throughput for both.
> 
> I get mildly better results with TSO and GSO off.

Since this code doesnt compile on 32bit arch as is and is not on based
on net-next, its a bit convoluted for me right now.

NLA_PUT : Its not that hard to get rid of it. 

if (nla_put(skb, TCA_OPTIONS, sizeof(opt), &opt))
	goto nla_put_failure;

and the 64 bit divide doesnt work on 32bit, you need do_div() helper.

ERROR: "__divdi3" [net/sched/sch_codel.ko] undefined!

Are you sure interval should be an s64 and not an u64 ?
(But my personal choice would be an unsigned long)

	u64 interval = q->interval;

	do_div(interval, int_sqrt(q->count));
	return ktime_add_ns(t, interval);

But a divide is expensive, and do_div() is _really_ expensive on 32bit.
So please prefer an "unsigned long" (max 4 second interval) and avoid
the do_div() game. If 4 sec limit is too low, change ns resolution to
us. Again it would be my personal choice.

same 64bit divide problem in codel_dump()

u64 interval = q->interval;
do_div(interval, NSEC_PER_USEC);
opt.interval = interval;

1000000 in codel_init() should be replaced by NSEC_PER_MSEC
(neat constants from include/linux/time.h)

The "char data[16];" in struct codel_skb_cb is of no use, please remove
it.

You have non ASCII chars to code the ' char in comments ...
Please remove them.

Last : about non conserving stuff, this is because you must call
qdisc_tree_decrease_qlen(sch, number_of_packets_dropped) in
codel_dequeue(), or else your parents has no idea of what happened.

Note : Do that once (batch), not for each packet, because
qdisc_tree_decrease_qlen() is really expensive since it does a linear
search to find parents (we dont have pointers to parents). If your
machine has 1000 qdisc, its slow as hell.