* [PATCH 1/2] net: netem: do not reorder when reordering is disabled @ 2013-08-20 15:11 Ferry Huberts 2013-08-20 15:11 ` [PATCH 2/2] net: netem: always adjust now/delay when not reordering Ferry Huberts 0 siblings, 1 reply; 16+ messages in thread From: Ferry Huberts @ 2013-08-20 15:11 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-20 15:11 [PATCH 1/2] net: netem: do not reorder when reordering is disabled Ferry Huberts @ 2013-08-20 15:11 ` Ferry Huberts 2013-08-20 18:31 ` Sergei Shtylyov ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Ferry Huberts @ 2013-08-20 15:11 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-20 15:11 ` [PATCH 2/2] net: netem: always adjust now/delay when not reordering Ferry Huberts @ 2013-08-20 18:31 ` Sergei Shtylyov 2013-08-20 20:33 ` Eric Dumazet 2013-08-21 15:17 ` [PATCH " Johannes Naab 2 siblings, 0 replies; 16+ messages in thread From: Sergei Shtylyov @ 2013-08-20 18:31 UTC (permalink / raw) To: Ferry Huberts; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-20 15:11 ` [PATCH 2/2] net: netem: always adjust now/delay when not reordering Ferry Huberts 2013-08-20 18:31 ` Sergei Shtylyov @ 2013-08-20 20:33 ` Eric Dumazet 2013-08-21 5:59 ` [PATCH v2 " Ferry Huberts 2013-08-21 15:17 ` [PATCH " Johannes Naab 2 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2013-08-20 20:33 UTC (permalink / raw) To: Ferry Huberts; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-20 20:33 ` Eric Dumazet @ 2013-08-21 5:59 ` Ferry Huberts 2013-08-21 6:14 ` Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Ferry Huberts @ 2013-08-21 5:59 UTC (permalink / raw) To: netdev 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 5:59 ` [PATCH v2 " Ferry Huberts @ 2013-08-21 6:14 ` Eric Dumazet 2013-08-21 7:04 ` Ferry Huberts 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2013-08-21 6:14 UTC (permalink / raw) To: Ferry Huberts; +Cc: netdev 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 6:14 ` Eric Dumazet @ 2013-08-21 7:04 ` Ferry Huberts 2013-08-21 13:30 ` Eric Dumazet 0 siblings, 1 reply; 16+ messages in thread From: Ferry Huberts @ 2013-08-21 7:04 UTC (permalink / raw) To: Eric Dumazet; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 7:04 ` Ferry Huberts @ 2013-08-21 13:30 ` Eric Dumazet 2013-08-21 14:02 ` mailings 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2013-08-21 13:30 UTC (permalink / raw) To: Ferry Huberts; +Cc: netdev 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 13:30 ` Eric Dumazet @ 2013-08-21 14:02 ` mailings 2013-08-21 14:10 ` mailings 0 siblings, 1 reply; 16+ messages in thread From: mailings @ 2013-08-21 14:02 UTC (permalink / raw) To: Eric Dumazet; +Cc: Ferry Huberts, netdev > 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 14:02 ` mailings @ 2013-08-21 14:10 ` mailings 0 siblings, 0 replies; 16+ messages in thread From: mailings @ 2013-08-21 14:10 UTC (permalink / raw) To: mailings; +Cc: Eric Dumazet, Ferry Huberts, netdev >> 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-20 15:11 ` [PATCH 2/2] net: netem: always adjust now/delay when not reordering Ferry Huberts 2013-08-20 18:31 ` Sergei Shtylyov 2013-08-20 20:33 ` Eric Dumazet @ 2013-08-21 15:17 ` Johannes Naab 2013-08-21 15:39 ` Eric Dumazet 2 siblings, 1 reply; 16+ messages in thread From: Johannes Naab @ 2013-08-21 15:17 UTC (permalink / raw) To: mailings; +Cc: netdev, eric.dumazet 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 -- ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 15:17 ` [PATCH " Johannes Naab @ 2013-08-21 15:39 ` Eric Dumazet 2013-08-21 16:14 ` Ferry Huberts 0 siblings, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2013-08-21 15:39 UTC (permalink / raw) To: Johannes Naab; +Cc: mailings, netdev 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 15:39 ` Eric Dumazet @ 2013-08-21 16:14 ` Ferry Huberts 2013-08-21 17:00 ` Eric Dumazet 2013-08-23 12:50 ` Ferry Huberts 0 siblings, 2 replies; 16+ messages in thread From: Ferry Huberts @ 2013-08-21 16:14 UTC (permalink / raw) To: Eric Dumazet; +Cc: Johannes Naab, netdev, hagen 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 16:14 ` Ferry Huberts @ 2013-08-21 17:00 ` Eric Dumazet 2013-08-21 17:35 ` Stephen Hemminger 2013-08-23 12:50 ` Ferry Huberts 1 sibling, 1 reply; 16+ messages in thread From: Eric Dumazet @ 2013-08-21 17:00 UTC (permalink / raw) To: Ferry Huberts; +Cc: Johannes Naab, netdev, hagen 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. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 17:00 ` Eric Dumazet @ 2013-08-21 17:35 ` Stephen Hemminger 0 siblings, 0 replies; 16+ messages in thread From: Stephen Hemminger @ 2013-08-21 17:35 UTC (permalink / raw) To: Eric Dumazet; +Cc: Ferry Huberts, Johannes Naab, netdev, hagen 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/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] net: netem: always adjust now/delay when not reordering 2013-08-21 16:14 ` Ferry Huberts 2013-08-21 17:00 ` Eric Dumazet @ 2013-08-23 12:50 ` Ferry Huberts 1 sibling, 0 replies; 16+ messages in thread From: Ferry Huberts @ 2013-08-23 12:50 UTC (permalink / raw) To: Eric Dumazet; +Cc: Johannes Naab, netdev, hagen 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-08-23 12:50 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-08-20 15:11 [PATCH 1/2] net: netem: do not reorder when reordering is disabled Ferry Huberts 2013-08-20 15:11 ` [PATCH 2/2] net: netem: always adjust now/delay when not reordering Ferry Huberts 2013-08-20 18:31 ` Sergei Shtylyov 2013-08-20 20:33 ` Eric Dumazet 2013-08-21 5:59 ` [PATCH v2 " Ferry Huberts 2013-08-21 6:14 ` Eric Dumazet 2013-08-21 7:04 ` Ferry Huberts 2013-08-21 13:30 ` Eric Dumazet 2013-08-21 14:02 ` mailings 2013-08-21 14:10 ` mailings 2013-08-21 15:17 ` [PATCH " Johannes Naab 2013-08-21 15:39 ` Eric Dumazet 2013-08-21 16:14 ` Ferry Huberts 2013-08-21 17:00 ` Eric Dumazet 2013-08-21 17:35 ` Stephen Hemminger 2013-08-23 12:50 ` Ferry Huberts
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.