[Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Täht]

Lossless IP networks are not possible. Dropping packets is the
fastest way to get back to a target delay, and if that involves
dropping ECN marked packets, so be it. When a network is in a
more steady state, ECN is fine...

My choice of "2 * target" as a threshold for dropping ECN
marked packets is entirely arbitrary.
---
 include/net/codel.h |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/net/codel.h b/include/net/codel.h
index 550debf..26944a0 100644
--- a/include/net/codel.h
+++ b/include/net/codel.h
@@ -280,7 +280,8 @@ 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 && INET_ECN_set_ce(skb) &&
+					vars->ldelay <= 2 * params->target) {
 					stats->ecn_mark++;
 					vars->drop_next =
 						codel_control_law(vars->drop_next,
@@ -305,7 +306,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
 			}
 		}
 	} else if (drop) {
-		if (params->ecn && INET_ECN_set_ce(skb)) {
+		if (params->ecn && INET_ECN_set_ce(skb) &&
+		        vars->ldelay <= 2 * params->target) {
 			stats->ecn_mark++;
 		} else {
 			qdisc_drop(skb, sch);
-- 
1.7.9.5

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Eric Dumazet]

On Sun, 2012-06-17 at 15:30 -0700, Dave Täht wrote:
> Lossless IP networks are not possible. Dropping packets is the
> fastest way to get back to a target delay, and if that involves
> dropping ECN marked packets, so be it. When a network is in a
> more steady state, ECN is fine...
> 

Just disable ecn then ? By the way ECN is disabled by default on CoDel.

> My choice of "2 * target" as a threshold for dropping ECN
> marked packets is entirely arbitrary.
> ---
>  include/net/codel.h |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/codel.h b/include/net/codel.h
> index 550debf..26944a0 100644
> --- a/include/net/codel.h
> +++ b/include/net/codel.h
> @@ -280,7 +280,8 @@ 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 && INET_ECN_set_ce(skb) &&
> +					vars->ldelay <= 2 * params->target) {
>  					stats->ecn_mark++;
>  					vars->drop_next =
>  						codel_control_law(vars->drop_next,
> @@ -305,7 +306,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
>  			}
>  		}
>  	} else if (drop) {
> -		if (params->ecn && INET_ECN_set_ce(skb)) {
> +		if (params->ecn && INET_ECN_set_ce(skb) &&
> +		        vars->ldelay <= 2 * params->target) {
>  			stats->ecn_mark++;
>  		} else {
>  			qdisc_drop(skb, sch);

Hmm, while I understand the idea, patch is wrong.

if (A && B && C)

Even if (C) is false, but A is true, B is evaluated.

Also, a 2 * target value is arbitrary, why not instead provide a real
attribute, that user can set with tc command ?

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Taht]

On Sun, Jun 17, 2012 at 10:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-06-17 at 15:30 -0700, Dave Täht wrote:
>> Lossless IP networks are not possible. Dropping packets is the
>> fastest way to get back to a target delay, and if that involves
>> dropping ECN marked packets, so be it. When a network is in a
>> more steady state, ECN is fine...
>>
>
> Just disable ecn then ? By the way ECN is disabled by default on CoDel.

I LIKE ecn. I like it enabled by default on fq_codel. I wouldn't mind if it
was enabled by default on everything... if we knew how to handle it.

>> My choice of "2 * target" as a threshold for dropping ECN
>> marked packets is entirely arbitrary.
>> ---
>>  include/net/codel.h |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/net/codel.h b/include/net/codel.h
>> index 550debf..26944a0 100644
>> --- a/include/net/codel.h
>> +++ b/include/net/codel.h
>> @@ -280,7 +280,8 @@ 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 && INET_ECN_set_ce(skb) &&
>> +                                     vars->ldelay <= 2 * params->target) {
>>                                       stats->ecn_mark++;
>>                                       vars->drop_next =
>>                                               codel_control_law(vars->drop_next,
>> @@ -305,7 +306,8 @@ static struct sk_buff *codel_dequeue(struct Qdisc *sch,
>>                       }
>>               }
>>       } else if (drop) {
>> -             if (params->ecn && INET_ECN_set_ce(skb)) {
>> +             if (params->ecn && INET_ECN_set_ce(skb) &&
>> +                     vars->ldelay <= 2 * params->target) {
>>                       stats->ecn_mark++;
>>               } else {
>>                       qdisc_drop(skb, sch);
>
> Hmm, while I understand the idea, patch is wrong.

so we are in agreement tho that ECN packets should be dropped in
some circumstances?

> if (A && B && C)
>
> Even if (C) is false, but A is true, B is evaluated.

A is always true (or always false)
B in this case is usually false except on the kinds of ECN-heavy
workloads that exposed this issue
and C is probably higher overhead than A or B

So a better conditional is possible...

> Also, a 2 * target value is arbitrary, why not instead provide a real
> attribute, that user can set with tc command ?

In part, that's why this is an RFC. I didn't want to introduce a tc
change at this late date (tho it can be done) with all the binary packages
(openwrt, fedora, ubuntu) that just landed. I did want to open up a
discussion of what would be sane - is 2 * target sane? 8 * target sane? or
thinking about sojourn time a little harder sane? Or for example, like SFB,
identifying unresponsive flows, instead, sane.

I'm allergic to knobs, too. So...

my next patch was going to be to the ns3 sim unless andrew beat me to it.

(side note, I noticed fq_codel defaulted to 10k packets which is
rather excessive for tiny routers - I just trimmed that down
significantly for cerowrt and the upcoming 3.3.8-4 release has the rfc
patch in it)

And apologies for not seeing this long ago,

I'm only just now catching up on things somewhat, I hope we can do
some benchmarks at various bandwidths and be able to see what works
well
under a variety of conditions.

>
>
> _______________________________________________
> Codel mailing list
> Codel@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/codel



-- 
Dave Täht
SKYPE: davetaht
http://ronsravings.blogspot.com/

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Eric Dumazet]

On Sun, 2012-06-17 at 22:40 -0400, Dave Taht wrote:
> On Sun, Jun 17, 2012 at 10:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> > if (A && B && C)
> >
> > Even if (C) is false, but A is true, B is evaluated.
> 
> A is always true (or always false)
> B in this case is usually false except on the kinds of ECN-heavy
> workloads that exposed this issue
> and C is probably higher overhead than A or B
> 
> So a better conditional is possible...

Not only better, but _correct_ ;)

To have a chance to be correct, your patch should have been :


if (params->ecn &&
    vars->ldelay <= 2 * params->target &&
    INET_ECN_set_ce(skb))

Because INET_ECN_set_ce(skb) is _doing_ ecn marking, and returns true is
ECN was successfully applied to the packet.

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Taht]

On Sun, Jun 17, 2012 at 10:58 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-06-17 at 22:40 -0400, Dave Taht wrote:
>> On Sun, Jun 17, 2012 at 10:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>> > if (A && B && C)
>> >
>> > Even if (C) is false, but A is true, B is evaluated.
>>
>> A is always true (or always false)
>> B in this case is usually false except on the kinds of ECN-heavy
>> workloads that exposed this issue
>> and C is probably higher overhead than A or B
>>
>> So a better conditional is possible...
>
> Not only better, but _correct_ ;)
>
> To have a chance to be correct, your patch should have been :
>
>
> if (params->ecn &&
>    vars->ldelay <= 2 * params->target &&
>    INET_ECN_set_ce(skb))
>
> Because INET_ECN_set_ce(skb) is _doing_ ecn marking, and returns true is
> ECN was successfully applied to the packet.

