[PATCH] net: force dst_default_metrics to const section

[PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

While investigating on network performance problems, I found this little
gem :

$ nm -v vmlinux | grep -1 dst_default_metrics
ffffffff82736540 b busy.46605 
ffffffff82736560 B dst_default_metrics
ffffffff82736598 b dst_busy_list

Apparently, declaring a const array without initializer put it in
(writeable) bss section, in middle of possibly often dirtied cache
lines.

Since we really want dst_default_metrics be const to avoid any possible
false sharing and catch any buggy writes, I force a null initializer.

ffffffff818a4c20 R dst_default_metrics

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
For reference, dst_default_metrics was added in 2.6.39

 net/core/dst.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/core/dst.c b/net/core/dst.c
index 069d51d..4c538be 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -149,7 +149,15 @@ int dst_discard(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(dst_discard);
 
-const u32 dst_default_metrics[RTAX_MAX];
+const u32 dst_default_metrics[RTAX_MAX] = {
+	/* This initializer is needed to force linker to place this variable
+	 * into const section. Otherwise it might end into bss section.
+	 * We really want to avoid false sharing on this variable, and catch
+	 * any writes on it.
+	 */
+	0
+};
+
 
 void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
 		int initial_ref, int initial_obsolete, unsigned short flags)

Re: [PATCH] net: force dst_default_metrics to const section

[Ben Hutchings]

On Tue, 2012-08-07 at 18:11 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> While investigating on network performance problems, I found this little
> gem :
> 
> $ nm -v vmlinux | grep -1 dst_default_metrics
> ffffffff82736540 b busy.46605 
> ffffffff82736560 B dst_default_metrics
> ffffffff82736598 b dst_busy_list
> 
> Apparently, declaring a const array without initializer put it in
> (writeable) bss section, in middle of possibly often dirtied cache
> lines.
> 
> Since we really want dst_default_metrics be const to avoid any possible
> false sharing and catch any buggy writes, I force a null initializer.
> 
> ffffffff818a4c20 R dst_default_metrics
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> For reference, dst_default_metrics was added in 2.6.39
> 
>  net/core/dst.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/dst.c b/net/core/dst.c
> index 069d51d..4c538be 100644
> --- a/net/core/dst.c
> +++ b/net/core/dst.c
> @@ -149,7 +149,15 @@ int dst_discard(struct sk_buff *skb)
>  }
>  EXPORT_SYMBOL(dst_discard);
>  
> -const u32 dst_default_metrics[RTAX_MAX];
> +const u32 dst_default_metrics[RTAX_MAX] = {
> +	/* This initializer is needed to force linker to place this variable
> +	 * into const section. Otherwise it might end into bss section.
> +	 * We really want to avoid false sharing on this variable, and catch
> +	 * any writes on it.
> +	 */
> +	0
> +};
> +

Some day the compiler may be smart enough to ignore the different
between explicit and implicit zero-initialisation, and put it back in
BSS.  Declaring this __cache_aligned_in_smp might be a better option.

Ben.
 
>  void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
>  		int initial_ref, int initial_obsolete, unsigned short flags)


-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>


> Some day the compiler may be smart enough to ignore the different
> between explicit and implicit zero-initialisation, and put it back in
> BSS.  Declaring this __cache_aligned_in_smp might be a better option.

__cache_aligned_in_smp aligns start of the structure, but can be
followed by another var in same cache line. Yes, this is bad.

By the way we dont care of cache alignment on this structure, only it
should be const. Its a soft requirement, machine wont crash if it is not
the case.

If compiler is smart one day as you say (it should first be non buggy
IMHO), then we can add a non zero field like this :

Thanks

[PATCH v2] net: force dst_default_metrics to const section

While investigating on network performance problems, I found this little
gem :

$ nm -v vmlinux | grep -1 dst_default_metrics
ffffffff82736540 b busy.46605 
ffffffff82736560 B dst_default_metrics
ffffffff82736598 b dst_busy_list

Apparently, declaring a const array without initializer put it in
(writeable) bss section, in middle of possibly often dirtied cache
lines.

Since we really want dst_default_metrics be const to avoid any possible
false sharing and catch any buggy writes, I force a null initializer.

