[PATCH 1/2] net: netem: do not reorder when reordering is disabled

[PATCH 1/2] net: netem: do not reorder when reordering is disabled

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

The case (q->reorder == 0 && get_crandom(&q->reorder_cor) == 0)
could result in reordering even though reordering is disabled.

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 net/sched/sch_netem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 82f6016..abe5fa6 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -454,7 +454,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	sch->qstats.backlog += qdisc_pkt_len(skb);
 
 	cb = netem_skb_cb(skb);
-	if (q->gap == 0 ||		/* not doing reordering */
+	if (q->gap == 0 || q->reorder == 0 || /* not doing reordering */
 	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
 	    q->reorder < get_crandom(&q->reorder_cor)) {
 		psched_time_t now;
-- 
1.8.3.1

[PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Not doing this (current behaviour) introduces reordering.

The packet_len_2_sched_time call is the only thing that logically
depends on q->rate, so move the now/delay adjustment out of the if.

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 net/sched/sch_netem.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index abe5fa6..86c73d2 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -457,6 +457,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	if (q->gap == 0 || q->reorder == 0 || /* not doing reordering */
 	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
 	    q->reorder < get_crandom(&q->reorder_cor)) {
+		struct sk_buff *last;
+
 		psched_time_t now;
 		psched_tdiff_t delay;
 
@@ -465,24 +467,22 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 		now = psched_get_time();
 
-		if (q->rate) {
-			struct sk_buff *last;
-
-			if (!skb_queue_empty(&sch->q))
-				last = skb_peek_tail(&sch->q);
-			else
-				last = netem_rb_to_skb(rb_last(&q->t_root));
-			if (last) {
-				/*
-				 * Last packet in queue is reference point (now),
-				 * calculate this time bonus and subtract
-				 * from delay.
-				 */
-				delay -= netem_skb_cb(last)->time_to_send - now;
-				delay = max_t(psched_tdiff_t, 0, delay);
-				now = netem_skb_cb(last)->time_to_send;
-			}
+		if (!skb_queue_empty(&sch->q))
+			last = skb_peek_tail(&sch->q);
+		else
+			last = netem_rb_to_skb(rb_last(&q->t_root));
+		if (last) {
+			/*
+			 * Last packet in queue is reference point (now),
+			 * calculate this time bonus and subtract
+			 * from delay.
+			 */
+			delay -= netem_skb_cb(last)->time_to_send - now;
+			delay = max_t(psched_tdiff_t, 0, delay);
+			now = netem_skb_cb(last)->time_to_send;
+		}
 
+		if (q->rate) {
 			delay += packet_len_2_sched_time(skb->len, q);
 		}
 
-- 
1.8.3.1

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Sergei Shtylyov]

Hello.

On 08/20/2013 07:11 PM, Ferry Huberts wrote:

> From: Ferry Huberts <ferry.huberts@pelagic.nl>

> Not doing this (current behaviour) introduces reordering.

> The packet_len_2_sched_time call is the only thing that logically
> depends on q->rate, so move the now/delay adjustment out of the if.

> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> ---
>   net/sched/sch_netem.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)

> diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
> index abe5fa6..86c73d2 100644
> --- a/net/sched/sch_netem.c
> +++ b/net/sched/sch_netem.c
[...]
> @@ -465,24 +467,22 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
[...]
>
> +		if (q->rate) {
>   			delay += packet_len_2_sched_time(skb->len, q);
>   		}

    Single statements shouldn't be enclosed in {} (and scripts/checkpatch.pl 
should complain about it).

WBR, Sergei

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Eric Dumazet]

On Tue, 2013-08-20 at 17:11 +0200, Ferry Huberts wrote:
> From: Ferry Huberts <ferry.huberts@pelagic.nl>
> 
> Not doing this (current behaviour) introduces reordering.
> 
> The packet_len_2_sched_time call is the only thing that logically
> depends on q->rate, so move the now/delay adjustment out of the if.
> 
> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> ---
>  net/sched/sch_netem.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)

I would like you to show how this was tested, for example if we have a
rate + delay + reorders.

