[PATCH] Move some variables into the "most_readonly" section??

[PATCH] Move some variables into the "most_readonly" section??

[christoph]

These variables cause false sharing in some circumstances. However, they 
are just small pointers and 4 byte ints. So this patch would result in 
some wastage of memory since each of those pointers then would occupy a 
whole cache line.

Do we have any provisions for this situation? Or do we need a new section 
for mostly_readonly?
 
Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
Signed-off-by: Shai Fultheim <shai@scalex86.org>
Signed-off-by: Christoph Lameter <christoph@scalex86.org>

Index: linux-2.6.12-rc6-mm1/arch/i386/kernel/time.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/arch/i386/kernel/time.c	2005-06-07 11:15:43.000000000 -0700
+++ linux-2.6.12-rc6-mm1/arch/i386/kernel/time.c	2005-06-07 11:27:57.000000000 -0700
@@ -88,7 +88,7 @@ EXPORT_SYMBOL(rtc_lock);
 DEFINE_SPINLOCK(i8253_lock);
 EXPORT_SYMBOL(i8253_lock);
 
-struct timer_opts *cur_timer = &timer_none;
+struct timer_opts *cur_timer __cacheline_aligned_mostly_readonly = &timer_none;
 
 /*
  * This is a special lock that is owned by the CPU and holds the index
Index: linux-2.6.12-rc6-mm1/arch/i386/kernel/timers/timer_hpet.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/arch/i386/kernel/timers/timer_hpet.c	2005-06-07 11:26:36.000000000 -0700
+++ linux-2.6.12-rc6-mm1/arch/i386/kernel/timers/timer_hpet.c	2005-06-07 11:27:57.000000000 -0700
@@ -18,7 +18,9 @@
 #include "mach_timer.h"
 #include <asm/hpet.h>
 
-static unsigned long hpet_usec_quotient;	/* convert hpet clks to usec */
+static unsigned long hpet_usec_quotient __cacheline_aligned_mostly_readonly;
+			/* convert hpet clks to usec */
+
 static unsigned long tsc_hpet_quotient;		/* convert tsc to hpet clks */
 static unsigned long hpet_last; 	/* hpet counter value at last tick*/
 static unsigned long last_tsc_low;	/* lsb 32 bits of Time Stamp Counter */
Index: linux-2.6.12-rc6-mm1/drivers/char/random.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/drivers/char/random.c	2005-06-07 11:13:41.000000000 -0700
+++ linux-2.6.12-rc6-mm1/drivers/char/random.c	2005-06-07 11:27:57.000000000 -0700
@@ -271,7 +271,8 @@ static int random_write_wakeup_thresh = 
  * samples to avoid wasting CPU time and reduce lock contention.
  */
 
-static int trickle_thresh = INPUT_POOL_WORDS * 28;
+static int trickle_thresh __cacheline_aligned_mostly_readonly =
+			INPUT_POOL_WORDS * 28;
 
 static DEFINE_PER_CPU(int, trickle_count) = 0;
 

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andi Kleen]

On Tue, Jun 07, 2005 at 12:58:38PM -0700, christoph wrote:
> These variables cause false sharing in some circumstances. However, they 
> are just small pointers and 4 byte ints. So this patch would result in 
> some wastage of memory since each of those pointers then would occupy a 
> whole cache line.
> 
> Do we have any provisions for this situation? Or do we need a new section 
> for mostly_readonly?


You lost me  - when the variable is in the mostly readonly section then
there should not be any false sharing because all the data on these
cache lines should be always in SHARED state. And there is no wasted
memory because the variables can be packed tightly there.

However this means __cacheline_aligned_mostly_readonly doesnt make much
sense since there is no need for alignment in read only. How about
replacing it with a __mostly_readonly that doesnt align and remove
__cacheline_aligned_mostly_readonly? 

-Andi

Re: [PATCH] Move some variables into the "most_readonly" section??

[christoph]

On Wed, 8 Jun 2005, Andi Kleen wrote:

> However this means __cacheline_aligned_mostly_readonly doesnt make much
> sense since there is no need for alignment in read only. How about
> replacing it with a __mostly_readonly that doesnt align and remove
> __cacheline_aligned_mostly_readonly? 

Hmm. No. The bigger cpu maps may benefit from cacheline alignment for 
even for read access. Here is a patch that introduces __mostly_readonly in 
addition to __cacheline_aligned_mostly_readonly:

---

Create a new __mostly_readonly attribute.

Turns out that some smaller entities would also benefit from placement
with other variables that are mostly readonly. The following patch
introduces the __mostly_readonly attribute. This is simply the
__cacheline_aligned_most_readonly attribute without the cacheline
alignment.

