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

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

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>
>>
>> ECN can be gamed, and it is generally faster under overload
>> to drop rather than mark packets, to get back to a target.
>>
>> This patch adds support for ecn_target which controls when
>> codel and fq_codel will start dropping rather than marking
>> ecn tagged packets.
>>
>> The default is 15ms.
>> ---
>>  include/linux/pkt_sched.h |    2 ++
>>  include/net/codel.h       |   11 +++++++++--
>>  net/sched/sch_codel.c     |   13 ++++++++++++-
>>  net/sched/sch_fq_codel.c  |   12 +++++++++++-
>>  4 files changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
>> index 32aef0a..f0dcf8c 100644
>> --- a/include/linux/pkt_sched.h
>> +++ b/include/linux/pkt_sched.h
>> @@ -663,6 +663,7 @@ enum {
>>       TCA_CODEL_LIMIT,
>>       TCA_CODEL_INTERVAL,
>>       TCA_CODEL_ECN,
>> +     TCA_CODEL_ECN_TARGET,
>>       __TCA_CODEL_MAX
>>  };
>>
>> @@ -691,6 +692,7 @@ enum {
>>       TCA_FQ_CODEL_ECN,
>>       TCA_FQ_CODEL_FLOWS,
>>       TCA_FQ_CODEL_QUANTUM,
>> +     TCA_FQ_CODEL_ECN_TARGET,
>>       __TCA_FQ_CODEL_MAX
>>  };
>>
>> diff --git a/include/net/codel.h b/include/net/codel.h
>> index 550debf..8a6a2b0 100644
>> --- a/include/net/codel.h
>> +++ b/include/net/codel.h
>> @@ -111,11 +111,13 @@ static inline u32 codel_time_to_us(codel_time_t val)
>>   * @target:  target queue size (in time units)
>>   * @interval:        width of moving time window
>>   * @ecn:     is Explicit Congestion Notification enabled
>> + * @ecn_target:      ecn target for queue drop anyway (in time units)
>>   */
>>  struct codel_params {
>>       codel_time_t    target;
>>       codel_time_t    interval;
>>       bool            ecn;
>> +     codel_time_t    ecn_target;
>>  };
>>
>>  /**
>> @@ -161,6 +163,7 @@ static void codel_params_init(struct codel_params *params)
>>       params->interval = MS2TIME(100);
>>       params->target = MS2TIME(5);
>>       params->ecn = false;
>> +     params->ecn_target = MS2TIME(15);
>>  }
>>
>>  static void codel_vars_init(struct codel_vars *vars)
>> @@ -280,7 +283,9 @@ 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 &&
>> +                                 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

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.

>
>> +                                     INET_ECN_set_ce(skb)) {
>>                                       stats->ecn_mark++;
>>                                       vars->drop_next =
>>                                               codel_control_law(vars->drop_next,
>> @@ -305,7 +310,9 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
>>                       }
>>               }
>>       } else if (drop) {
>> -             if (params->ecn && INET_ECN_set_ce(skb)) {
>> +             if (params->ecn &&
>> +                 params->ecn_target > vars->ldelay &&
>> +                 INET_ECN_set_ce(skb)) {
>>                       stats->ecn_mark++;
>>               } else {
>>                       qdisc_drop(skb, sch);
>> diff --git a/net/sched/sch_codel.c b/net/sched/sch_codel.c
>> index 2f9ab17..b008a35 100644
>> --- a/net/sched/sch_codel.c
>> +++ b/net/sched/sch_codel.c
>> @@ -109,6 +109,7 @@ static const struct nla_policy codel_policy[TCA_CODEL_MAX + 1] = {
>>       [TCA_CODEL_LIMIT]       = { .type = NLA_U32 },
>>       [TCA_CODEL_INTERVAL]    = { .type = NLA_U32 },
>>       [TCA_CODEL_ECN]         = { .type = NLA_U32 },
>> +     [TCA_CODEL_ECN_TARGET]  = { .type = NLA_U32 },
>>  };
>>
>>  static int codel_change(struct Qdisc *sch, struct nlattr *opt)
>> @@ -133,6 +134,13 @@ static int codel_change(struct Qdisc *sch, struct nlattr *opt)
>>               q->params.target = ((u64)target * NSEC_PER_USEC) >> CODEL_SHIFT;
>>       }
>>
>> +     if (tb[TCA_CODEL_ECN_TARGET]) {
>> +             u32 target = nla_get_u32(tb[TCA_CODEL_TARGET]);
>> +
>> +             q->params.ecn_target = ((u64)target * NSEC_PER_USEC) >>
>> +                                     CODEL_SHIFT;
>> +     }
>> +
>>       if (tb[TCA_CODEL_INTERVAL]) {
>>               u32 interval = nla_get_u32(tb[TCA_CODEL_INTERVAL]);
>>
>> @@ -145,6 +153,7 @@ static int codel_change(struct Qdisc *sch, struct nlattr *opt)
>>       if (tb[TCA_CODEL_ECN])
>>               q->params.ecn = !!nla_get_u32(tb[TCA_CODEL_ECN]);
>>
>> +
>
> extra new line ?
>
>>       qlen = sch->q.qlen;
>>       while (sch->q.qlen > sch->limit) {
>>               struct sk_buff *skb = __skb_dequeue(&sch->q);
>> @@ -199,7 +208,9 @@ static int codel_dump(struct Qdisc *sch, struct sk_buff *skb)
>>           nla_put_u32(skb, TCA_CODEL_INTERVAL,
>>                       codel_time_to_us(q->params.interval)) ||
>>           nla_put_u32(skb, TCA_CODEL_ECN,
>> -                     q->params.ecn))
>> +                     q->params.ecn) ||
>> +         nla_put_u32(skb, TCA_CODEL_ECN_TARGET,
>> +                     codel_time_to_us(q->params.ecn_target)))
>>               goto nla_put_failure;
>>
>>       return nla_nest_end(skb, opts);
>> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
>> index 9fc1c62..d974e07 100644
>> --- a/net/sched/sch_fq_codel.c
>> +++ b/net/sched/sch_fq_codel.c
>> @@ -297,6 +297,7 @@ static const struct nla_policy fq_codel_policy[TCA_FQ_CODEL_MAX + 1] = {
>>       [TCA_FQ_CODEL_ECN]      = { .type = NLA_U32 },
>>       [TCA_FQ_CODEL_FLOWS]    = { .type = NLA_U32 },
>>       [TCA_FQ_CODEL_QUANTUM]  = { .type = NLA_U32 },
>> +     [TCA_FQ_CODEL_ECN_TARGET]       = { .type = NLA_U32 },
>>  };
>>
>>  static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt)
>> @@ -327,6 +328,13 @@ static int fq_codel_change(struct Qdisc *sch, struct nlattr *opt)
>>               q->cparams.target = (target * NSEC_PER_USEC) >> CODEL_SHIFT;
>>       }
>>
>> +     if (tb[TCA_FQ_CODEL_ECN_TARGET]) {
>> +             u64 target = nla_get_u32(tb[TCA_FQ_CODEL_TARGET]);
>> +
>> +             q->cparams.ecn_target = (target * NSEC_PER_USEC) >>
>> +                                     CODEL_SHIFT;
>> +     }
>> +
>>       if (tb[TCA_FQ_CODEL_INTERVAL]) {
>>               u64 interval = nla_get_u32(tb[TCA_FQ_CODEL_INTERVAL]);
>>
>> @@ -447,7 +455,9 @@ static int fq_codel_dump(struct Qdisc *sch, struct sk_buff *skb)
>>           nla_put_u32(skb, TCA_FQ_CODEL_QUANTUM,
>>                       q->quantum) ||
>>           nla_put_u32(skb, TCA_FQ_CODEL_FLOWS,
>> -                     q->flows_cnt))
>> +                     q->flows_cnt) ||
>> +         nla_put_u32(skb, TCA_FQ_CODEL_ECN_TARGET,
>> +                     codel_time_to_us(q->cparams.ecn_target)))
>>               goto nla_put_failure;
>>
>>       nla_nest_end(skb, opts);
>
>
> _______________________________________________
> Codel mailing list
> Codel@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/codel



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