* [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head @ 2011-03-01 8:03 Lai Jiangshan 2011-03-01 8:16 ` David Miller 2011-03-01 8:20 ` [PATCH 4/4] " Eric Dumazet 0 siblings, 2 replies; 8+ messages in thread From: Lai Jiangshan @ 2011-03-01 8:03 UTC (permalink / raw) To: Ingo Molnar Cc: Paul E. McKenney, Christoph Lameter, Pekka Enberg, Eric Dumazet, David S. Miller, Matt Mackall, linux-mm, linux-kernel, netdev 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> ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head 2011-03-01 8:03 [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head Lai Jiangshan @ 2011-03-01 8:16 ` David Miller 2011-03-01 8:20 ` Eric Dumazet 2011-03-01 8:53 ` [PATCH 4/4 V2] " Lai Jiangshan 2011-03-01 8:20 ` [PATCH 4/4] " Eric Dumazet 1 sibling, 2 replies; 8+ messages in thread From: David Miller @ 2011-03-01 8:16 UTC (permalink / raw) To: laijs Cc: mingo, paulmck, cl, penberg, eric.dumazet, mpm, linux-mm, linux-kernel, netdev 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head 2011-03-01 8:16 ` David Miller @ 2011-03-01 8:20 ` Eric Dumazet 2011-03-01 8:53 ` [PATCH 4/4 V2] " Lai Jiangshan 1 sibling, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2011-03-01 8:20 UTC (permalink / raw) To: David Miller Cc: laijs, mingo, paulmck, cl, penberg, mpm, linux-mm, linux-kernel, netdev Le mardi 01 mars 2011 A 00:16 -0800, David Miller a A(C)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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head 2011-03-01 8:16 ` David Miller 2011-03-01 8:20 ` Eric Dumazet @ 2011-03-01 8:53 ` Lai Jiangshan 2011-03-01 9:20 ` Eric Dumazet 1 sibling, 1 reply; 8+ messages in thread From: Lai Jiangshan @ 2011-03-01 8:53 UTC (permalink / raw) To: David Miller Cc: mingo, paulmck, cl, penberg, eric.dumazet, mpm, linux-mm, linux-kernel, netdev 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> ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head 2011-03-01 8:53 ` [PATCH 4/4 V2] " Lai Jiangshan @ 2011-03-01 9:20 ` Eric Dumazet 2011-03-02 2:46 ` Lai Jiangshan 0 siblings, 1 reply; 8+ messages in thread From: Eric Dumazet @ 2011-03-01 9:20 UTC (permalink / raw) To: Lai Jiangshan Cc: David Miller, mingo, paulmck, cl, penberg, mpm, linux-mm, linux-kernel, netdev Le mardi 01 mars 2011 A 16:53 +0800, Lai Jiangshan a A(C)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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head 2011-03-01 9:20 ` Eric Dumazet @ 2011-03-02 2:46 ` Lai Jiangshan 2011-03-02 3:02 ` Eric Dumazet 0 siblings, 1 reply; 8+ messages in thread From: Lai Jiangshan @ 2011-03-02 2:46 UTC (permalink / raw) To: Eric Dumazet Cc: David Miller, mingo, paulmck, cl, penberg, mpm, linux-mm, linux-kernel, netdev 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 -- 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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4 V2] net,rcu: don't assume the size of struct rcu_head 2011-03-02 2:46 ` Lai Jiangshan @ 2011-03-02 3:02 ` Eric Dumazet 0 siblings, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2011-03-02 3:02 UTC (permalink / raw) To: Lai Jiangshan Cc: David Miller, mingo, paulmck, cl, penberg, mpm, linux-mm, linux-kernel, netdev Le mercredi 02 mars 2011 A 10:46 +0800, Lai Jiangshan a A(C)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> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head 2011-03-01 8:03 [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head Lai Jiangshan 2011-03-01 8:16 ` David Miller @ 2011-03-01 8:20 ` Eric Dumazet 1 sibling, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2011-03-01 8:20 UTC (permalink / raw) To: Lai Jiangshan Cc: Ingo Molnar, Paul E. McKenney, Christoph Lameter, Pekka Enberg, David S. Miller, Matt Mackall, linux-mm, linux-kernel, netdev Le mardi 01 mars 2011 A 16:03 +0800, Lai Jiangshan a A(C)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> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-03-02 3:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-03-01 8:03 [PATCH 4/4] net,rcu: don't assume the size of struct rcu_head Lai Jiangshan 2011-03-01 8:16 ` David Miller 2011-03-01 8:20 ` Eric Dumazet 2011-03-01 8:53 ` [PATCH 4/4 V2] " Lai Jiangshan 2011-03-01 9:20 ` Eric Dumazet 2011-03-02 2:46 ` Lai Jiangshan 2011-03-02 3:02 ` Eric Dumazet 2011-03-01 8:20 ` [PATCH 4/4] " Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).