The patch places cur_timer, hpet_usec_quotient and trickle_thresh
into that section.
 
Signed-off-by: Alok N Kataria <alokk@calsoftinc.com>
Signed-off-by: Shai Fultheim <shai@scalex86.org>
Signed-off-by: Christoph Lameter <christoph@scalex86.org>

Index: linux-2.6.12-rc6-mm1/arch/i386/kernel/time.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/arch/i386/kernel/time.c	2005-06-14 15:43:18.000000000 -0700
+++ linux-2.6.12-rc6-mm1/arch/i386/kernel/time.c	2005-06-14 15:43:19.000000000 -0700
@@ -88,7 +88,7 @@ EXPORT_SYMBOL(rtc_lock);
 DEFINE_SPINLOCK(i8253_lock);
 EXPORT_SYMBOL(i8253_lock);
 
-struct timer_opts *cur_timer = &timer_none;
+struct timer_opts *cur_timer __mostly_readonly = &timer_none;
 
 /*
  * This is a special lock that is owned by the CPU and holds the index
Index: linux-2.6.12-rc6-mm1/arch/i386/kernel/timers/timer_hpet.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/arch/i386/kernel/timers/timer_hpet.c	2005-06-14 15:43:18.000000000 -0700
+++ linux-2.6.12-rc6-mm1/arch/i386/kernel/timers/timer_hpet.c	2005-06-14 15:43:19.000000000 -0700
@@ -18,7 +18,9 @@
 #include "mach_timer.h"
 #include <asm/hpet.h>
 
-static unsigned long hpet_usec_quotient;	/* convert hpet clks to usec */
+static unsigned long hpet_usec_quotient __mostly_readonly;
+			/* convert hpet clks to usec */
+
 static unsigned long tsc_hpet_quotient;		/* convert tsc to hpet clks */
 static unsigned long hpet_last; 	/* hpet counter value at last tick*/
 static unsigned long last_tsc_low;	/* lsb 32 bits of Time Stamp Counter */
Index: linux-2.6.12-rc6-mm1/drivers/char/random.c
===================================================================
--- linux-2.6.12-rc6-mm1.orig/drivers/char/random.c	2005-06-14 15:43:18.000000000 -0700
+++ linux-2.6.12-rc6-mm1/drivers/char/random.c	2005-06-14 15:43:19.000000000 -0700
@@ -271,7 +271,8 @@ static int random_write_wakeup_thresh = 
  * samples to avoid wasting CPU time and reduce lock contention.
  */
 
-static int trickle_thresh = INPUT_POOL_WORDS * 28;
+static int trickle_thresh __mostly_readonly =
+			INPUT_POOL_WORDS * 28;
 
 static DEFINE_PER_CPU(int, trickle_count) = 0;
 
Index: linux-2.6.12-rc6-mm1/include/linux/cache.h
===================================================================
--- linux-2.6.12-rc6-mm1.orig/include/linux/cache.h	2005-06-14 15:43:18.000000000 -0700
+++ linux-2.6.12-rc6-mm1/include/linux/cache.h	2005-06-14 15:46:58.000000000 -0700
@@ -26,9 +26,13 @@
 #endif
 
 #if defined(CONFIG_X86)
+
+#define __mostly_readonly __attribute__((__section__(".data.mostly_readonly")))
+
 #define __cacheline_aligned_mostly_readonly		\
   __attribute__((__aligned__(SMP_CACHE_BYTES),		\
 		 __section__(".data.mostly_readonly")))
+
 #else
 #define __cacheline_aligned_mostly_readonly __cacheline_aligned
 #endif

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andi Kleen]

On Tue, Jun 14, 2005 at 03:54:24PM -0700, christoph wrote:
> On Wed, 8 Jun 2005, Andi Kleen wrote:
> 
> > However this means __cacheline_aligned_mostly_readonly doesnt make much
> > sense since there is no need for alignment in read only. How about
> > replacing it with a __mostly_readonly that doesnt align and remove
> > __cacheline_aligned_mostly_readonly? 
> 
> Hmm. No. The bigger cpu maps may benefit from cacheline alignment for 
> even for read access. 

Why? Can you please explain that. It doesn't make sense to me.

Any read only cache line should be freely shareable over all caches.

Thanks.

-Andi

Re: [PATCH] Move some variables into the "most_readonly" section??

[Christoph Lameter]

On Wed, 15 Jun 2005, Andi Kleen wrote:

> > Hmm. No. The bigger cpu maps may benefit from cacheline alignment for 
> > even for read access. 
> 
> Why? Can you please explain that. It doesn't make sense to me.

Its more likely to get a big piece of the array in a single 
cacheline if the array starts at the beginning of a cacheline.

