Skip to contentSkip to conversation

net,rcu: don't assume the size of struct rcu_head

PATCH Superseded Lai Jiangshan 3 participants 8 messages Mar 1, 2011 – Mar 2, 2011

Subsystems (3)
Networking Network drivers Memory management
lore
Share & tools
lore

Lai Jiangshan

author
#0 lore raw

PATCH 4/4 net,rcu: don't assume the size of struct rcu_head

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/net/dst.h +3 -10
index 93b0310..4ef6c4a 100644
@@ -62,8 +62,6 @@ struct dst_entry {
 	struct hh_cache		*hh;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
-#else
-	void			*__pad1;
 #endif
 	int			(*input)(struct sk_buff*);
 	int			(*output)(struct sk_buff*);

@@ -74,23 +72,18 @@ struct dst_entry {
 
 #ifdef CONFIG_NET_CLS_ROUTE
 	__u32			tclassid;
-#else
-	__u32			__pad2;
 #endif
 
 
 	/*
 	 * Align __refcnt to a 64 bytes alignment
 	 * (L1_CACHE_SIZE would be too much)
-	 */
-#ifdef CONFIG_64BIT
-	long			__pad_to_align_refcnt[1];
-#endif
-	/*
+	 *
 	 * __refcnt wants to be on a different cache line from
 	 * input/output/ops or performance tanks badly
 	 */
-	atomic_t		__refcnt;	/* client references	*/
+	atomic_t		__refcnt	/* client references	*/
+				__attribute__((aligned (64)));
 	int			__use;
 	unsigned long		lastuse;
 	union {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#1 lore raw

From: Lai Jiangshan <[email protected]>
Date: Tue, 01 Mar 2011 16:03:44 +0800

Quoted 8 lines

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Signed-off-by Lai Jiangshan <[email protected]>

We don't want to use the align if it's going to waste lots of space.

Instead we want to rearrange the structure so that the alignment comes more cheaply.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#2 lore raw
Quoted 10 lines · David Miller

From: Lai Jiangshan <[email protected]>
Date: Tue, 01 Mar 2011 16:03:44 +0800

Quoted 7 lines

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Lai Jiangshan <[email protected]>

We don't want to use the align if it's going to waste lots of space.

Instead we want to rearrange the structure so that the alignment comes more cheaply.

Oh well, I should have read your answer before sending mine :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#3 lore raw

PATCH v2 4/4 net,rcu: don't assume the size of struct rcu_head

In reply to David Miller
Quoted 10 lines · David Miller

From: Lai Jiangshan <[email protected]>
Date: Tue, 01 Mar 2011 16:03:44 +0800

Quoted 7 lines

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Lai Jiangshan <[email protected]>

We don't want to use the align if it's going to waste lots of space.

Instead we want to rearrange the structure so that the alignment comes more cheaply.

Subject: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding are hardly suit for the changes. So we rearrange the structure, and move the seldom access rcu_head to the end of the structure.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/net/dst.h +8 -8
index 93b0310..d8c5296 100644
@@ -37,7 +37,6 @@
 struct sk_buff;
 
 struct dst_entry {
-	struct rcu_head		rcu_head;
 	struct dst_entry	*child;
 	struct net_device       *dev;
 	short			error;

@@ -78,6 +77,13 @@ struct dst_entry {
 	__u32			__pad2;
 #endif
 
+	unsigned long		lastuse;
+	union {
+		struct dst_entry	*next;
+		struct rtable __rcu	*rt_next;
+		struct rt6_info		*rt6_next;
+		struct dn_route __rcu	*dn_next;
+	};
 
 	/*
 	 * Align __refcnt to a 64 bytes alignment

@@ -92,13 +98,7 @@ struct dst_entry {
 	 */
 	atomic_t		__refcnt;	/* client references	*/
 	int			__use;
-	unsigned long		lastuse;
-	union {
-		struct dst_entry	*next;
-		struct rtable __rcu	*rt_next;
-		struct rt6_info		*rt6_next;
-		struct dn_route __rcu	*dn_next;
-	};
+	struct rcu_head		rcu_head;
 };
 
 #ifdef __KERNEL__
#4 lore raw
Quoted 61 lines · Lai Jiangshan
Quoted 10 lines · David Miller

From: Lai Jiangshan <[email protected]>
Date: Tue, 01 Mar 2011 16:03:44 +0800

Quoted 7 lines

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Lai Jiangshan <[email protected]>

We don't want to use the align if it's going to waste lots of space.

Instead we want to rearrange the structure so that the alignment comes more cheaply.

Subject: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding are hardly suit for the changes. So we rearrange the structure, and move the seldom access rcu_head to the end of the structure.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/net/dst.h +7 -2
index 93b0310..d8c5296 100644
@@ -37,7 +37,6 @@
 struct sk_buff;
 
 struct dst_entry {
-	struct rcu_head		rcu_head;
 	struct dst_entry	*child;
 	struct net_device       *dev;
 	short			error;

@@ -78,6 +77,13 @@ struct dst_entry {
 	__u32			__pad2;
 #endif
 
+	unsigned long		lastuse;
+	union {
+		struct dst_entry	*next;
+		struct rtable __rcu	*rt_next;
+		struct rt6_info		*rt6_next;
+		struct dn_route __rcu	*dn_next;
+	};
 
 	/*
 	 * Align __refcnt to a 64 bytes alignment

@@ -92,13 +98,7 @@ struct dst_entry {
 	 */
 	atomic_t		__refcnt;	/* client references	*/
 	int			__use;
-	unsigned long		lastuse;

include/net/dst.h struct dst_entry { · line 104

Quoted 10 lines
include/net/dst.h +1 -6
-	union {
-		struct dst_entry	*next;
-		struct rtable __rcu	*rt_next;
-		struct rt6_info		*rt6_next;
-		struct dn_route __rcu	*dn_next;
-	};
+	struct rcu_head		rcu_head;
 };
 
 #ifdef __KERNEL__

Nope...

"lastuse" and "next" must be in this place, or this introduce false sharing we wanted to avoid in the past.

I suggest you leave this code as is, we will address the problem when rcu_head changes (assuming we can test a CONFIG_RCU_HEAD_DEBUG or something)

First part of "struct dst_entry" is mostly read, while part beginning after refcnt is often written.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw
Quoted 78 lines · Eric Dumazet
Quoted 61 lines · Lai Jiangshan
Quoted 10 lines · David Miller

From: Lai Jiangshan <[email protected]>
Date: Tue, 01 Mar 2011 16:03:44 +0800

Quoted 7 lines

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Quoted 6 lines
Quoted 1 line
Signed-off-by Lai Jiangshan <[email protected]>

We don't want to use the align if it's going to waste lots of space.

Instead we want to rearrange the structure so that the alignment comes more cheaply.

Subject: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding are hardly suit for the changes. So we rearrange the structure, and move the seldom access rcu_head to the end of the structure.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/net/dst.h +7 -2
index 93b0310..d8c5296 100644
@@ -37,7 +37,6 @@
 struct sk_buff;
 
 struct dst_entry {
-	struct rcu_head		rcu_head;
 	struct dst_entry	*child;
 	struct net_device       *dev;
 	short			error;

@@ -78,6 +77,13 @@ struct dst_entry {
 	__u32			__pad2;
 #endif
 
+	unsigned long		lastuse;
+	union {
+		struct dst_entry	*next;
+		struct rtable __rcu	*rt_next;
+		struct rt6_info		*rt6_next;
+		struct dn_route __rcu	*dn_next;
+	};
 
 	/*
 	 * Align __refcnt to a 64 bytes alignment

@@ -92,13 +98,7 @@ struct dst_entry {
 	 */
 	atomic_t		__refcnt;	/* client references	*/
 	int			__use;
-	unsigned long		lastuse;

include/net/dst.h struct dst_entry { · line 104

Quoted 10 lines
include/net/dst.h +1 -6
-	union {
-		struct dst_entry	*next;
-		struct rtable __rcu	*rt_next;
-		struct rt6_info		*rt6_next;
-		struct dn_route __rcu	*dn_next;
-	};
+	struct rcu_head		rcu_head;
 };
 
 #ifdef __KERNEL__

Nope...

"lastuse" and "next" must be in this place, or this introduce false sharing we wanted to avoid in the past.

Quoted 7 lines

I suggest you leave this code as is, we will address the problem when rcu_head changes (assuming we can test a CONFIG_RCU_HEAD_DEBUG or something)

First part of "struct dst_entry" is mostly read, while part beginning after refcnt is often written.

Is it the cause of false sharing? I thought that all are rare write(except __refcnt) since it is protected by RCU.

Do you allow me just move the seldom access rcu_head to the end of the structure and add pads before __refcnt? I guess it increases about 3% the size of dst_entry.

I accept that I leave this code as is, when I change rcu_head I will notify you.

Thanks,
Lai

#6 lore raw
Quoted 5 lines · Lai Jiangshan

Is it the cause of false sharing? I thought that all are rare write(except __refcnt) since it is protected by RCU.

Do you allow me just move the seldom access rcu_head to the end of the structure and add pads before __refcnt? I guess it increases about 3% the size of dst_entry.

dst_entry is a base class.

Its included at the beginning of other structs.

Moving rcu_head "at the end" just move it right in the middle of upper objects as a matter of fact. This might add one cache line miss on critical network object. A complete audit is needed.

David is doing some changes in this area, so things move fast anyway.

Quoted 1 line

I accept that I leave this code as is, when I change rcu_head I will

notify you.

Quoted 1 line

Thanks

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

#7 lore raw
In reply to Lai Jiangshan
Quoted 39 lines · Lai Jiangshan

struct dst_entry assumes the size of struct rcu_head as 2 * sizeof(long) and manually adds pads for aligning for "__refcnt".

When the size of struct rcu_head is changed, these manual padding is wrong. Use __attribute__((aligned (64))) instead.

Signed-off-by Lai Jiangshan <[email protected]>

---

include/net/dst.h +0 -9
index 93b0310..4ef6c4a 100644
@@ -62,8 +62,6 @@ struct dst_entry {
 	struct hh_cache		*hh;
 #ifdef CONFIG_XFRM
 	struct xfrm_state	*xfrm;
-#else
-	void			*__pad1;
 #endif
 	int			(*input)(struct sk_buff*);
 	int			(*output)(struct sk_buff*);

@@ -74,23 +72,18 @@ struct dst_entry {
 
 #ifdef CONFIG_NET_CLS_ROUTE
 	__u32			tclassid;
-#else
-	__u32			__pad2;
 #endif
 
 
 	/*
 	 * Align __refcnt to a 64 bytes alignment
 	 * (L1_CACHE_SIZE would be too much)
-	 */
-#ifdef CONFIG_64BIT
-	long			__pad_to_align_refcnt[1];
-#endif
-	/*

include/net/dst.h struct dst_entry { · line 89

Quoted 10 lines
include/net/dst.h +3 -1
+	 *
 	 * __refcnt wants to be on a different cache line from
 	 * input/output/ops or performance tanks badly
 	 */
-	atomic_t		__refcnt;	/* client references	*/
+	atomic_t		__refcnt	/* client references	*/
+				__attribute__((aligned (64)));
 	int			__use;
 	unsigned long		lastuse;
 	union {

If struct rcu_head is bigger, this is for debugging purposes, so we dont care about performance, and can avoid wasting ~64 bytes.

Some machines still have about 2.000.000 active dst entries : the convoluted checks we added in include/net/dst.h are here to make sure we dont have huge holes in the dst structure.

(This might change when/if IP route cache is gone)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>