Re: [Codel] [RFCv2 PATCH] codel: add ecn_target for when to drop rather than mark ecn packets

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

On Mon, Jun 25, 2012 at 8:20 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2012-06-25 at 07:50 -0700, Dave Taht wrote:
>> On Sun, Jun 24, 2012 at 10:22 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > On Sun, 2012-06-24 at 22:00 -0700, Dave Täht wrote:
>> >> From: Dave Taht <dave.taht@bufferbloat.net>
>
>> >>                               codel_Newton_step(vars);
>> >> -                             if (params->ecn && INET_ECN_set_ce(skb)) {
>> >> +                             if (params->ecn &&
>> >> +                                 params->ecn_target > vars->ldelay &&
>> >
>> > Wrong test ?
>> >
>> > We want ECN if delay < ecn_target, not if delay > ecn_target
>> >
>> > (unresponsive flows will make delay being above ecn_target, while
>> > responsive ones should make delay more like target ( < en_target)
>> >
>> > if (params->ecn &&
>> >    vars->ldelay <= params->ecn_target &&
>> >    INET_ECN_set_ce(skb)) {
>>
>> The orig is the same test, as params->ecn_target > vars->ldelay
>> is equivalent to vars->ldelay <= params->ecn_target
>
> Yes, I must say I dont like the testing of a variable against a LIMIT
> using
>
>  if (LIMIT > variable)
>
> I prefer for readability
>
>  if (variable < LIMIT)
>
> If you take a look at linux code, your form is very seldom used.
>
>> My own mental debate was whether to switch ecn from a bool to being a
>> codel_time_t,
>> and use a value of 0 for noecn and whatever for the ecn target value.
>
> Well, why not.

It was the way I was leaning until I observed me dropping ecn enabled
mosh, ssh, and babel packets , which tend to be small, so I started
thinking in terms of dropping ecn packets on a graduated packet size
scale after exceeding target. ESPECIALLY in case of overload you want
command and control packets to get through.

... So I figured getting another RFC out was a good idea. O brave new
world that has such new drop strategies in it!


>
>



-- 
Dave Täht
http://www.bufferbloat.net/projects/cerowrt/wiki - "3.3.8-6 is out
with fq_codel!"