Thanks

[PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Not doing this (current behaviour) introduces reordering.

The packet_len_2_sched_time call is the only thing that logically
depends on q->rate, so move the now/delay adjustment out of the if.

How to test:
------------
- Create a script to ping the default gateway using a queueing discipline:
cat > ./netemreordering << "EOF"
fields=( $(route -n | grep -E '^0.0.0.0\b' | awk '{ print $2 " " $NF; }') )
tc qdisc del dev "${fields[1]}" root
tc qdisc add dev "${fields[1]}" handle 1 root netem delay 10ms 500ms
ping -c 10 -i 0.1 -W 18 "${fields[0]}"
tc qdisc del dev "${fields[1]}" root
EOF
chmod 755 ./netemreordering
- Run the script:
sudo ./netemreordering


Current Behaviour:
------------------
PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=111 ms
64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=311 ms
64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=201 ms
64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=412 ms
64 bytes from 10.0.0.1: icmp_seq=8 ttl=64 time=58.1 ms
64 bytes from 10.0.0.1: icmp_seq=7 ttl=64 time=168 ms
64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=379 ms
64 bytes from 10.0.0.1: icmp_seq=9 ttl=64 time=171 ms
64 bytes from 10.0.0.1: icmp_seq=10 ttl=64 time=62.0 ms
64 bytes from 10.0.0.1: icmp_seq=6 ttl=64 time=491 ms

--- 10.0.0.1 ping statistics ---
10 packets transmitted, 10 received, 0% packet loss, time 961ms
rtt min/avg/max/mdev = 58.105/236.707/491.683/144.911 ms, pipe 5


Fixed Behaviour:
----------------
PING 10.0.0.1 (10.0.0.1) 56(84) bytes of data.
64 bytes from 10.0.0.1: icmp_seq=1 ttl=64 time=244 ms
64 bytes from 10.0.0.1: icmp_seq=2 ttl=64 time=135 ms
64 bytes from 10.0.0.1: icmp_seq=3 ttl=64 time=188 ms
64 bytes from 10.0.0.1: icmp_seq=4 ttl=64 time=87.7 ms
64 bytes from 10.0.0.1: icmp_seq=5 ttl=64 time=207 ms
64 bytes from 10.0.0.1: icmp_seq=6 ttl=64 time=107 ms
64 bytes from 10.0.0.1: icmp_seq=7 ttl=64 time=199 ms
64 bytes from 10.0.0.1: icmp_seq=8 ttl=64 time=98.4 ms
64 bytes from 10.0.0.1: icmp_seq=9 ttl=64 time=61.0 ms
64 bytes from 10.0.0.1: icmp_seq=10 ttl=64 time=295 ms

--- 10.0.0.1 ping statistics ---
10 packets transmitted, 10 received, 0% packet loss, time 912ms
rtt min/avg/max/mdev = 61.002/162.580/295.638/72.336 ms, pipe 3


v2:
- fix checkpatch 'braces' warning
- add more comments on how to test

Reported-by: Teco Boot <teco@inf-net.nl>
Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 net/sched/sch_netem.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index abe5fa6..64e0653 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -457,6 +457,8 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 	if (q->gap == 0 || q->reorder == 0 || /* not doing reordering */
 	    q->counter < q->gap - 1 ||	/* inside last reordering gap */
 	    q->reorder < get_crandom(&q->reorder_cor)) {
+		struct sk_buff *last;
+
 		psched_time_t now;
 		psched_tdiff_t delay;
 
@@ -465,26 +467,23 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch)
 
 		now = psched_get_time();
 
-		if (q->rate) {
-			struct sk_buff *last;
-
-			if (!skb_queue_empty(&sch->q))
-				last = skb_peek_tail(&sch->q);
-			else
-				last = netem_rb_to_skb(rb_last(&q->t_root));
-			if (last) {
-				/*
-				 * Last packet in queue is reference point (now),
-				 * calculate this time bonus and subtract
-				 * from delay.
-				 */
-				delay -= netem_skb_cb(last)->time_to_send - now;
-				delay = max_t(psched_tdiff_t, 0, delay);
-				now = netem_skb_cb(last)->time_to_send;
-			}
+		if (!skb_queue_empty(&sch->q))
+			last = skb_peek_tail(&sch->q);
+		else
+			last = netem_rb_to_skb(rb_last(&q->t_root));
+		if (last) {
+			/*
+			 * Last packet in queue is reference point (now),
+			 * calculate this time bonus and subtract
+			 * from delay.
+			 */
+			delay -= netem_skb_cb(last)->time_to_send - now;
+			delay = max_t(psched_tdiff_t, 0, delay);
+			now = netem_skb_cb(last)->time_to_send;
+		}
 
+		if (q->rate)
 			delay += packet_len_2_sched_time(skb->len, q);
-		}
 
 		cb->time_to_send = now + delay;
 		cb->tstamp_save = skb->tstamp;
-- 
1.8.3.1

Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[Eric Dumazet]

On Wed, 2013-08-21 at 07:59 +0200, Ferry Huberts wrote:
> From: Ferry Huberts <ferry.huberts@pelagic.nl>
> 
> Not doing this (current behaviour) introduces reordering.
> 
> The packet_len_2_sched_time call is the only thing that logically
> depends on q->rate, so move the now/delay adjustment out of the if.
> 
> How to test:
> -----------

I ask again :

Did you test a config with both rate limiting and delay.

Netem primary use is to emulate say a 1Mbits link with a rtt of 50ms

netem rate 1Mbit delay 50ms


Because the "delay 10ms 500ms" is very strange : effective delay is in
the following range : -490 ms ... 510 ms

Its probably clamped to 0ms ... 510ms

I really doubt this is what you wanted.

Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[Ferry Huberts]

On 21/08/13 08:14, Eric Dumazet wrote:
> On Wed, 2013-08-21 at 07:59 +0200, Ferry Huberts wrote:
>> From: Ferry Huberts <ferry.huberts@pelagic.nl>
>>
>> Not doing this (current behaviour) introduces reordering.
>>
>> The packet_len_2_sched_time call is the only thing that logically
>> depends on q->rate, so move the now/delay adjustment out of the if.
>>
>> How to test:
>> -----------
> 
> I ask again :
> 
> Did you test a config with both rate limiting and delay.

(sorry for missing that question)

Just did so and with rate limiting I get no reordering, which is logical
looking at the code.

The thing is, the evaluation q->rate is within the 'no-reordering' block
and in the current situation you can get reordering (with that 'strange'
command). My patch makes sure that no reordering will occur, and
effectively 'clamps' the realised delay, which currently isn't done.

> 
> Netem primary use is to emulate say a 1Mbits link with a rtt of 50ms
> 
> netem rate 1Mbit delay 50ms
> 
> 
> Because the "delay 10ms 500ms" is very strange : effective delay is in
> the following range : -490 ms ... 510 ms
> 
> Its probably clamped to 0ms ... 510ms

Currently it isn't. With my patch it's 'clamped' in the sense that if
the calculated delay would make the packet be scheduled before the
previous one, that the delay of the current packet is adjusted so that
it's scheduled right after the previous one.

> 
> I really doubt this is what you wanted.

It's the command Teco showed me to test/reproduce it.


-- 
Ferry Huberts

Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[Eric Dumazet]

On Wed, 2013-08-21 at 09:04 +0200, Ferry Huberts wrote:
> 
> On 21/08/13 08:14, Eric Dumazet wrote:
> > On Wed, 2013-08-21 at 07:59 +0200, Ferry Huberts wrote:
> >> From: Ferry Huberts <ferry.huberts@pelagic.nl>
> >>
> >> Not doing this (current behaviour) introduces reordering.
> >>
> >> The packet_len_2_sched_time call is the only thing that logically
> >> depends on q->rate, so move the now/delay adjustment out of the if.
> >>
> >> How to test:
> >> -----------
> > 
> > I ask again :
> > 
> > Did you test a config with both rate limiting and delay.
> 
> (sorry for missing that question)
> 
> Just did so and with rate limiting I get no reordering, which is logical
> looking at the code.
> 
> The thing is, the evaluation q->rate is within the 'no-reordering' block
> and in the current situation you can get reordering (with that 'strange'
> command). My patch makes sure that no reordering will occur, and
> effectively 'clamps' the realised delay, which currently isn't done.


OK, let me be very clear.

I would like you post the results of regression tests, to make sure that
you do not add a new regression.

It seems that you want _us_ to check all this for you.

With "rate 1Mbits delay 100ms 10ms", and ping probes sent every 100ms,
the pong reply of _all_ probes should be between 90ms and 110ms

Is it still the case after your patch ?

Thanks

Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[mailings]

> On Wed, 2013-08-21 at 09:04 +0200, Ferry Huberts wrote:
>> On 21/08/13 08:14, Eric Dumazet wrote:
>> > On Wed, 2013-08-21 at 07:59 +0200, Ferry Huberts wrote:
>> >> From: Ferry Huberts <ferry.huberts@pelagic.nl>
>> >>
>> >> Not doing this (current behaviour) introduces reordering.
>> >>
>> >> The packet_len_2_sched_time call is the only thing that logically
depends on q->rate, so move the now/delay adjustment out of the if.
>> >>
>> >> How to test:
>> >> -----------
>> >
>> > I ask again :
>> >
>> > Did you test a config with both rate limiting and delay.
>> (sorry for missing that question)
>> Just did so and with rate limiting I get no reordering, which is
logical
>> looking at the code.
>> The thing is, the evaluation q->rate is within the 'no-reordering'
block
>> and in the current situation you can get reordering (with that
'strange'
>> command). My patch makes sure that no reordering will occur, and
effectively 'clamps' the realised delay, which currently isn't done.
>
>
> OK, let me be very clear.
>
> I would like you post the results of regression tests, to make sure that
you do not add a new regression.
>

Since I'm new to netdev development and therefore the processes attached
to it, let me ask where I can find such regression tests?

I'm happy to run them once I know about them... ;-)


> It seems that you want _us_ to check all this for you.
>
> With "rate 1Mbits delay 100ms 10ms", and ping probes sent every 100ms,
the pong reply of _all_ probes should be between 90ms and 110ms
>
> Is it still the case after your patch ?
>

Yes it is.

In the script I described in the commit message I replaced the line
  tc qdisc add dev "${fields[1]}" handle 1 root netem delay 10ms 500ms
with the line (1Mbit gave 'Illegal "rate"')
  tc qdisc add dev "${fields[1]}" handle 1 root netem rate 1024000 delay
100ms 10ms

The output then was (tried a 10 times):
# ./netemreordering
PING 192.168.160.1 (192.168.160.1) 56(84) bytes of data.
64 bytes from 192.168.160.1: icmp_seq=1 ttl=254 time=114 ms
64 bytes from 192.168.160.1: icmp_seq=2 ttl=254 time=109 ms
64 bytes from 192.168.160.1: icmp_seq=3 ttl=254 time=106 ms
64 bytes from 192.168.160.1: icmp_seq=4 ttl=254 time=100 ms
64 bytes from 192.168.160.1: icmp_seq=5 ttl=254 time=111 ms
64 bytes from 192.168.160.1: icmp_seq=6 ttl=254 time=92.2 ms
64 bytes from 192.168.160.1: icmp_seq=7 ttl=254 time=99.8 ms
64 bytes from 192.168.160.1: icmp_seq=8 ttl=254 time=91.4 ms
64 bytes from 192.168.160.1: icmp_seq=9 ttl=254 time=97.4 ms
64 bytes from 192.168.160.1: icmp_seq=10 ttl=254 time=95.5 ms

--- 192.168.160.1 ping statistics ---
10 packets transmitted, 10 received, 0% packet loss, time 910ms
rtt min/avg/max/mdev = 91.413/101.879/114.534/7.723 ms, pipe 2


Is this enough?
Please let me know, I'm happy to oblige.

Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering

[mailings]

>> On Wed, 2013-08-21 at 09:04 +0200, Ferry Huberts wrote:
>>> On 21/08/13 08:14, Eric Dumazet wrote:
>>> > On Wed, 2013-08-21 at 07:59 +0200, Ferry Huberts wrote:
>>> >> From: Ferry Huberts <ferry.huberts@pelagic.nl>
>>> >>
>>> >> Not doing this (current behaviour) introduces reordering.
>>> >>
>>> >> The packet_len_2_sched_time call is the only thing that logically
> depends on q->rate, so move the now/delay adjustment out of the if.
>>> >>
>>> >> How to test:
>>> >> -----------
>>> >
>>> > I ask again :
>>> >
>>> > Did you test a config with both rate limiting and delay.
>>> (sorry for missing that question)
>>> Just did so and with rate limiting I get no reordering, which is
> logical
>>> looking at the code.
>>> The thing is, the evaluation q->rate is within the 'no-reordering'
> block
>>> and in the current situation you can get reordering (with that
> 'strange'
>>> command). My patch makes sure that no reordering will occur, and
> effectively 'clamps' the realised delay, which currently isn't done.
>>
>>
>> OK, let me be very clear.
>>
>> I would like you post the results of regression tests, to make sure that
> you do not add a new regression.
>>
>
> Since I'm new to netdev development and therefore the processes attached
> to it, let me ask where I can find such regression tests?
>
> I'm happy to run them once I know about them... ;-)
>
>
>> It seems that you want _us_ to check all this for you.
>>
>> With "rate 1Mbits delay 100ms 10ms", and ping probes sent every 100ms,
> the pong reply of _all_ probes should be between 90ms and 110ms
>>
>> Is it still the case after your patch ?
>>
>
> Yes it is.
>
> In the script I described in the commit message I replaced the line
>   tc qdisc add dev "${fields[1]}" handle 1 root netem delay 10ms 500ms
> with the line (1Mbit gave 'Illegal "rate"')
>   tc qdisc add dev "${fields[1]}" handle 1 root netem rate 1024000 delay
> 100ms 10ms
>
> The output then was (tried a 10 times):
> # ./netemreordering
> PING 192.168.160.1 (192.168.160.1) 56(84) bytes of data.
> 64 bytes from 192.168.160.1: icmp_seq=1 ttl=254 time=114 ms

Wow. And of course Murphy would do this.
This is the _only_ deviation there was and I copy&pasted it...

I re-checked and it really is the only one.
I re-ran 20 more times and all were ok.
Must have been a busy router I guess.

> 64 bytes from 192.168.160.1: icmp_seq=2 ttl=254 time=109 ms
> 64 bytes from 192.168.160.1: icmp_seq=3 ttl=254 time=106 ms
> 64 bytes from 192.168.160.1: icmp_seq=4 ttl=254 time=100 ms
> 64 bytes from 192.168.160.1: icmp_seq=5 ttl=254 time=111 ms
> 64 bytes from 192.168.160.1: icmp_seq=6 ttl=254 time=92.2 ms
> 64 bytes from 192.168.160.1: icmp_seq=7 ttl=254 time=99.8 ms
> 64 bytes from 192.168.160.1: icmp_seq=8 ttl=254 time=91.4 ms
> 64 bytes from 192.168.160.1: icmp_seq=9 ttl=254 time=97.4 ms
> 64 bytes from 192.168.160.1: icmp_seq=10 ttl=254 time=95.5 ms
>
> --- 192.168.160.1 ping statistics ---
> 10 packets transmitted, 10 received, 0% packet loss, time 910ms
> rtt min/avg/max/mdev = 91.413/101.879/114.534/7.723 ms, pipe 2
>
>
> Is this enough?
> Please let me know, I'm happy to oblige.
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Johannes Naab]

