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.
diff --git a/include/net/gro_cells.h b/include/net/gro_cells.h
index cf6c74550baa..124b8a5537e3 100644
--- a/include/net/gro_cells.h
+++ b/include/net/gro_cells.h
@@ -19,7 +19,10 @@ static inline void gro_cells_receive(struct gro_cells
*gcells, struct sk_buff *s
struct gro_cell *cell;
struct net_device *dev = skb->dev;
- if (!gcells->cells || skb_cloned(skb) || !(dev->features &
NETIF_F_GRO)) {
+ if (!gcells->cells ||
+ skb->_skb_refdst ||
+ skb_cloned(skb) ||
+ !(dev->features & NETIF_F_GRO)) {
netif_rx(skb);
return;
}
