[Codel] [RFCv2 PATCH] iproute2: Add ecn_target option to codel and fq_codel

[Codel] [RFCv2 PATCH] iproute2: Add ecn_target option to codel and fq_codel

[Dave Täht]

From: Dave Taht <dave.taht@bufferbloat.net>

ECN was not part of the original codel design and adding support
for it revealed problems in two areas. 1) ECN can be gamed.
2) Dropping packets under overload more rapidly frees up
bandwidth than marking packets.

Two possible scenarios of use - on egress from a network,
ecn_target could be set low, to drop more often, to
ensure lowest latency for other packets.

On ingress, it could be set high to mark packets more often,
to lower data loss while still signalling the end application
that bandwidth is a problem.

ecn_target is not engaged until after codel enters a dropping
state overall.
---
 include/linux/pkt_sched.h |    2 ++
 man/man8/tc-codel.8       |    3 +++
 man/man8/tc-fq_codel.8    |    5 +++++
 tc/q_codel.c              |   17 ++++++++++++++++-
 tc/q_fq_codel.c           |   17 ++++++++++++++++-
 5 files changed, 42 insertions(+), 2 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/man/man8/tc-codel.8 b/man/man8/tc-codel.8
index 61f163f..0e97249 100644
--- a/man/man8/tc-codel.8
+++ b/man/man8/tc-codel.8
@@ -64,6 +64,9 @@ is the acceptable minimum standing/persistent queue delay. This minimum delay
 is identified by tracking the local minimum queue delay that packets experience.
 Default and recommended value is 5ms.
 
+.SS ecn_target
+is the overall delay at which ecn marked packets will be dropped rather than marked.
+
 .SS interval
 is used to ensure that the measured minimum delay does not become too stale. The
 minimum delay must be experienced in the last epoch of length
diff --git a/man/man8/tc-fq_codel.8 b/man/man8/tc-fq_codel.8
index 8b43c10..9cb5be8 100644
--- a/man/man8/tc-fq_codel.8
+++ b/man/man8/tc-fq_codel.8
@@ -49,6 +49,11 @@ and is the acceptable minimum
 standing/persistent queue delay. This minimum delay is identified by tracking
 the local minimum queue delay that packets experience.  Default value is 5ms.
 
+.SS ecn_target
+has the same semantics as
+.B codel
+and is the delay beyond which ecn packets will be dropped rather than marked.
+
 .SS interval
 has the same semantics as
 .B codel
diff --git a/tc/q_codel.c b/tc/q_codel.c
index dc4b3f6..1400680 100644
--- a/tc/q_codel.c
+++ b/tc/q_codel.c
@@ -53,7 +53,7 @@
 
 static void explain(void)
 {
-	fprintf(stderr, "Usage: ... codel [ limit PACKETS ] [ target TIME]\n");
+	fprintf(stderr, "Usage: ... codel [ limit PACKETS ] [ target TIME] [ ecn_target TIME ]\n");
 	fprintf(stderr, "                 [ interval TIME ] [ ecn | noecn ]\n");
 }
 
@@ -62,6 +62,7 @@ static int codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 {
 	unsigned limit = 0;
 	unsigned target = 0;
+	unsigned ecn_target = 0;
 	unsigned interval = 0;
 	int ecn = -1;
 	struct rtattr *tail;
@@ -79,6 +80,12 @@ static int codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 				fprintf(stderr, "Illegal \"target\"\n");
 				return -1;
 			}