On 08/20/2013 05:11 PM, Ferry Huberts wrote:
> From: Ferry Huberts <ferry.huberts@pelagic.nl>
> 
> Not doing this (current behaviour) introduces reordering.
> 
> The packet_len_2_sched_time call is the only thing that logically
> depends on q->rate, so move the now/delay adjustment out of the if.
> 
> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> ---

Hi,

The documentation for netem does explicitly mention the reordering with
jitter, and gives instructions on how to avoid it. (I have not tested if
it works as intended).

http://www.linuxfoundation.org/collaborate/workgroups/networking/netem#How_to_reorder_packets_based_on_jitter.3F

Could you test if that fixes your problem already?


Assuming that the documentation is right:

For the sake of stability I strongly oppose changing existing and
documented behavior without opt-in. (Then again I'm not involved in
kernel development.)

If the documentation turns out to be wrong:

The netem rate extension (with all the reordering changes for that case,
which are needed for proper rate emulation in that model) was introduced
in 3.3 by Hagen Paul Pfeifer
http://marc.info/?l=linux-netdev&m=132215646820338&w=2. You could ping
him, and try to get his input/Acked-by on your patch. I only did some
bug fixing in 3.8 http://marc.info/?l=linux-netdev&m=135897762224399&w=2.

Best regards,