If these maps would start in the middle of a cacheline then additional 
cacheline fetches may become necessary to scan an array etc.

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andi Kleen]

On Tue, Jun 14, 2005 at 04:17:14PM -0700, Christoph Lameter wrote:
> On Wed, 15 Jun 2005, Andi Kleen wrote:
> 
> > > Hmm. No. The bigger cpu maps may benefit from cacheline alignment for 
> > > even for read access. 
> > 
> > Why? Can you please explain that. It doesn't make sense to me.
> 
> Its more likely to get a big piece of the array in a single 
> cacheline if the array starts at the beginning of a cacheline.
> 
> If these maps would start in the middle of a cacheline then additional 
> cacheline fetches may become necessary to scan an array etc.

But the CPUs do prefetching anyways for that. Do you have numbers
that this is actually worth it? 

-Andi

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andrew Morton]

christoph <christoph@scalex86.org> wrote:
>
> On Wed, 8 Jun 2005, Andi Kleen wrote:
> 
> > However this means __cacheline_aligned_mostly_readonly doesnt make much
> > sense since there is no need for alignment in read only. How about
> > replacing it with a __mostly_readonly that doesnt align and remove
> > __cacheline_aligned_mostly_readonly? 
> 
> Hmm. No.

Think so.  If an object is in its own cacheline then it won't be pingponged
around by writes to unrelated nearby objects.

> The bigger cpu maps may benefit from cacheline alignment for 
> even for read access.

A tiny bit, because the bitmaps might straddle one more cacheline than they
strictly need to.

> Here is a patch that introduces __mostly_readonly in 
> addition to __cacheline_aligned_mostly_readonly:

I think readmostliness and alignment are mostly-unrelated concepts and
should have separate tag thingies.  IOW,
__cacheline_aligned_mostly_readonly goes away and to handle things like the
cpu maps we do:

char foo[8] __cacheline_aligned _mostly_readonly = { whatever };


(I shall now go away and quietly tear my hair out.  Those mostly-readonly
patches caused a mountain of grief:

optimise-storage-of-read-mostly-variables.patch
optimise-storage-of-read-mostly-variables-fix.patch
optimise-storage-of-read-mostly-variables-x86_64-fix.patch
optimise-storage-of-read-mostly-variables-x86_64-fix-fix.patch
optimise-storage-of-read-mostly-variables-x86_64-fix-fix-fix.patch
move-some-more-structures-into-mostly_readonly-and-readonly.patch
kexec-x86_64-optimise-storage-of-read-mostly-variables-x86_64-fix.patch

Once this is sorted I'll drop the lot and we start from a clean slate).

Re: [PATCH] Move some variables into the "most_readonly" section??

[Christoph Lameter]

On Wed, 15 Jun 2005, Andi Kleen wrote:

> > If these maps would start in the middle of a cacheline then additional 
> > cacheline fetches may become necessary to scan an array etc.
> 
> But the CPUs do prefetching anyways for that. Do you have numbers
> that this is actually worth it? 

Its not only for scanning an array. A struct may contains a lot of 
related information like for example boot_cpu_data. Aligning 
increases locality and the likelyhood that no additional cachelines have 
to be fetched.

Re: [PATCH] Move some variables into the "most_readonly" section??

[christoph]

On Tue, 14 Jun 2005, Andrew Morton wrote:

> I think readmostliness and alignment are mostly-unrelated concepts and
> should have separate tag thingies.  IOW,
> __cacheline_aligned_mostly_readonly goes away and to handle things like the
> cpu maps we do:

Yup that makes the whole thing much more sane. Can we specify multiple 
attributes to a variable?

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andi Kleen]

On Tue, Jun 14, 2005 at 04:27:55PM -0700, Christoph Lameter wrote:
> On Wed, 15 Jun 2005, Andi Kleen wrote:
> 
> > > If these maps would start in the middle of a cacheline then additional 
> > > cacheline fetches may become necessary to scan an array etc.
> > 
> > But the CPUs do prefetching anyways for that. Do you have numbers
> > that this is actually worth it? 
> 
> Its not only for scanning an array. A struct may contains a lot of 
> related information like for example boot_cpu_data. Aligning 
> increases locality and the likelyhood that no additional cachelines have 
> to be fetched.

On the other hand it will increase the overall working set so there might
be more cache misses again. It's a trade off and the outcome is not clear.

Do you have numbers? 

When in doubt I would suggest the less wasteful in memory solution.

-Andi

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andrew Morton]

