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

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

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)) {


> +					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);