From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-bk0-f43.google.com (mail-bk0-f43.google.com [209.85.214.43]) (using TLSv1 with cipher RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority" (verified OK)) by huchra.bufferbloat.net (Postfix) with ESMTPS id 73463202213 for ; Fri, 4 May 2012 01:56:07 -0700 (PDT) Received: by bkty5 with SMTP id y5so4559014bkt.16 for ; Fri, 04 May 2012 01:56:04 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:from:to:cc:in-reply-to:references:content-type:date :message-id:mime-version:x-mailer:content-transfer-encoding; bh=Qc50LzzRrJ2cPGuZVCGN1N28jJjZgqrBn7O3jclieJI=; b=Uf7fUghBIT6jP//ImcQgGF4ARTtjZYS5fN/+RRzV0etPvBbhqafuYLOP49vJvWyV4J Gq5djXbDsiAdoW0hHZXJBZrMa8EdacIinWyayab4+j2cEfgJSAsESH9CL11s1re341jT ZzLIo4dNe77iVVFgXu6beG7CreQb90hZ5e045dSnTYFSryfSh+bbKVuTaj7Lpv+BrWUU M95nBySDY4Ghs6vfb4s36dsp/7gNQ34Q0hGSl3OwRjKnNeWBV/XU7R2fC6fs6LCpRh3x 9CPlGhkTrpIj2VWFaWQXFvuRSdS6H8mgmm3O7fBerZagLcazaTZHfQVSE+Nr3lFmi3UR HFkg== Received: by 10.204.157.151 with SMTP id b23mr1758124bkx.44.1336121764851; Fri, 04 May 2012 01:56:04 -0700 (PDT) Received: from [172.28.131.195] ([74.125.122.49]) by mx.google.com with ESMTPS id z17sm15418149bkw.12.2012.05.04.01.56.02 (version=SSLv3 cipher=OTHER); Fri, 04 May 2012 01:56:03 -0700 (PDT) From: Eric Dumazet To: Dave =?ISO-8859-1?Q?T=E4ht?= In-Reply-To: <1336116589-18550-1-git-send-email-dave.taht@bufferbloat.net> References: <1336116589-18550-1-git-send-email-dave.taht@bufferbloat.net> Content-Type: text/plain; charset="UTF-8" Date: Fri, 04 May 2012 10:56:01 +0200 Message-ID: <1336121761.3752.115.camel@edumazet-glaptop> Mime-Version: 1.0 X-Mailer: Evolution 2.28.3 Content-Transfer-Encoding: 8bit Cc: codel@lists.bufferbloat.net Subject: Re: [Codel] [PATCH] codel take 2 X-BeenThere: codel@lists.bufferbloat.net X-Mailman-Version: 2.1.13 Precedence: list List-Id: CoDel AQM discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 May 2012 08:56:08 -0000 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.