On Thu, 2012-01-05 at 17:46 +0000, Eric Dumazet wrote:

Le jeudi 05 janvier 2012 à 17:13 +0000, Ian Campbell a écrit :

quoted

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index da0c97f..b6de604 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -189,8 +189,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	 * aligned memory blocks, unless SLUB/SLAB debug is enabled.
 	 * Both skb->head and skb_shared_info are cache line aligned.
 	 */
-	size = SKB_DATA_ALIGN(size);
-	size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+	size = SKB_DATA_ALIGN(size + sizeof(struct skb_shared_info));
 	data = kmalloc_node_track_caller(size, gfp_mask, node);
 	if (!data)
 		goto nodata;

Unfortunately this makes the frequently use part of skb_shared_info not
any more in a single cache line.

This will slow down many operations like kfree_skb() of cold skbs (TX
completion for example)

I've been toying with the following which moves the infrequently used
destructor_arg field to the front. With 32 or 64 byte cache lines this
is then the only field which ends up on a different cache line. With 128
byte cache lines it is all already on the same line.

Does that alleviate your concern? If so I can clean it up and include it
in the series.

Cheers,
Ian.

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index cbc815c..533c2ea 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -249,6 +249,11 @@ 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;

@@ -264,9 +269,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];

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e942a86..592b8a8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -219,7 +219,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);