Skip to contentSkip to conversation

net: fix kmemcheck annotations

PATCH In review Eric Dumazet 2 participants 4 messages Oct 29, 2009 – Oct 30, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Eric Dumazet

author
#0 lore raw

PATCH 1/1 net: fix kmemcheck annotations

struct sk_buff kmemcheck annotations enlarged this structure by 8/16 bytes

Fix this by moving 'protocol' inside flags1 bitfield, and queue_mapping inside flags2 bitfield.

Signed-off-by Eric Dumazet <[email protected]>

---

include/linux/skbuff.h +2 -2
index 0c68fbd..defd51d 100644
@@ -354,8 +354,8 @@ struct sk_buff {
 				ipvs_property:1,
 				peeked:1,
 				nf_trace:1;
+	__be16			protocol:16;
 	kmemcheck_bitfield_end(flags1);
-	__be16			protocol;
 
 	void			(*destructor)(struct sk_buff *skb);
 #if defined(CONFIG_NF_CONNTRACK) || defined(CONFIG_NF_CONNTRACK_MODULE)

@@ -367,7 +367,6 @@ struct sk_buff {
 #endif
 
 	int			iif;
-	__u16			queue_mapping;
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */
 #ifdef CONFIG_NET_CLS_ACT

@@ -376,6 +375,7 @@ struct sk_buff {
 #endif
 
 	kmemcheck_bitfield_begin(flags2);
+	__u16			queue_mapping:16;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	__u8			ndisc_nodetype:2;
 #endif
maintainer
#1 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 29 Oct 2009 11:10:37 +0100

Quoted 9 lines
include/linux/skbuff.h +0 -1
@@ -367,7 +367,6 @@ struct sk_buff {
 #endif
 
 	int			iif;
-	__u16			queue_mapping;
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */
 #ifdef CONFIG_NET_CLS_ACT

include/linux/skbuff.h struct sk_buff { · line 381

Quoted 7 lines
include/linux/skbuff.h +1 -0
@@ -376,6 +375,7 @@ struct sk_buff {
 #endif
 
 	kmemcheck_bitfield_begin(flags2);
+	__u16			queue_mapping:16;
 #ifdef CONFIG_IPV6_NDISC_NODETYPE
 	__u8			ndisc_nodetype:2;
 #endif

We may be trading size for performance here, I wonder if it's wise to move queue_mapping like that and what locality change we get as a result.

author
#2 lore raw
Quoted 4 lines · David Miller

We may be trading size for performance here, I wonder if it's wise to move queue_mapping like that and what locality change we get as a result.

I agree, but could not convince me it makes a difference.

On 64bit arches, queue_mapping doesnt change its cache line location (0xa4 -> 0xa8)

On 32bit arches, sizeof(sk_buf)=0xB0, rx and tx paths touch all
of 3 cache lines anyway.

Only thing I can think at this moment is to reorder skb so that TX completion path dont touch all cache lines (putting together cb[] and some not touched fields). We could probably gain one cache line miss.

It's a bit hard because of the next/prev fields that must
be first members of structure, but I believe you had some work in progress in this area, to stick a standard list_head ?

maintainer
#3 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 29 Oct 2009 11:44:48 +0100

Quoted 7 lines
Quoted 4 lines · David Miller

We may be trading size for performance here, I wonder if it's wise to move queue_mapping like that and what locality change we get as a result.

I agree, but could not convince me it makes a difference.

Ok, I'll apply your patch as-is to net-2.6, thanks!