Johannes
-- 

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Eric Dumazet]

On Wed, 2013-08-21 at 17:17 +0200, Johannes Naab wrote:
> On 08/20/2013 05:11 PM, Ferry Huberts wrote:
> > From: Ferry Huberts <ferry.huberts@pelagic.nl>
> > 
> > Not doing this (current behaviour) introduces reordering.
> > 
> > The packet_len_2_sched_time call is the only thing that logically
> > depends on q->rate, so move the now/delay adjustment out of the if.
> > 
> > Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
> > ---
> 
> Hi,
> 
> The documentation for netem does explicitly mention the reordering with
> jitter, and gives instructions on how to avoid it. (I have not tested if
> it works as intended).


Yes.

The user specifically adds a random delay of 0 to 510 ms to packets,
and expect netem to not reorder packets sent every 100ms.

They see netem as a single medium between two endpoints with a guarantee
of no reordering, and cumulative delays.

So if you send a burst of 100 packets, they might expect the last packet
will be send after ~50 seconds.

This clearly needs a new option to netem, because this is not the
default behavior we want.

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Ferry Huberts]

On 21/08/13 17:39, Eric Dumazet wrote:
> On Wed, 2013-08-21 at 17:17 +0200, Johannes Naab wrote:
>> On 08/20/2013 05:11 PM, Ferry Huberts wrote:
>>> From: Ferry Huberts <ferry.huberts@pelagic.nl>
>>>
>>> Not doing this (current behaviour) introduces reordering.
>>>
>>> The packet_len_2_sched_time call is the only thing that logically
>>> depends on q->rate, so move the now/delay adjustment out of the if.
>>>
>>> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
>>> ---
>>
>> Hi,
>>
>> The documentation for netem does explicitly mention the reordering with
>> jitter, and gives instructions on how to avoid it. (I have not tested if
>> it works as intended).
> 
> 
> Yes.
> 
> The user specifically adds a random delay of 0 to 510 ms to packets,
> and expect netem to not reorder packets sent every 100ms.
> 
> They see netem as a single medium between two endpoints with a guarantee
> of no reordering, and cumulative delays.