+		} else if (strcmp(*argv, "ecn_target") == 0) {
+			NEXT_ARG();
+			if (get_time(&ecn_target, *argv)) {
+				fprintf(stderr, "Illegal \"ecn_target\"\n");
+				return -1;
+			}
 		} else if (strcmp(*argv, "interval") == 0) {
 			NEXT_ARG();
 			if (get_time(&interval, *argv)) {
@@ -108,6 +115,8 @@ static int codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 		addattr_l(n, 1024, TCA_CODEL_INTERVAL, &interval, sizeof(interval));
 	if (target)
 		addattr_l(n, 1024, TCA_CODEL_TARGET, &target, sizeof(target));
+	if (ecn_target)
+		addattr_l(n, 1024, TCA_CODEL_ECN_TARGET, &ecn_target, sizeof(ecn_target));
 	if (ecn != -1)
 		addattr_l(n, 1024, TCA_CODEL_ECN, &ecn, sizeof(ecn));
 	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
@@ -120,6 +129,7 @@ static int codel_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	unsigned limit;
 	unsigned interval;
 	unsigned target;
+	unsigned ecn_target;
 	unsigned ecn;
 	SPRINT_BUF(b1);
 
@@ -138,6 +148,11 @@ static int codel_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 		target = rta_getattr_u32(tb[TCA_CODEL_TARGET]);
 		fprintf(f, "target %s ", sprint_time(target, b1));
 	}
+	if (tb[TCA_CODEL_ECN_TARGET] &&
+	    RTA_PAYLOAD(tb[TCA_CODEL_ECN_TARGET]) >= sizeof(__u32)) {
+		target = rta_getattr_u32(tb[TCA_CODEL_ECN_TARGET]);
+		fprintf(f, "ecn_target %s ", sprint_time(ecn_target, b1));
+	}
 	if (tb[TCA_CODEL_INTERVAL] &&
 	    RTA_PAYLOAD(tb[TCA_CODEL_INTERVAL]) >= sizeof(__u32)) {
 		interval = rta_getattr_u32(tb[TCA_CODEL_INTERVAL]);
diff --git a/tc/q_fq_codel.c b/tc/q_fq_codel.c
index 1d3bfa2..db14d52 100644
--- a/tc/q_fq_codel.c
+++ b/tc/q_fq_codel.c
@@ -51,7 +51,7 @@
 static void explain(void)
 {
 	fprintf(stderr, "Usage: ... fq_codel [ limit PACKETS ] [ flows NUMBER ]\n");
-	fprintf(stderr, "                    [ target TIME] [ interval TIME ]\n");
+	fprintf(stderr, "                    [ target TIME] [ ecn_target TIME ] [ interval TIME ]\n");
 	fprintf(stderr, "                    [ quantum BYTES ] [ [no]ecn ]\n");
 }
 
@@ -61,6 +61,7 @@ static int fq_codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	unsigned limit = 0;
 	unsigned flows = 0;
 	unsigned target = 0;
+	unsigned ecn_target = 0;
 	unsigned interval = 0;
 	unsigned quantum = 0;
 	int ecn = -1;
@@ -91,6 +92,12 @@ static int fq_codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 				fprintf(stderr, "Illegal \"target\"\n");
 				return -1;
 			}
+		} else if (strcmp(*argv, "ecn_target") == 0) {
+			NEXT_ARG();
+			if (get_time(&ecn_target, *argv)) {
+				fprintf(stderr, "Illegal \"ecn_target\"\n");
+				return -1;
+			}
 		} else if (strcmp(*argv, "interval") == 0) {
 			NEXT_ARG();
 			if (get_time(&interval, *argv)) {
@@ -124,6 +131,8 @@ static int fq_codel_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 		addattr_l(n, 1024, TCA_FQ_CODEL_INTERVAL, &interval, sizeof(interval));
 	if (target)
 		addattr_l(n, 1024, TCA_FQ_CODEL_TARGET, &target, sizeof(target));
+	if (ecn_target)
+		addattr_l(n, 1024, TCA_FQ_CODEL_ECN_TARGET, &ecn_target, sizeof(ecn_target));
 	if (ecn != -1)
 		addattr_l(n, 1024, TCA_FQ_CODEL_ECN, &ecn, sizeof(ecn));
 	tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail;
@@ -137,6 +146,7 @@ static int fq_codel_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt
 	unsigned flows;
 	unsigned interval;
 	unsigned target;
+	unsigned ecn_target;
 	unsigned ecn;
 	unsigned quantum;
 	SPRINT_BUF(b1);
@@ -166,6 +176,11 @@ static int fq_codel_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt
 		target = rta_getattr_u32(tb[TCA_FQ_CODEL_TARGET]);
 		fprintf(f, "target %s ", sprint_time(target, b1));
 	}
+	if (tb[TCA_FQ_CODEL_ECN_TARGET] &&
+	    RTA_PAYLOAD(tb[TCA_FQ_CODEL_ECN_TARGET]) >= sizeof(__u32)) {
+		target = rta_getattr_u32(tb[TCA_FQ_CODEL_ECN_TARGET]);
+		fprintf(f, "ecn_target %s ", sprint_time(ecn_target, b1));
+	}
 	if (tb[TCA_FQ_CODEL_INTERVAL] &&
 	    RTA_PAYLOAD(tb[TCA_FQ_CODEL_INTERVAL]) >= sizeof(__u32)) {
 		interval = rta_getattr_u32(tb[TCA_FQ_CODEL_INTERVAL]);
-- 
1.7.9.5

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

[Dave Täht]

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 &&
+					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]);
 
+
 	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);
-- 
1.7.9.5

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

[Eric Dumazet]

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

Re: [Codel] [RFCv2 PATCH] iproute2: Add ecn_target option to codel and fq_codel

[Eric Dumazet]

