Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Taht <dave.taht@gmail.com>]

On Sun, Jun 17, 2012 at 10:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-06-17 at 15:30 -0700, Dave Täht wrote:
>> Lossless IP networks are not possible. Dropping packets is the
>> fastest way to get back to a target delay, and if that involves
>> dropping ECN marked packets, so be it. When a network is in a
>> more steady state, ECN is fine...
>>
>
> Just disable ecn then ? By the way ECN is disabled by default on CoDel.

I LIKE ecn. I like it enabled by default on fq_codel. I wouldn't mind if it
was enabled by default on everything... if we knew how to handle it.

>> My choice of "2 * target" as a threshold for dropping ECN
>> marked packets is entirely arbitrary.
>> ---
>>  include/net/codel.h |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/codel.h b/include/net/codel.h
>> index 550debf..26944a0 100644
>> --- a/include/net/codel.h
>> +++ b/include/net/codel.h
>> @@ -280,7 +280,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
>>                                               * since there is no more divide
>>                                               */
>>                               codel_Newton_step(vars);
>> -                             if (params->ecn && INET_ECN_set_ce(skb)) {
>> +                             if (params->ecn && INET_ECN_set_ce(skb) &&
>> +                                     vars->ldelay <= 2 * params->target) {
>>                                       stats->ecn_mark++;
>>                                       vars->drop_next =
>>                                               codel_control_law(vars->drop_next,
>> @@ -305,7 +306,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
>>                       }
>>               }
>>       } else if (drop) {
>> -             if (params->ecn && INET_ECN_set_ce(skb)) {
>> +             if (params->ecn && INET_ECN_set_ce(skb) &&
>> +                     vars->ldelay <= 2 * params->target) {
>>                       stats->ecn_mark++;
>>               } else {
>>                       qdisc_drop(skb, sch);
>
> Hmm, while I understand the idea, patch is wrong.

so we are in agreement tho that ECN packets should be dropped in
some circumstances?

> if (A && B && C)
>
> Even if (C) is false, but A is true, B is evaluated.

A is always true (or always false)
B in this case is usually false except on the kinds of ECN-heavy
workloads that exposed this issue
and C is probably higher overhead than A or B

So a better conditional is possible...

> Also, a 2 * target value is arbitrary, why not instead provide a real
> attribute, that user can set with tc command ?

In part, that's why this is an RFC. I didn't want to introduce a tc
change at this late date (tho it can be done) with all the binary packages
(openwrt, fedora, ubuntu) that just landed. I did want to open up a
discussion of what would be sane - is 2 * target sane? 8 * target sane? or
thinking about sojourn time a little harder sane? Or for example, like SFB,
identifying unresponsive flows, instead, sane.

I'm allergic to knobs, too. So...

my next patch was going to be to the ns3 sim unless andrew beat me to it.

(side note, I noticed fq_codel defaulted to 10k packets which is
rather excessive for tiny routers - I just trimmed that down
significantly for cerowrt and the upcoming 3.3.8-4 release has the rfc
patch in it)

And apologies for not seeing this long ago,

I'm only just now catching up on things somewhat, I hope we can do
some benchmarks at various bandwidths and be able to see what works
well
under a variety of conditions.

>
>
> _______________________________________________
> Codel mailing list
> Codel@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/codel



-- 
Dave Täht
SKYPE: davetaht
http://ronsravings.blogspot.com/