ffffffff818a4c20 R dst_default_metrics

Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Ben Hutchings <bhutchings@solarflare.com>
---
 include/net/dst.h |    2 +-
 net/core/dst.c    |   10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/net/dst.h b/include/net/dst.h
index baf5978..621e351 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -110,7 +110,7 @@ struct dst_entry {
 };
 
 extern u32 *dst_cow_metrics_generic(struct dst_entry *dst, unsigned long old);
-extern const u32 dst_default_metrics[RTAX_MAX];
+extern const u32 dst_default_metrics[];
 
 #define DST_METRICS_READ_ONLY	0x1UL
 #define __DST_METRICS_PTR(Y)	\
diff --git a/net/core/dst.c b/net/core/dst.c
index 069d51d..56d6361 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -149,7 +149,15 @@ int dst_discard(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(dst_discard);
 
-const u32 dst_default_metrics[RTAX_MAX];
+const u32 dst_default_metrics[RTAX_MAX + 1] = {
+	/* This initializer is needed to force linker to place this variable
+	 * into const section. Otherwise it might end into bss section.
+	 * We really want to avoid false sharing on this variable, and catch
+	 * any writes on it.
+	 */
+	[RTAX_MAX] = 0xdeadbeef,
+};
+
 
 void *dst_alloc(struct dst_ops *ops, struct net_device *dev,
 		int initial_ref, int initial_obsolete, unsigned short flags)

Re: [PATCH] net: force dst_default_metrics to const section

[David Miller]

From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 7 Aug 2012 21:15:27 +0100

> Some day the compiler may be smart enough to ignore the different
> between explicit and implicit zero-initialisation, and put it back in
> BSS.  Declaring this __cache_aligned_in_smp might be a better option.

I'm surprised it doesn't already do this.

It definitely puts scalar explicit zero initializers into the BSS.

Re: [PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

On Tue, 2012-08-07 at 14:55 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 7 Aug 2012 21:15:27 +0100
> 
> > Some day the compiler may be smart enough to ignore the different
> > between explicit and implicit zero-initialisation, and put it back in
> > BSS.  Declaring this __cache_aligned_in_smp might be a better option.
> 
> I'm surprised it doesn't already do this.
> 
> It definitely puts scalar explicit zero initializers into the BSS.

Not a const :


# cat try.c
const int scalar_value = 0;
const int scalar_value_bss;
int scalar_value = 0;
int scalar_value_bss;
main(int argc, char *argv[])
{
return 0;
}
# gcc -o try try.c && nm -v try|grep scalar_value
00000000004005cc R cscalar_value
0000000000601028 B scalar_value
000000000060102c B scalar_value_bss
0000000000601030 B cscalar_value_bss


gcc 4.6.3

Re: [PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

On Wed, 2012-08-08 at 00:10 +0200, Eric Dumazet wrote:

> # gcc -o try try.c && nm -v try|grep scalar_value
> 00000000004005cc R cscalar_value
> 0000000000601028 B scalar_value
> 000000000060102c B scalar_value_bss
> 0000000000601030 B cscalar_value_bss
> 

Sorry the source was :

const int cscalar_value = 0;
const int cscalar_value_bss;
int scalar_value = 0;
int scalar_value_bss;

main(int argc, char *argv[])
{
return 0;
}

Re: [PATCH] net: force dst_default_metrics to const section

[Ben Hutchings]

On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> 
> > Some day the compiler may be smart enough to ignore the different
> > between explicit and implicit zero-initialisation, and put it back in
> > BSS.  Declaring this __cache_aligned_in_smp might be a better option.
> 
> __cache_aligned_in_smp aligns start of the structure, but can be
> followed by another var in same cache line. Yes, this is bad.

Oh, that's unexpected.

> By the way we dont care of cache alignment on this structure, only it
> should be const. Its a soft requirement, machine wont crash if it is not
> the case.

Right.

> If compiler is smart one day as you say (it should first be non buggy
> IMHO), then we can add a non zero field like this :
[...]

That would work, but it's ugly!  How about defining and using a
meaningfully-named macro that expands to __section(.rodata)?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

On Tue, 2012-08-07 at 23:12 +0100, Ben Hutchings wrote:
> On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > 
> > > Some day the compiler may be smart enough to ignore the different
> > > between explicit and implicit zero-initialisation, and put it back in
> > > BSS.  Declaring this __cache_aligned_in_smp might be a better option.
> > 
> > __cache_aligned_in_smp aligns start of the structure, but can be
> > followed by another var in same cache line. Yes, this is bad.
> 
> Oh, that's unexpected.
> 
> > By the way we dont care of cache alignment on this structure, only it
> > should be const. Its a soft requirement, machine wont crash if it is not
> > the case.
> 
> Right.
> 
> > If compiler is smart one day as you say (it should first be non buggy
> > IMHO), then we can add a non zero field like this :
> [...]
> 
> That would work, but it's ugly!  How about defining and using a
> meaningfully-named macro that expands to __section(.rodata)?

You are kidding. I prefer plain C and not having to mess with all
arches.

Re: [PATCH] net: force dst_default_metrics to const section

[Ben Hutchings]

On Wed, 2012-08-08 at 00:34 +0200, Eric Dumazet wrote:
> On Tue, 2012-08-07 at 23:12 +0100, Ben Hutchings wrote:
> > On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > > 
> > > 
> > > > Some day the compiler may be smart enough to ignore the different
> > > > between explicit and implicit zero-initialisation, and put it back in
> > > > BSS.  Declaring this __cache_aligned_in_smp might be a better option.
> > > 
> > > __cache_aligned_in_smp aligns start of the structure, but can be
> > > followed by another var in same cache line. Yes, this is bad.
> > 
> > Oh, that's unexpected.
> > 
> > > By the way we dont care of cache alignment on this structure, only it
> > > should be const. Its a soft requirement, machine wont crash if it is not
> > > the case.
> > 
> > Right.
> > 
> > > If compiler is smart one day as you say (it should first be non buggy
> > > IMHO), then we can add a non zero field like this :
> > [...]
> > 
> > That would work, but it's ugly!  How about defining and using a
> > meaningfully-named macro that expands to __section(.rodata)?
> 
> You are kidding. I prefer plain C and not having to mess with all
> arches.

Any consideration of implementation details like BSS and cache line
sharing is already outside of 'plain C'.  And you don't have to 'mess
with all arches'; just look at what <linux/init.h> and <linux/module.h>
do.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH] net: force dst_default_metrics to const section

[Eric Dumazet]

On Tue, 2012-08-07 at 23:44 +0100, Ben Hutchings wrote:

> Any consideration of implementation details like BSS and cache line
> sharing is already outside of 'plain C'.  And you don't have to 'mess
> with all arches'; just look at what <linux/init.h> and <linux/module.h>
> do.

"const" is the clean and portable way to express my needs. No magic
section.

All is self-contained in the definition of the metrics, with a nice
comment.

All const are naturally shared by all cpus, without adding extra cache
line boundaries.

Please feel free to send another version, but I wont spend more time
myself.

Thanks

Re: [PATCH] net: force dst_default_metrics to const section

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 08 Aug 2012 00:56:40 +0200

> On Tue, 2012-08-07 at 23:44 +0100, Ben Hutchings wrote:
> 
>> Any consideration of implementation details like BSS and cache line
>> sharing is already outside of 'plain C'.  And you don't have to 'mess
>> with all arches'; just look at what <linux/init.h> and <linux/module.h>
>> do.
> 
> "const" is the clean and portable way to express my needs. No magic
> section.
> 
> All is self-contained in the definition of the metrics, with a nice
> comment.
> 
> All const are naturally shared by all cpus, without adding extra cache
> line boundaries.

I agree with Eric and will apply his patch.

Re: [PATCH] net: force dst_default_metrics to const section

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Aug 2012 22:55:45 +0200

> [PATCH v2] net: force dst_default_metrics to const section
> 
> While investigating on network performance problems, I found this little
> gem :
> 
> $ nm -v vmlinux | grep -1 dst_default_metrics
> ffffffff82736540 b busy.46605 
> ffffffff82736560 B dst_default_metrics
> ffffffff82736598 b dst_busy_list
> 
> Apparently, declaring a const array without initializer put it in
> (writeable) bss section, in middle of possibly often dirtied cache
> lines.
> 
> Since we really want dst_default_metrics be const to avoid any possible
> false sharing and catch any buggy writes, I force a null initializer.
> 
> ffffffff818a4c20 R dst_default_metrics
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>

Applied, thanks.