On Sun, 2012-06-24 at 22:00 -0700, Dave Täht wrote:
> From: Dave Taht <dave.taht@bufferbloat.net>
> 
> ECN was not part of the original codel design and adding support
> for it revealed problems in two areas. 1) ECN can be gamed.
> 2) Dropping packets under overload more rapidly frees up
> bandwidth than marking packets.
> 
> Two possible scenarios of use - on egress from a network,
> ecn_target could be set low, to drop more often, to
> ensure lowest latency for other packets.
> 
> On ingress, it could be set high to mark packets more often,
> to lower data loss while still signalling the end application
> that bandwidth is a problem.
> 
> ecn_target is not engaged until after codel enters a dropping
> state overall.
> ---
>  include/linux/pkt_sched.h |    2 ++
>  man/man8/tc-codel.8       |    3 +++
>  man/man8/tc-fq_codel.8    |    5 +++++
>  tc/q_codel.c              |   17 ++++++++++++++++-
>  tc/q_fq_codel.c           |   17 ++++++++++++++++-
>  5 files changed, 42 insertions(+), 2 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/man/man8/tc-codel.8 b/man/man8/tc-codel.8
> index 61f163f..0e97249 100644
> --- a/man/man8/tc-codel.8
> +++ b/man/man8/tc-codel.8
> @@ -64,6 +64,9 @@ is the acceptable minimum standing/persistent queue delay. This minimum delay
>  is identified by tracking the local minimum queue delay that packets experience.
>  Default and recommended value is 5ms.
>  
> +.SS ecn_target
> +is the overall delay at which ecn marked packets will be dropped rather than marked.
> +
>  .SS interval
>  is used to ensure that the measured minimum delay does not become too stale. The
>  minimum delay must be experienced in the last epoch of length
> diff --git a/man/man8/tc-fq_codel.8 b/man/man8/tc-fq_codel.8
> index 8b43c10..9cb5be8 100644
> --- a/man/man8/tc-fq_codel.8
> +++ b/man/man8/tc-fq_codel.8
> @@ -49,6 +49,11 @@ and is the acceptable minimum
>  standing/persistent queue delay. This minimum delay is identified by tracking
>  the local minimum queue delay that packets experience.  Default value is 5ms.
>  
> +.SS ecn_target
> +has the same semantics as
> +.B codel
> +and is the delay beyond which ecn packets will be dropped rather than marked.
> +

What do you mean by "ecn_target has the same semantics as codel" ?

Re: [Codel] [RFCv2 PATCH] iproute2: Add ecn_target option to codel and fq_codel

[Eric Dumazet]

On Sun, 2012-06-24 at 22:00 -0700, Dave Täht wrote:
> From: Dave Taht <dave.taht@bufferbloat.net>
> 
> ECN was not part of the original codel design and adding support
> for it revealed problems in two areas. 1) ECN can be gamed.
> 2) Dropping packets under overload more rapidly frees up
> bandwidth than marking packets.
> 
> Two possible scenarios of use - on egress from a network,
> ecn_target could be set low, to drop more often, to
> ensure lowest latency for other packets.
> 
> On ingress, it could be set high to mark packets more often,
> to lower data loss while still signalling the end application
> that bandwidth is a problem.
> 
> ecn_target is not engaged until after codel enters a dropping
> state overall.

I would suggest 'drop_above' instead of ecn_target, since its quite
different than the 'target'

(And dont display/output it if ecn is not set, no need to confuse users
who didn't enable ecn on CoDel)

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

[Dave Taht]

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!"

Re: [Codel] [RFCv2 PATCH] iproute2: Add ecn_target option to codel and fq_codel

[Dave Taht]

On Sun, Jun 24, 2012 at 10:29 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 was not part of the original codel design and adding support
>> for it revealed problems in two areas. 1) ECN can be gamed.
>> 2) Dropping packets under overload more rapidly frees up
>> bandwidth than marking packets.
>>
>> Two possible scenarios of use - on egress from a network,
>> ecn_target could be set low, to drop more often, to
>> ensure lowest latency for other packets.
>>
>> On ingress, it could be set high to mark packets more often,
>> to lower data loss while still signalling the end application
>> that bandwidth is a problem.
>>
>> ecn_target is not engaged until after codel enters a dropping
>> state overall.
>
> I would suggest 'drop_above' instead of ecn_target, since its quite
> different than the 'target'

OK. I will respin this after resolving the issue I raised in the previous patch
comment (just switch ecn to being an int?), and do some testing. As noted
I was (de)impressed at the amount of ecn drops I had with it set to 2*target
on pure ecn enabled streams. In one test it was 95% drops...

And I'm still open to saner approaches. Another one might be to at graduated
levels drop big packets down to smaller ones.

