[Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Dave Taht]

We've had a raft of deployment questions regarding fq_codel, memory
use, and the impact of unresponsive flows
over on the cerowrt development list.

sort of summary of the thread here, with some codel related ideas

https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000423.html

some early data on wifi interactions here

https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000407.html

-- 
Dave Täht
http://www.bufferbloat.net/projects/cerowrt/wiki - "3.3.8-17 is out
with fq_codel!"

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Eric Dumazet]

On Mon, 2012-08-20 at 14:05 -0700, Dave Taht wrote:
> We've had a raft of deployment questions regarding fq_codel, memory
> use, and the impact of unresponsive flows
> over on the cerowrt development list.
> 
> sort of summary of the thread here, with some codel related ideas
> 
> https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000423.html
> 
> some early data on wifi interactions here
> 
> https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000407.html
> 

CC netdev

codel / fq_codel have a packet limit per qdisc, like any other linux
qdisc.

DEFAULT_CODEL_LIMIT = 1000 packets

fq_codel : 10240 packets

For memory constrained boxes, it would be wise to add a 'truesize' limit

Not a bytes limit based on skb->len sum, because a skb->len = 60 bytes
packet might consume far more memory (more than 2 Kbytes)

ath9k driver is known to deliver very fat skbs (skb->truesize is too
big)

One 'other way' to limit bloat would be to reallocate skbs when one
qdisc begins to use more than 25 % of its truesize limit

enqueue(q, skb)
{
	if (q->truesize_sum > q->truesize_limit / 4)
		skb = skb_reduce_truesize(skb, GFP_ATOMIC);
	...
}

This is what some drivers already do in their rx handler (this is called
copybreak), but this would do the copy (and extra cpu cost) only in
stress situations.

Note : we also have a very big struct skb_shared_info (320 bytes), we
probably could shrink it for packets without fragments.

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Andrew McGregor]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]


On 21/08/2012, at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:

> ath9k driver is known to deliver very fat skbs (skb->truesize is too
> big)

What would it take to fix this?

Andrew

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 2330 bytes --]

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Eric Dumazet]

On Tue, 2012-08-21 at 22:27 +1200, Andrew McGregor wrote:
> On 21/08/2012, at 8:38 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 
> > ath9k driver is known to deliver very fat skbs (skb->truesize is too
> > big)
> 
> What would it take to fix this?
> 
> Andrew

Someone could use the following : 

https://gerrit.chromium.org/gerrit/#/c/18412/

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Dave Taht]

On Tue, Aug 21, 2012 at 1:38 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2012-08-20 at 14:05 -0700, Dave Taht wrote:
>> We've had a raft of deployment questions regarding fq_codel, memory
>> use, and the impact of unresponsive flows
>> over on the cerowrt development list.
>>
>> sort of summary of the thread here, with some codel related ideas
>>
>> https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000423.html
>>
>> some early data on wifi interactions here
>>
>> https://lists.bufferbloat.net/pipermail/cerowrt-devel/2012-August/000407.html
>>
>
> CC netdev
>
> codel / fq_codel have a packet limit per qdisc, like any other linux
> qdisc.
>
> DEFAULT_CODEL_LIMIT = 1000 packets
>
> fq_codel : 10240 packets
>
> For memory constrained boxes, it would be wise to add a 'truesize' limit

From looking over the history of this idea, it does seem to be a good
idea for small devices with potentially big queues.

http://www.spinics.net/lists/netdev/msg176967.html

That said I do tend to agree with davem's summary in fixing the
wifi drivers in the first place. The current allocation in the ath9k driver
doesn't make any sense in the first place, which (to me) implies magic
lies underneath that I'm reluctant to fiddle with, without deep knowledge
of how the ath9k driver behaves with wep/wpa/ampdus, etc.


> Not a bytes limit based on skb->len sum, because a skb->len = 60 bytes
> packet might consume far more memory (more than 2 Kbytes)
>
> ath9k driver is known to deliver very fat skbs (skb->truesize is too
> big)
>
> One 'other way' to limit bloat would be to reallocate skbs when one
> qdisc begins to use more than 25 % of its truesize limit
>
> enqueue(q, skb)
> {
>         if (q->truesize_sum > q->truesize_limit / 4)
>                 skb = skb_reduce_truesize(skb, GFP_ATOMIC);
>         ...
> }

This basically involves adding truesize accounting to the codel qdisc
and settling on the api proposed above, which differs from the
originally proposed patch. Is there a reason why we would
ever not try this atomically and is this form correct for 3.3 and
later kernels?

(am about to hack this into skbuff.h and codel.h as I write)


+/*
+ * Caller want to reduce memory needs before queueing skb
+ * The (expensive) copy should not be be done in fast path.
+ */
+static struct sk_buff *skb_reduce_truesize(struct sk_buff *skb)
+{
+	if (skb->truesize > 2 * SKB_TRUESIZE(skb->len)) {
+		struct sk_buff *nskb;
+
+		nskb = skb_copy_expand(skb, skb_headroom(skb), 0,
+				       GFP_ATOMIC | __GFP_NOWARN);
+		if (nskb) {
+			__kfree_skb(skb);
+			skb = nskb;
+		}
+	}
+	return skb;
+}





> This is what some drivers already do in their rx handler (this is called
> copybreak), but this would do the copy (and extra cpu cost) only in
> stress situations.
>
> Note : we also have a very big struct skb_shared_info (320 bytes), we
> probably could shrink it for packets without fragments.

The ath9k allocator failing is at 19xx bytes.

-- 
Dave Täht
http://www.bufferbloat.net/projects/cerowrt/wiki - "3.3.8-17 is out
with fq_codel!"

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Eric Dumazet]

On Sun, 2012-08-26 at 14:36 -0700, Dave Taht wrote:

> From looking over the history of this idea, it does seem to be a good
> idea for small devices with potentially big queues.
> 
> http://www.spinics.net/lists/netdev/msg176967.html
> 
> That said I do tend to agree with davem's summary in fixing the
> wifi drivers in the first place. The current allocation in the ath9k driver
> doesn't make any sense in the first place, which (to me) implies magic
> lies underneath that I'm reluctant to fiddle with, without deep knowledge
> of how the ath9k driver behaves with wep/wpa/ampdus, etc.
> 

Problem is some hardware cannot do this in a smart way, without paying
the price of a sometime expensive copy.

Thats why I refined this idea to actually trigger only if current
memory needs are above a threshold.

If packets are received and immediately consumed, without potentially
staying a long time in a queue, it doesnt really matter they use 200 or
400% more ram than the rightly sized packets.

Re: [Codel] coping with memory limitations and packet flooding in codel and fq_codel

[Dave Taht]

On Mon, Aug 27, 2012 at 4:37 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Sun, 2012-08-26 at 14:36 -0700, Dave Taht wrote:
>
>> From looking over the history of this idea, it does seem to be a good
>> idea for small devices with potentially big queues.
>>
>> http://www.spinics.net/lists/netdev/msg176967.html
>>
>> That said I do tend to agree with davem's summary in fixing the
>> wifi drivers in the first place. The current allocation in the ath9k driver
>> doesn't make any sense in the first place, which (to me) implies magic
>> lies underneath that I'm reluctant to fiddle with, without deep knowledge
>> of how the ath9k driver behaves with wep/wpa/ampdus, etc.
>>
>
> Problem is some hardware cannot do this in a smart way, without paying
> the price of a sometime expensive copy.
>
> Thats why I refined this idea to actually trigger only if current
> memory needs are above a threshold.

Concur. However, rather than implement truesize +/- accounting in the
qdisc, which seemed potentially hairy, I just did a:

static int codel_qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch)
{
        struct codel_sched_data *q = qdisc_priv(sch);
        int qlen;
        if (likely((qlen = qdisc_qlen(sch)) < sch->limit)) {
                if (qlen > sch->limit / 8)
                        skb = skb_reduce_truesize(skb);

As I am also typically lowering the fq_codel limit to 1024 or 1200
from the default of 10k, this will kick in at a pretty appropriate
time, there too,
after I add the code...

And that said this stuff is totally unneeded on bigger iron. Last year
we were safely stuffing 10s and 100s of MB into the tx queue rings
without issues... ( :)

> If packets are received and immediately consumed, without potentially
> staying a long time in a queue, it doesnt really matter they use 200 or
> 400% more ram than the rightly sized packets.

+1
>
>
>



-- 
Dave Täht
http://www.bufferbloat.net/projects/cerowrt/wiki - "3.3.8-17 is out
with fq_codel!"