christoph <christoph@scalex86.org> wrote:
>
> On Tue, 14 Jun 2005, Andrew Morton wrote:
> 
> > I think readmostliness and alignment are mostly-unrelated concepts and
> > should have separate tag thingies.  IOW,
> > __cacheline_aligned_mostly_readonly goes away and to handle things like the
> > cpu maps we do:
> 
> Yup that makes the whole thing much more sane. Can we specify multiple 
> attributes to a variable?

I suppose so.

Compiling this:

int x   __attribute__((__aligned__(32)))
        __attribute__((__section__(".data.mostly_readonly")));


generates this:

	.file	"t.c"
	.version	"01.01"
gcc2_compiled.:
.globl x
	.section	.data.mostly_readonly,"aw",@progbits
	.align 32
	.type	 x,@object
	.size	 x,4
x:
	.zero	4
	.ident	"GCC: (GNU) 2.96 20000731 (Red Hat Linux 7.3 2.96-113)"

Seems OK?

Re: [PATCH] Move some variables into the "most_readonly" section??

[christoph]

On Tue, 14 Jun 2005, Andrew Morton wrote:

> > Yup that makes the whole thing much more sane. Can we specify multiple 
> > attributes to a variable?
> Seems OK?

Looks fine. Want a patch against the existing fixes in mm1? So that we 
have a whatever-fix-fix-fix-fix-fix-fix-fix?

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andrew Morton]

christoph <christoph@scalex86.org> wrote:
>
> On Tue, 14 Jun 2005, Andrew Morton wrote:
> 
> > > Yup that makes the whole thing much more sane. Can we specify multiple 
> > > attributes to a variable?
> > Seems OK?
> 
> Looks fine. Want a patch against the existing fixes in mm1? So that we 
> have a whatever-fix-fix-fix-fix-fix-fix-fix?

Let's see what it looks like.  Two patches please.  One againt the core
(optimise-storage-of-read-mostly-variables*.patch) and one against the
users (move-some-more-structures-into-mostly_readonly-and-readonly.patch).

Re: [PATCH] Move some variables into the "most_readonly" section??

[Nick Piggin]

Andrew Morton wrote:
> christoph <christoph@scalex86.org> wrote:
> 
>>On Tue, 14 Jun 2005, Andrew Morton wrote:
>>
>>
>>>I think readmostliness and alignment are mostly-unrelated concepts and
>>>should have separate tag thingies.  IOW,
>>>__cacheline_aligned_mostly_readonly goes away and to handle things like the
>>>cpu maps we do:
>>
>>Yup that makes the whole thing much more sane. Can we specify multiple 
>>attributes to a variable?
> 
> 
> I suppose so.
> 
> Compiling this:
> 
> int x   __attribute__((__aligned__(32)))
>         __attribute__((__section__(".data.mostly_readonly")));
> 

Can I just throw in something unrelated and not very constructive
and ask that we call it 'read_mostly' instead of mostly_readonly?

mostly_readonly kind of says to me that most items in the section
are read only, read_mostly says all the items are mostly only read.

-- 
SUSE Labs, Novell Inc.

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] Move some variables into the "most_readonly" section??

[Andi Kleen]

On Tue, Jun 14, 2005 at 05:16:02PM -0700, Andrew Morton wrote:
> christoph <christoph@scalex86.org> wrote:
> >
> > On Tue, 14 Jun 2005, Andrew Morton wrote:
> > 
> > > > Yup that makes the whole thing much more sane. Can we specify multiple 
> > > > attributes to a variable?
> > > Seems OK?
> > 
> > Looks fine. Want a patch against the existing fixes in mm1? So that we 
> > have a whatever-fix-fix-fix-fix-fix-fix-fix?
> 
> Let's see what it looks like.  Two patches please.  One againt the core
> (optimise-storage-of-read-mostly-variables*.patch) and one against the
> users (move-some-more-structures-into-mostly_readonly-and-readonly.patch).

I think it would be better to first still see numbers for
this questionable optimizations.

-Andi

Re: [PATCH] Move some variables into the "most_readonly" section??

[Anton Blanchard]

> I think it would be better to first still see numbers for
> this questionable optimizations.

Agreed, it would be nice to see some benchmark numbers.

Anton

Re: [PATCH] Move some variables into the "most_readonly" section??

[christoph]

On Fri, 17 Jun 2005, Anton Blanchard wrote:

> > I think it would be better to first still see numbers for
> > this questionable optimizations.
> 
> Agreed, it would be nice to see some benchmark numbers.

Sorry seem to have lost track of this thread of thought.

These are optimizations that try to avoid false aliasing as
a result of the linker placing hot spots into the same cacheline. These
are not predictable per se. Some future patch may rearrange variables that 
then produce another case of false aliasing.

In order to demonstrate the effect, I would need to be allowed to put 
some hot variables into the same cacheline. And that seem to be pointless 
because we all know the outcome.