heh. OK, point taken, patch corrected here. I am averaging one bug per
two lines of code...



-- 
Dave Täht
SKYPE: davetaht
http://ronsravings.blogspot.com/

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Eric Dumazet]

On Sun, 2012-06-17 at 22:40 -0400, Dave Taht wrote:

> (side note, I noticed fq_codel defaulted to 10k packets which is
> rather excessive for tiny routers - I just trimmed that down
> significantly for cerowrt and the upcoming 3.3.8-4 release has the rfc
> patch in it)
> 
> And apologies for not seeing this long ago,

10k packets is too small to absorb a burst of 64bytes packets on 10Gb
links. Whole CoDel point is to accept packets at enqueue and drop them
at dequeue _if_ sejourn time too big. Number of packets should be
irrelevant.

If you don't know how much packet can be sent on wire per unit of time,
just set a reasonable big limit.

1000 packets limit is not reasonable, while 10k is.

linux average machines have more ram than tiny routers, dont assume we
release specialized code. It should be generic enough, granted it can be
easily tuned.

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Taht]

On Sun, Jun 17, 2012 at 11:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-06-17 at 22:40 -0400, Dave Taht wrote:
>
>> (side note, I noticed fq_codel defaulted to 10k packets which is
>> rather excessive for tiny routers - I just trimmed that down
>> significantly for cerowrt and the upcoming 3.3.8-4 release has the rfc
>> patch in it)
>>
>> And apologies for not seeing this long ago,
>
> 10k packets is too small to absorb a burst of 64bytes packets on 10Gb
> links. Whole CoDel point is to accept packets at enqueue and drop them
> at dequeue _if_ sejourn time too big. Number of packets should be
> irrelevant.

I agree the setting is good for 10GigE. 40GigE is coming up. 100GigE
is being specified...

> If you don't know how much packet can be sent on wire per unit of time,
> just set a reasonable big limit.
>
> 1000 packets limit is not reasonable, while 10k is.
>
> linux average machines have more ram than tiny routers, dont assume we
> release specialized code. It should be generic enough, granted it can be
> easily tuned.

I don't have a problem with the limit being set to sane values for
modern systems. I note that for 10GigE we are also setting target,
interval at
lower values manually already, so setting that value additionally
seems less work than requiring that systems running at GigE and below
set it - systems that already have a pretty standard 1000 packet limit
baked into their PFIFO_FAST assumptions.

An overall better no knobs approach would be to be able to set the
limit based on the observed hard or soft line rate of the interface.

That said, so long as the need for setting a lower limit in fq_codel
is well documented for smaller systems like tiny routers I don't have
a problem with it. A sane limit can be calculated for soft-limited
interfaces on devices with memory constraints as well

>



-- 
Dave Täht
SKYPE: davetaht
http://ronsravings.blogspot.com/

Re: [Codel] [RFC PATCH] Codel: Enable packet drop with ECN-marked packets on a threshold

[Dave Taht]

OK, so to move forward on testing this on linux directly I'll
produce a better patch, and add tc support for

ecn_limit? ecn_target? ecn_drop_limit? harddrop_at?

However: I'm perfectly happy leaving the mainline linux implementation
alone for a while, fiddling with ns3 instead, and especially
continuing to solicit other ideas towards better handling ecn marked
bursts inside codel,
than merely arbitrarily doing "target * x"


-- 
Dave Täht
SKYPE: davetaht
http://ronsravings.blogspot.com/