* [PATCH net] net: fq_codel: Fix off-by-one error
@ 2013-03-28 23:52 Vijay Subramanian
2013-03-29 15:01 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Vijay Subramanian @ 2013-03-28 23:52 UTC (permalink / raw)
To: netdev; +Cc: davem, eric.dumazet, Vijay Subramanian
Currently, we hold a max of sch->limit -1 number of packets instead of
sch->limit packets. Fix this off-by-one error.
Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
---
net/sched/sch_fq_codel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
index 4e606fc..5578628 100644
--- a/net/sched/sch_fq_codel.c
+++ b/net/sched/sch_fq_codel.c
@@ -195,7 +195,7 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
flow->deficit = q->quantum;
flow->dropped = 0;
}
- if (++sch->q.qlen < sch->limit)
+ if (++sch->q.qlen <= sch->limit)
return NET_XMIT_SUCCESS;
q->drop_overlimit++;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-28 23:52 [PATCH net] net: fq_codel: Fix off-by-one error Vijay Subramanian
@ 2013-03-29 15:01 ` Eric Dumazet
2013-03-29 19:33 ` David Miller
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Eric Dumazet @ 2013-03-29 15:01 UTC (permalink / raw)
To: Vijay Subramanian; +Cc: netdev, davem
On Thu, 2013-03-28 at 16:52 -0700, Vijay Subramanian wrote:
> Currently, we hold a max of sch->limit -1 number of packets instead of
> sch->limit packets. Fix this off-by-one error.
>
> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
> ---
> net/sched/sch_fq_codel.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_fq_codel.c b/net/sched/sch_fq_codel.c
> index 4e606fc..5578628 100644
> --- a/net/sched/sch_fq_codel.c
> +++ b/net/sched/sch_fq_codel.c
> @@ -195,7 +195,7 @@ static int fq_codel_enqueue(struct sk_buff *skb, struct Qdisc *sch)
> flow->deficit = q->quantum;
> flow->dropped = 0;
> }
> - if (++sch->q.qlen < sch->limit)
> + if (++sch->q.qlen <= sch->limit)
> return NET_XMIT_SUCCESS;
>
> q->drop_overlimit++;
Just curious, did you play changing the default limit (10240 packets) ?
Acked-by: Eric Dumazet <edumazet@google.com>
Thanks !
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-29 15:01 ` Eric Dumazet
@ 2013-03-29 19:33 ` David Miller
2013-03-29 21:49 ` Vijay Subramanian
2013-03-30 6:53 ` Markus Trippelsdorf
2 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2013-03-29 19:33 UTC (permalink / raw)
To: eric.dumazet; +Cc: subramanian.vijay, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 29 Mar 2013 08:01:20 -0700
> On Thu, 2013-03-28 at 16:52 -0700, Vijay Subramanian wrote:
>> Currently, we hold a max of sch->limit -1 number of packets instead of
>> sch->limit packets. Fix this off-by-one error.
>>
>> Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com>
...
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied and queued up for -stable.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-29 15:01 ` Eric Dumazet
2013-03-29 19:33 ` David Miller
@ 2013-03-29 21:49 ` Vijay Subramanian
2013-03-30 6:53 ` Markus Trippelsdorf
2 siblings, 0 replies; 8+ messages in thread
From: Vijay Subramanian @ 2013-03-29 21:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev, davem
> Just curious, did you play changing the default limit (10240 packets) ?
>
>
Eric,
So far we have not changed the default but we are planning to run some
more tests next week with fq_codel.
We can try different values for limit. If you have any other
suggestions for testing, please let me know.
Thanks!
Vijay
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-29 15:01 ` Eric Dumazet
2013-03-29 19:33 ` David Miller
2013-03-29 21:49 ` Vijay Subramanian
@ 2013-03-30 6:53 ` Markus Trippelsdorf
2013-03-30 14:42 ` Eric Dumazet
2 siblings, 1 reply; 8+ messages in thread
From: Markus Trippelsdorf @ 2013-03-30 6:53 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Vijay Subramanian, netdev, davem
On 2013.03.29 at 08:01 -0700, Eric Dumazet wrote:
>
> Just curious, did you play changing the default limit (10240 packets) ?
I did some tests on my home router (running OpenWrt trunk) that is rate-
limited with hfsc to the speed of the cable modem.
My tests seem to indicate that lowering the default limit to 1024
packets results in much better latency behavior when using bittorrent.
With the default limit (10240 packets) I would get huge ping latencies
from 600-1200ms when downloading e.g.:
http://download.opensuse.org/distribution/12.3/iso/openSUSE-12.3-DVD-x86_64.iso.torrent
with hundreds of peers.
Setting the limit to 1024 did get the latencies back in check (20-30ms
with occasional spikes of ~100ms).
fq_codel limit 1024:
Host Loss% Snt Last Avg Best Wrst StDev
1. OpenWrt.lan 0.0% 179 0.2 0.3 0.2 1.0 0.1
2. ???
3. 83-169-179-150-isp.superkabel.de 0.6% 179 13.4 29.3 7.1 214.1 26.1
4. 88-134-192-0-dynip.superkabel.de 0.6% 179 13.5 28.2 7.8 213.9 26.3
5. 88-134-196-230-dynip.superkabel.de 1.1% 179 10.7 27.7 7.5 225.6 26.7
6. 88-134-203-141-dynip.superkabel.de 0.6% 179 18.8 30.4 7.6 216.7 25.2
88-134-203-10-dynip.superkabel.de
88-134-203-6-dynip.superkabel.de
88-134-203-14-dynip.superkabel.de
7. 88-134-201-5-dynip.superkabel.de 0.6% 179 8.4 37.1 7.5 453.5 50.6
8. ???
9. 209.85.249.182 0.6% 179 11.8 28.5 8.2 185.6 25.4
10. 66.249.95.175 0.6% 179 9.2 34.5 8.3 263.9 31.3
66.249.95.67
11. 64.233.174.29 0.6% 179 13.4 30.6 7.8 297.8 31.7
216.239.48.53
64.233.174.55
12. ???
13. google-public-dns-a.google.com 0.0% 178 13.7 29.7 7.6 312.7 33.8
fq_codel limit 10240:
Host Loss% Snt Last Avg Best Wrst StDev
1. OpenWrt.lan 0.0% 179 0.5 0.3 0.2 0.8 0.1
2. ???
3. 83-169-179-150-isp.superkabel.de 1.1% 179 13.4 98.9 7.6 888.4 170.0
4. 88-134-192-0-dynip.superkabel.de 1.1% 179 16.8 95.5 8.0 892.5 171.9
5. 88-134-196-230-dynip.superkabel.d 1.1% 179 12.8 96.2 7.7 913.8 173.8
6. 88-134-203-6-dynip.superkabel.de 2.2% 179 38.2 105.9 9.5 926.1 175.9
88-134-203-10-dynip.superkabel.de
88-134-203-14-dynip.superkabel.de
88-134-203-141-dynip.superkabel.de
7. 88-134-201-5-dynip.superkabel.de 1.7% 179 44.0 103.3 8.0 960.6 178.9
8. ???
9. 209.85.249.182 1.7% 179 20.8 98.7 7.7 900.0 176.1
10. 66.249.95.175 1.7% 179 17.0 100.2 8.1 861.9 174.9
66.249.95.67
11. 64.233.174.55 1.7% 179 50.3 102.6 7.7 826.6 171.7
216.239.48.53
64.233.174.29
12. ???
13. google-public-dns-a.google.com 1.1% 179 12.5 98.2 8.1 849.8 168.9
--
Markus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-30 6:53 ` Markus Trippelsdorf
@ 2013-03-30 14:42 ` Eric Dumazet
2013-03-30 15:08 ` Markus Trippelsdorf
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2013-03-30 14:42 UTC (permalink / raw)
To: Markus Trippelsdorf; +Cc: Vijay Subramanian, netdev, davem
On Sat, 2013-03-30 at 07:53 +0100, Markus Trippelsdorf wrote:
> On 2013.03.29 at 08:01 -0700, Eric Dumazet wrote:
> >
> > Just curious, did you play changing the default limit (10240 packets) ?
>
> I did some tests on my home router (running OpenWrt trunk) that is rate-
> limited with hfsc to the speed of the cable modem.
>
> My tests seem to indicate that lowering the default limit to 1024
> packets results in much better latency behavior when using bittorrent.
>
> With the default limit (10240 packets) I would get huge ping latencies
> from 600-1200ms when downloading e.g.:
> http://download.opensuse.org/distribution/12.3/iso/openSUSE-12.3-DVD-x86_64.iso.torrent
> with hundreds of peers.
>
> Setting the limit to 1024 did get the latencies back in check (20-30ms
> with occasional spikes of ~100ms).
Hi Markus
I am very bored having to explain {fq_}codel principles each time
someone does this kind of experiments.
Codel principle is _allowing_ bursts, as long as the queue is
controlled. Read Codel paper for details. TCP can be slow to lower the
queues, it takes several RTT. So observing large queues is quite normal
in your case.
Bittorent uses its own rate limiting technique, defeating
current cwnd control done in the TCP stack, because of a very known
problem
( http://www.ietf.org/id/draft-ietf-tcpm-newcwv-00.txt )
So if your goal is reducing latencies for a _given_ class of flows, just
use prio + 3 fq_codel, and classify your packets to make sure your
lovely ping packets are not dropped or behind long packets.
fq_codel by itself is not universal.
My question about fq_codel limit was related to something completely
different.
The default is 10240 packets. The logic behind is to control the queue
at dequeue, not enqueue. But we needed an safety limit to avoid OOM in
case rate of enqueue() is insane.
It could theoretically hurt a low end machine, in case a burst fills the
queue with big GSO packets. But then these low end machines should not
use GRO/TSO anyway (as this is against anti bufferbloat techniques)
Probably a better choice would have been to limit sum(skb->truesize), or
sum(skb->len) (aka current sch->qstats.backlog)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-30 14:42 ` Eric Dumazet
@ 2013-03-30 15:08 ` Markus Trippelsdorf
2013-03-30 15:28 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Markus Trippelsdorf @ 2013-03-30 15:08 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Vijay Subramanian, netdev, davem
On 2013.03.30 at 07:42 -0700, Eric Dumazet wrote:
> On Sat, 2013-03-30 at 07:53 +0100, Markus Trippelsdorf wrote:
> > On 2013.03.29 at 08:01 -0700, Eric Dumazet wrote:
> > >
> > > Just curious, did you play changing the default limit (10240 packets) ?
> >
> > I did some tests on my home router (running OpenWrt trunk) that is rate-
> > limited with hfsc to the speed of the cable modem.
> >
> > My tests seem to indicate that lowering the default limit to 1024
> > packets results in much better latency behavior when using bittorrent.
> >
> > With the default limit (10240 packets) I would get huge ping latencies
> > from 600-1200ms when downloading e.g.:
> > http://download.opensuse.org/distribution/12.3/iso/openSUSE-12.3-DVD-x86_64.iso.torrent
> > with hundreds of peers.
> >
> > Setting the limit to 1024 did get the latencies back in check (20-30ms
> > with occasional spikes of ~100ms).
>
> Bittorent uses its own rate limiting technique, defeating
> current cwnd control done in the TCP stack, because of a very known
> problem
>
> ( http://www.ietf.org/id/draft-ietf-tcpm-newcwv-00.txt )
>
> So if your goal is reducing latencies for a _given_ class of flows, just
> use prio + 3 fq_codel, and classify your packets to make sure your
> lovely ping packets are not dropped or behind long packets.
This is exactly the setup that I'm using right now (prio + 4 fq_codel
with bittorent set to low).
And setting the fq_codel limit to 1024 improves latency in this
situation.
That's all I wanted to communicate. If the result doesn't interest you,
just ignore my mail.
--
Markus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] net: fq_codel: Fix off-by-one error
2013-03-30 15:08 ` Markus Trippelsdorf
@ 2013-03-30 15:28 ` Eric Dumazet
0 siblings, 0 replies; 8+ messages in thread
From: Eric Dumazet @ 2013-03-30 15:28 UTC (permalink / raw)
To: Markus Trippelsdorf; +Cc: Vijay Subramanian, netdev, davem
On Sat, 2013-03-30 at 16:08 +0100, Markus Trippelsdorf wrote:
> This is exactly the setup that I'm using right now (prio + 4 fq_codel
> with bittorent set to low).
> And setting the fq_codel limit to 1024 improves latency in this
> situation.
> That's all I wanted to communicate. If the result doesn't interest you,
> just ignore my mail.
>
I didn't ignore your mail, I spent time from my Saturday to answer you.
If your prio setting was right, a limit of 10 should be enough for the
high prio queue, and a mere pfifo would be ok.
By definition, high prio packets should have a minimum latency (assuming
of course that BQL is enabled on your device)
Then, if all your packets land into he same prio queue, classification
is not correct.
If your link is oversubscribed (and Bittorent tends to push links to
over subscribed situation), then you want to increase drops by reducing
queue lengths.
fq_codel default limit is only a hint, like all defaults.
Some users want to increase it, others want to decrease it.
In your case, I suspect the number of flows is too large (and you get
hash collisions in fq), _and_ your ping packets land the crowded
fq_codel qdisc, instead of a small queue reserved for high prio packets.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-03-30 15:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-03-28 23:52 [PATCH net] net: fq_codel: Fix off-by-one error Vijay Subramanian
2013-03-29 15:01 ` Eric Dumazet
2013-03-29 19:33 ` David Miller
2013-03-29 21:49 ` Vijay Subramanian
2013-03-30 6:53 ` Markus Trippelsdorf
2013-03-30 14:42 ` Eric Dumazet
2013-03-30 15:08 ` Markus Trippelsdorf
2013-03-30 15:28 ` Eric Dumazet
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.