Re: [Xen-devel] [PATCH 05/10] net: move destructor_arg to the front of sk_buff.

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-04-10 at 16:05 +0100, Eric Dumazet wrote:
> On Tue, 2012-04-10 at 15:26 +0100, Ian Campbell wrote:
> > As of the previous patch we align the end (rather than the start) of the 
> > struct
> > to a cache line and so, with 32 and 64 byte cache lines and the shinfo size
> > increase from the next patch, the first 8 bytes of the struct end up on a
> > different cache line to the rest of it so make sure it is something 
> > relatively
> > unimportant to avoid hitting an extra cache line on hot operations such as
> > kfree_skb.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> > Cc: Eric Dumazet <eric.dumazet@xxxxxxxxx>
> > ---
> >  include/linux/skbuff.h |   15 ++++++++++-----
> >  net/core/skbuff.c      |    5 ++++-
> >  2 files changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 0ad6a46..f0ae39c 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -265,6 +265,15 @@ struct ubuf_info {
> >   * the end of the header data, ie. at skb->end.
> >   */
> >  struct skb_shared_info {
> > +   /* Intermediate layers must ensure that destructor_arg
> > +    * remains valid until skb destructor */
> > +   void            *destructor_arg;
> > +
> > +   /*
> > +    * Warning: all fields from here until dataref are cleared in
> > +    * __alloc_skb()
> > +    *
> > +    */
> >     unsigned char   nr_frags;
> >     __u8            tx_flags;
> >     unsigned short  gso_size;
> > @@ -276,14 +285,10 @@ struct skb_shared_info {
> >     __be32          ip6_frag_id;
> >  
> >     /*
> > -    * Warning : all fields before dataref are cleared in __alloc_skb()
> > +    * Warning: all fields before dataref are cleared in __alloc_skb()
> >      */
> >     atomic_t        dataref;
> >  
> > -   /* Intermediate layers must ensure that destructor_arg
> > -    * remains valid until skb destructor */
> > -   void *          destructor_arg;
> > -
> >     /* must be last field, see pskb_expand_head() */
> >     skb_frag_t      frags[MAX_SKB_FRAGS];
> >  };
> > diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> > index d4e139e..b8a41d6 100644
> > --- a/net/core/skbuff.c
> > +++ b/net/core/skbuff.c
> > @@ -214,7 +214,10 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t 
> > gfp_mask,
> >  
> >     /* make sure we initialize shinfo sequentially */
> >     shinfo = skb_shinfo(skb);
> > -   memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
> > +
> > +   memset(&shinfo->nr_frags, 0,
> > +          offsetof(struct skb_shared_info, dataref)
> > +          - offsetof(struct skb_shared_info, nr_frags));
> >     atomic_set(&shinfo->dataref, 1);
> >     kmemcheck_annotate_variable(shinfo->destructor_arg);
> >  
> 
> Not sure if we can do the same in build_skb()

I don't think there's any chance of there being a destructor_arg to
preserve in that case?

Regardless of that though I think for consistency it would be worth
pulling the common shinfo init out into a helper and using it in both
places.

I'll make that change.

Ian.

> 
> 



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel