On Wed, Jan 20, 2016 at 4:07 PM, Eric Dumazet <eric.duma...@gmail.com> wrote:
> On Wed, 2016-01-20 at 16:43 -0700, John wrote:
>>
>> On 01/19/2016 06:31 PM, Thomas Graf wrote:
>> > On 01/19/16 at 04:51pm, Jesse Gross wrote:
>> >> On Tue, Jan 19, 2016 at 4:17 PM, Eric Dumazet <eric.duma...@gmail.com>
>> >> wrote:
>> >>> So what is the purpose of having a dst if we need to drop it ?
>> >>>
>> >>> Adding code in GRO would be fine if someone explains me the purpose of
>> >>> doing apparently useless work.
>> >>>
>> >>> (refcounting on dst is not exactly free)
>> >> In the GRO case, the dst is only dropped on the packets which have
>> >> been merged and therefore need to be freed (the GRO_MERGED_FREE case).
>> >> It's not being thrown away for the overall frame, just metadata that
>> >> has been duplicated on each individual frame, similar to the metadata
>> >> in struct sk_buff itself. And while it is not used by the IP stack
>> >> there are other consumers (eBPF/OVS/etc.). This entire process is
>> >> controlled by the COLLECT_METADATA flag on tunnels, so there is no
>> >> cost in situations where it is not actually used.
>> > Right. There were thoughts around leveraging a per CPU scratch
>> > buffer without a refcount and turn it into a full reference when
>> > the packet gets enqueued somewhere but the need hasn't really come
>> > up yet.
>> >
>> > Jesse, is this what you have in mind:
>> >
>> > diff --git a/net/core/dev.c b/net/core/dev.c
>> > index cc9e365..3a5e96d 100644
>> > --- a/net/core/dev.c
>> > +++ b/net/core/dev.c
>> > @@ -4548,9 +4548,10 @@ static gro_result_t napi_skb_finish(gro_result_t
>> > ret, struct sk_buff *skb)
>> > break;
>> >
>> > case GRO_MERGED_FREE:
>> > - if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD)
>> > + if (NAPI_GRO_CB(skb)->free == NAPI_GRO_FREE_STOLEN_HEAD) {
>> > + skb_release_head_state(skb);
>> > kmem_cache_free(skbuff_head_cache, skb);
>> > - else
>> > + } else
>> > __kfree_skb(skb);
>> > break;
>> So I've tested the below patch (same as one above with minor
>> modifications made to make it compile) and it worked - no memory leak.
>> Should I submit this or...?
>
> Unfortunately fix is not complete.
>
> As someone mentioned, GRO should not aggregate packets having different
> dst.
>
> This part is hard to achieve, as a pointer comparison wont be enough :
> Each skb has its own meta dst allocation.
>
> Quite frankly, I would rather disable GRO for packets with a dst,
> instead of making GRO dog slow.
I have a patch that implements the comparison between dsts. For
packets without a dst, there isn't really a cost and if we do have a
dst then GRO is still a benefit. So it seems like it is worth doing,
even if it is more expensive than is ideal.