> (And dont display/output it if ecn is not set, no need to confuse users
> who didn't enable ecn on CoDel)
>
>
>
> _______________________________________________
> 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!"

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

[Eric Dumazet]

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.

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

[Dave Taht]

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!"

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

[Eric Dumazet]

On Mon, 2012-06-25 at 08:30 -0700, Dave Taht wrote:

> 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.
> 

Don't try to add in CoDel things that should be done at another layer.
There is no classification in CoDel : Its a FIFO.

Only way is to prioritize control packets if you need to, and use
several queues.

I dont understand... Are you saying that you feel the need to drop
packets instead of marking them in a 'good citizen world' (none of your
flow pretends to use ECN to avoid drops) ?

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

[Jonathan Morton]

My impression is that ECN ignorant flows do exist, because of stupid routers rather than malice, and that Dave considers this a problem worth tackling in codel. 

However I would disagree that the problem needs tackling here, at least for fq_codel which already segregates non-responsive flows from others. The ignorant flows will experience higher latency if this link is a bottleneck for them, that is all. 

Plain codel might need a more robust defence, but I have a different suggestion for that. Determine whether a prolonged bout of marking is being caused by ECN ignorant flows, and if so disable ECN marking (just drop) until the ignorant flows have clearly gone away. 

The key to knowledge is not to rely on others to teach you it. 

On 25 Jun 2012, at 18:45, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Mon, 2012-06-25 at 08:30 -0700, Dave Taht wrote:
> 
>> 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.
>> 
> 
> Don't try to add in CoDel things that should be done at another layer.
> There is no classification in CoDel : Its a FIFO.
> 
> Only way is to prioritize control packets if you need to, and use
> several queues.
> 
> I dont understand... Are you saying that you feel the need to drop
> packets instead of marking them in a 'good citizen world' (none of your
> flow pretends to use ECN to avoid drops) ?
> 
> 
> 
> _______________________________________________
> Codel mailing list
> Codel@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/codel

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

[Dave Taht]

On Mon, Jun 25, 2012 at 10:48 AM, Jonathan Morton <chromatix99@gmail.com> wrote:
> My impression is that ECN ignorant flows do exist, because of stupid routers rather than malice, and that Dave considers this a problem worth tackling in codel.

Malice will exist. It is easy to create a set of flows that will
overwhelm either codel or fq_codel, with ecn enabled.

fq_codel does respond well to udp flooding, I note, far better than pfifo_fast.

>
> However I would disagree that the problem needs tackling here, at least for fq_codel which already segregates non-responsive flows from others. The ignorant flows will experience higher latency if this link is a bottleneck for them, that is all.

the first version of fq_codel had a separate codel queue per flow,
which would behave as you say. The lastest version uses the global
drop probability, which makes more sense in the general case.

I continue to prototype with qfq+codel which has the first behavior,
presently. I like the idea of a more perfect fluid model, especially
at the bandwidths I operate at (2-30Mbit), with the cpu horsepower
available to make more better choices than fq_codel can.


> Plain codel might need a more robust defence, but I have a different suggestion for that. Determine whether a prolonged bout of marking is being caused by ECN ignorant flows, and if so disable ECN marking (just drop) until the ignorant flows have clearly gone away.

Well, an example algo for that would be helpful. SFB tried to do this.

I note that codel operates independently of RTT and I am thinking hard
that RTT related issues need to be explored overall.

> The key to knowledge is not to rely on others to teach you it.

I am an experimenter first and foremost. My inspirations are more from
edison than tesla. Queue behavior is rarely intuitive, and the only
way to build intuition, for me, at least, is to design an experiment
and observe what happens.

Kelly Johnson could "see air". Perhaps van and kathie can "see
packets". Me, on the other hand...

>
> On 25 Jun 2012, at 18:45, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> On Mon, 2012-06-25 at 08:30 -0700, Dave Taht wrote:
>>
>>> 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.
>>>
>>
>> Don't try to add in CoDel things that should be done at another layer.
>> There is no classification in CoDel : Its a FIFO.
>>
>> Only way is to prioritize control packets if you need to, and use
>> several queues.
>>
>> I dont understand... Are you saying that you feel the need to drop
>> packets instead of marking them in a 'good citizen world' (none of your
>> flow pretends to use ECN to avoid drops) ?
>>
>>
>>
>> _______________________________________________
>> 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!"

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

[Eric Dumazet]

On Mon, 2012-06-25 at 11:25 -0700, Dave Taht wrote:
> On Mon, Jun 25, 2012 at 10:48 AM, Jonathan Morton <chromatix99@gmail.com> wrote:
> > My impression is that ECN ignorant flows do exist, because of stupid routers rather than malice, and that Dave considers this a problem worth tackling in codel.
> 
> Malice will exist. It is easy to create a set of flows that will
> overwhelm either codel or fq_codel, with ecn enabled.

Then don't enable ecn. Simple.

Don't try to solve ecn problems at Qdisc layer, its not going to work.

fq_codel is default ecn enabled, because you can overwhelm fq_codel with
or without ecn anyway.