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

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

[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 <laijs@cn.fujitsu.com>
---
diff --git a/include/net/dst.h b/include/net/dst.h
index 93b0310..4ef6c4a 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[David Miller]

From: Lai Jiangshan <laijs@cn.fujitsu.com>
Date: Tue, 01 Mar 2011 16:03:44 +0800

> 
> 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 <laijs@cn.fujitsu.com>

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Eric Dumazet]

Le mardi 01 mars 2011 à 00:16 -0800, David Miller a écrit :
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
> Date: Tue, 01 Mar 2011 16:03:44 +0800
> 
> > 
> > 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 <laijs@cn.fujitsu.com>
> 
> 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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Lai Jiangshan]

On 03/01/2011 04:16 PM, David Miller wrote:
> From: Lai Jiangshan <laijs@cn.fujitsu.com>
> Date: Tue, 01 Mar 2011 16:03:44 +0800
> 
>>
>> 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 <laijs@cn.fujitsu.com>
> 
> 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 <laijs@cn.fujitsu.com>
---

diff --git a/include/net/dst.h b/include/net/dst.h
index 93b0310..d8c5296 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -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__

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Eric Dumazet]

Le mardi 01 mars 2011 à 16:53 +0800, Lai Jiangshan a écrit :
> On 03/01/2011 04:16 PM, David Miller wrote:
> > From: Lai Jiangshan <laijs@cn.fujitsu.com>
> > Date: Tue, 01 Mar 2011 16:03:44 +0800
> > 
> >>
> >> 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 <laijs@cn.fujitsu.com>
> > 
> > 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 <laijs@cn.fujitsu.com>
> ---
> 
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 93b0310..d8c5296 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -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__

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Lai Jiangshan]

On 03/01/2011 05:20 PM, Eric Dumazet wrote:
> Le mardi 01 mars 2011 à 16:53 +0800, Lai Jiangshan a écrit :
>> On 03/01/2011 04:16 PM, David Miller wrote:
>>> From: Lai Jiangshan <laijs@cn.fujitsu.com>
>>> Date: Tue, 01 Mar 2011 16:03:44 +0800
>>>
>>>>
>>>> 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 <laijs@cn.fujitsu.com>
>>>
>>> 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 <laijs@cn.fujitsu.com>
>> ---
>>
>> diff --git a/include/net/dst.h b/include/net/dst.h
>> index 93b0310..d8c5296 100644
>> --- a/include/net/dst.h
>> +++ b/include/net/dst.h
>> @@ -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__
> 
> 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.
> 

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

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

[Eric Dumazet]

Le mercredi 02 mars 2011 à 10:46 +0800, Lai Jiangshan a écrit :

> 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.

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

Thanks


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>

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

[Eric Dumazet]

Le mardi 01 mars 2011 à 16:03 +0800, Lai Jiangshan a écrit :
> 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 <laijs@cn.fujitsu.com>
> ---

> diff --git a/include/net/dst.h b/include/net/dst.h
> index 93b0310..4ef6c4a 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -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 {

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 majordomo@kvack.org.  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:"dont@kvack.org"> email@kvack.org </a>