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

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

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.