Well no. We expected no reordering because reordering is not enabled.

The documentation is very confusing if you compare it to the source
code, and even incorrect.

What the code does is (when reordering is disabled):
- reorders if the rate is NOT set
- does NOT reorder if the rate is set
That is quite different, the documentation doesn't even mention the rate
nor the reordering setting in this context.

I'm confused on how to proceed now, so CC-ing Hagen Paul Pfeifer

I'll also discuss this with Teco, who asked me to write up a patch.

> 
> So if you send a burst of 100 packets, they might expect the last packet
> will be send after ~50 seconds.
> 
> This clearly needs a new option to netem, because this is not the
> default behavior we want.
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Ferry Huberts

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Eric Dumazet]

On Wed, 2013-08-21 at 18:14 +0200, Ferry Huberts wrote:

> Well no. We expected no reordering because reordering is not enabled.


Sending packets with random delays happening in the 'network' _will_
reorder packets at the receiver.

The 'reorder' netem attribute is quite limited and not practical,
because it only queues the packet at the head of the queue instead of
tail. This is not what happens on the networks.

You want something very special, and this needs a new parameter to netem
qdisc, or a new qdisc.

If I setup "netem rate 1Mbit delay 1000ms 50ms", and send a burst of 100
small packets, I expect these _all_ packets reach the destination in
less than 1050ms.

