>-----Original Message----- >From: Eric Dumazet [mailto:eric.dumazet@xxxxxxxxx] >Sent: Monday, November 08, 2010 4:25 PM >To: Xin, Xiaohui >Cc: netdev@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >mst@xxxxxxxxxx; mingo@xxxxxxx; davem@xxxxxxxxxxxxx; herbert@xxxxxxxxxxxxxxxxxxx; >jdike@xxxxxxxxxxxxxxx >Subject: Re: Re:[PATCH v14 06/17] Use callback to deal with skb_release_data() specially. > >Le lundi 08 novembre 2010 à 16:03 +0800, xiaohui.xin@xxxxxxxxx a écrit : >> From: Xin Xiaohui <xiaohui.xin@xxxxxxxxx> >> >> >> Hmm, I suggest you read the comment two lines above. >> >> >> >> If destructor_arg is now cleared each time we allocate a new skb, then, >> >> please move it before dataref in shinfo structure, so that the following >> >> memset() does the job efficiently... >> > >> > >> >Something like : >> > >> >diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h >> >index e6ba898..2dca504 100644 >> >--- a/include/linux/skbuff.h >> >+++ b/include/linux/skbuff.h >> >@@ -195,6 +195,9 @@ struct skb_shared_info { >> > __be32 ip6_frag_id; >> > __u8 tx_flags; >> > struct sk_buff *frag_list; >> >+ /* Intermediate layers must ensure that destructor_arg >> >+ * remains valid until skb destructor */ >> >+ void *destructor_arg; >> > struct skb_shared_hwtstamps hwtstamps; >> > >> > /* >> >@@ -202,9 +205,6 @@ struct skb_shared_info { >> > */ >> > 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]; >> > }; >> > >> > >> >> Will that affect the cache line? > >What do you mean ? > >> Or, we can move the line to clear destructor_arg to the end of __alloc_skb(). >> It looks like as the following, which one do you prefer? >> >> Thanks >> Xiaohui >> >> --- >> net/core/skbuff.c | 8 ++++++++ >> 1 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >> index c83b421..df852f2 100644 >> --- a/net/core/skbuff.c >> +++ b/net/core/skbuff.c >> @@ -224,6 +224,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, >> >> child->fclone = SKB_FCLONE_UNAVAILABLE; >> } >> + shinfo->destructor_arg = NULL; >> out: >> return skb; >> nodata: > >I dont understand why you want to do this. > >This adds an instruction, makes code bigger, and no obvious gain for me, >at memory transactions side. > >If integrated in the existing memset(), cost is an extra iteration to >perform the clear of this field. > Ok. Thanks for this explanation and will update with your solution. Thanks Xiaohui -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
