[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 at 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!"