I do not want packet1 being delivered at t0+1020ms,
packet2 being delivered at t0+1020+1030ms
packet100 being delivered at t0+1020+1030++...+ = t0+~100sec

If your patch solves the problem, good, but I see no clear test of this.

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Stephen Hemminger]

On Wed, 21 Aug 2013 10:00:17 -0700
Eric Dumazet <eric.dumazet@gmail.com> wrote:

> On Wed, 2013-08-21 at 18:14 +0200, Ferry Huberts wrote:
> 
> > Well no. We expected no reordering because reordering is not enabled.
> 
> 
> Sending packets with random delays happening in the 'network' _will_
> reorder packets at the receiver.
> 
> The 'reorder' netem attribute is quite limited and not practical,
> because it only queues the packet at the head of the queue instead of
> tail. This is not what happens on the networks.
> 
> You want something very special, and this needs a new parameter to netem
> qdisc, or a new qdisc.
> 
> If I setup "netem rate 1Mbit delay 1000ms 50ms", and send a burst of 100
> small packets, I expect these _all_ packets reach the destination in
> less than 1050ms.
> 
> I do not want packet1 being delivered at t0+1020ms,
> packet2 being delivered at t0+1020+1030ms
> packet100 being delivered at t0+1020+1030++...+ = t0+~100sec
> 
> If your patch solves the problem, good, but I see no clear test of this.

The current behavior followed what NISTnet did. At the time, I wanted
NISTnet users to be able to use netem without a lot of surprises.
  http://www-x.antd.nist.gov/nistnet/

Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering

[Ferry Huberts]

On 21/08/13 18:14, Ferry Huberts wrote:
> 
> 
> On 21/08/13 17:39, Eric Dumazet wrote:
>> On Wed, 2013-08-21 at 17:17 +0200, Johannes Naab wrote:
>>> On 08/20/2013 05:11 PM, Ferry Huberts wrote:
>>>> From: Ferry Huberts <ferry.huberts@pelagic.nl>
>>>>
>>>> Not doing this (current behaviour) introduces reordering.
>>>>
>>>> The packet_len_2_sched_time call is the only thing that logically
>>>> depends on q->rate, so move the now/delay adjustment out of the if.
>>>>
>>>> Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
>>>> ---
>>>
>>> Hi,
>>>
>>> The documentation for netem does explicitly mention the reordering with
>>> jitter, and gives instructions on how to avoid it. (I have not tested if
>>> it works as intended).
>>
>>
>> Yes.
>>
>> The user specifically adds a random delay of 0 to 510 ms to packets,
>> and expect netem to not reorder packets sent every 100ms.
>>
>> They see netem as a single medium between two endpoints with a guarantee
>> of no reordering, and cumulative delays.
> 
> Well no. We expected no reordering because reordering is not enabled.
> 
> The documentation is very confusing if you compare it to the source
> code, and even incorrect.
> 
> What the code does is (when reordering is disabled):
> - reorders if the rate is NOT set
> - does NOT reorder if the rate is set
> That is quite different, the documentation doesn't even mention the rate
> nor the reordering setting in this context.
> 
> I'm confused on how to proceed now, so CC-ing Hagen Paul Pfeifer
> 
> I'll also discuss this with Teco, who asked me to write up a patch.
> 

I discussed this issue with Teco and we decided to drop thes patches.

We still feel that the behaviour is unexpected and that (at least) the
documentation should be updated to reflect the actual behaviour.

We're going with the fifo approach for now.

So thanks for the feedback and discussions.
And apologies for any disturbances ;-)


Ferry