[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Will Deacon]

The linker script assumes a cacheline size of 32 bytes when aligning
the .data..cacheline_aligned and .data..percpu sections.

This patch updates the script to use L1_CACHE_BYTES, which should be set
to 64 on platforms that require it.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---

v2: incorporated suggestions from Stephen Boyd

 arch/arm/kernel/vmlinux.lds.S |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/vmlinux.lds.S b/arch/arm/kernel/vmlinux.lds.S
index f76e755..1077e4f 100644
--- a/arch/arm/kernel/vmlinux.lds.S
+++ b/arch/arm/kernel/vmlinux.lds.S
@@ -4,6 +4,7 @@
  */
 
 #include <asm-generic/vmlinux.lds.h>
+#include <asm/cache.h>
 #include <asm/thread_info.h>
 #include <asm/memory.h>
 #include <asm/page.h>
@@ -181,7 +182,7 @@ SECTIONS
 	}
 #endif
 
-	PERCPU_SECTION(32)
+	PERCPU_SECTION(L1_CACHE_BYTES)
 
 #ifdef CONFIG_XIP_KERNEL
 	__data_loc = ALIGN(4);		/* location in binary */
@@ -212,8 +213,8 @@ SECTIONS
 #endif
 
 		NOSAVE_DATA
-		CACHELINE_ALIGNED_DATA(32)
-		READ_MOSTLY_DATA(32)
+		CACHELINE_ALIGNED_DATA(L1_CACHE_BYTES)
+		READ_MOSTLY_DATA(L1_CACHE_BYTES)
 
 		/*
 		 * The exception fixup table (might need resorting at runtime)
-- 
1.7.4.1

[PATCH 2/2] ARM: cache: assume 64-byte L1 cachelines for ARMv7 CPUs

[Will Deacon]

To ensure correct alignment of cacheline-aligned data, the maximum
cacheline size needs to be known at compile time.

Since Cortex-A8 and Cortex-A15 have 64-byte cachelines (and it is likely
that there will be future ARMv7 implementations with the same line size)
then it makes sense to assume that CPU_V7 implies a 64-byte L1 cacheline
size. For CPUs with smaller caches, this will result in some harmless
padding but will help with single zImage work and avoid hitting subtle
bugs with misaligned data structures.

Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/arm/mm/Kconfig |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
index 4cefb57..493e5ea5 100644
--- a/arch/arm/mm/Kconfig
+++ b/arch/arm/mm/Kconfig
@@ -887,7 +887,7 @@ config ARM_L1_CACHE_SHIFT_6
 
 config ARM_L1_CACHE_SHIFT
 	int
-	default 6 if ARM_L1_CACHE_SHIFT_6
+	default 6 if ARM_L1_CACHE_SHIFT_6 || CPU_V7
 	default 5
 
 config ARM_DMA_MEM_BUFFERABLE
-- 
1.7.4.1

[PATCH 2/2] ARM: cache: assume 64-byte L1 cachelines for ARMv7 CPUs

[Shilimkar, Santosh]

Will,
On Mon, Jan 16, 2012 at 4:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> To ensure correct alignment of cacheline-aligned data, the maximum
> cacheline size needs to be known at compile time.
>
> Since Cortex-A8 and Cortex-A15 have 64-byte cachelines (and it is likely
> that there will be future ARMv7 implementations with the same line size)
> then it makes sense to assume that CPU_V7 implies a 64-byte L1 cacheline
> size. For CPUs with smaller caches, this will result in some harmless
> padding but will help with single zImage work and avoid hitting subtle
> bugs with misaligned data structures.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
> ?arch/arm/mm/Kconfig | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> index 4cefb57..493e5ea5 100644
> --- a/arch/arm/mm/Kconfig
> +++ b/arch/arm/mm/Kconfig
> @@ -887,7 +887,7 @@ config ARM_L1_CACHE_SHIFT_6
>
> ?config ARM_L1_CACHE_SHIFT
> ? ? ? ?int
> - ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6
> + ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6 || CPU_V7

Will be really harmless on A9 ? We have L2 also to be
considered here which hard codes the line size as 32.

[PATCH 2/2] ARM: cache: assume 64-byte L1 cachelines for ARMv7 CPUs

[Russell King - ARM Linux]

On Mon, Jan 16, 2012 at 04:52:14PM +0100, Shilimkar, Santosh wrote:
> Will,
> On Mon, Jan 16, 2012 at 4:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> > To ensure correct alignment of cacheline-aligned data, the maximum
> > cacheline size needs to be known at compile time.
> >
> > Since Cortex-A8 and Cortex-A15 have 64-byte cachelines (and it is likely
> > that there will be future ARMv7 implementations with the same line size)
> > then it makes sense to assume that CPU_V7 implies a 64-byte L1 cacheline
> > size. For CPUs with smaller caches, this will result in some harmless
> > padding but will help with single zImage work and avoid hitting subtle
> > bugs with misaligned data structures.
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> > ?arch/arm/mm/Kconfig | ? ?2 +-
> > ?1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> > index 4cefb57..493e5ea5 100644
> > --- a/arch/arm/mm/Kconfig
> > +++ b/arch/arm/mm/Kconfig
> > @@ -887,7 +887,7 @@ config ARM_L1_CACHE_SHIFT_6
> >
> > ?config ARM_L1_CACHE_SHIFT
> > ? ? ? ?int
> > - ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6
> > + ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6 || CPU_V7
> 
> Will be really harmless on A9 ? We have L2 also to be
> considered here which hard codes the line size as 32.

Going for a larger cache line size is safe: this is used for aligning
data structures and similar, and aligning to 64-byte means that it's
also 32-byte aligned.

[PATCH 2/2] ARM: cache: assume 64-byte L1 cachelines for ARMv7 CPUs

[Will Deacon]

On Mon, Jan 16, 2012 at 04:09:31PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 16, 2012 at 04:52:14PM +0100, Shilimkar, Santosh wrote:
> > Will,
> > On Mon, Jan 16, 2012 at 4:44 PM, Will Deacon <will.deacon@arm.com> wrote:
> > > To ensure correct alignment of cacheline-aligned data, the maximum
> > > cacheline size needs to be known at compile time.
> > >
> > > Since Cortex-A8 and Cortex-A15 have 64-byte cachelines (and it is likely
> > > that there will be future ARMv7 implementations with the same line size)
> > > then it makes sense to assume that CPU_V7 implies a 64-byte L1 cacheline
> > > size. For CPUs with smaller caches, this will result in some harmless
> > > padding but will help with single zImage work and avoid hitting subtle
> > > bugs with misaligned data structures.
> > >
> > > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > > ---
> > > ?arch/arm/mm/Kconfig | ? ?2 +-
> > > ?1 files changed, 1 insertions(+), 1 deletions(-)
> > >
> > > diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
> > > index 4cefb57..493e5ea5 100644
> > > --- a/arch/arm/mm/Kconfig
> > > +++ b/arch/arm/mm/Kconfig
> > > @@ -887,7 +887,7 @@ config ARM_L1_CACHE_SHIFT_6
> > >
> > > ?config ARM_L1_CACHE_SHIFT
> > > ? ? ? ?int
> > > - ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6
> > > + ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6 || CPU_V7
> > 
> > Will be really harmless on A9 ? We have L2 also to be
> > considered here which hard codes the line size as 32.
> 
> Going for a larger cache line size is safe: this is used for aligning
> data structures and similar, and aligning to 64-byte means that it's
> also 32-byte aligned.

Indeed, it should be fine. As for L2, that's something that should only
affect the L2CC code so we don't need a compile-time definition for that.

I might rework this code anyway (this was just to see what people think of
the idea) so that ARM_L1_CACHE_SHIFT_6 is selected by CPU_V7. Then I can
remove the explicit selects of the config option by platforms that already
select CPU_V7.

Will

[PATCH 2/2] ARM: cache: assume 64-byte L1 cachelines for ARMv7 CPUs

[Shilimkar, Santosh]

On Mon, Jan 16, 2012 at 5:09 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Jan 16, 2012 at 04:52:14PM +0100, Shilimkar, Santosh wrote:
>> Will,
>> On Mon, Jan 16, 2012 at 4:44 PM, Will Deacon <will.deacon@arm.com> wrote:
>> > To ensure correct alignment of cacheline-aligned data, the maximum
>> > cacheline size needs to be known at compile time.
>> >
>> > Since Cortex-A8 and Cortex-A15 have 64-byte cachelines (and it is likely
>> > that there will be future ARMv7 implementations with the same line size)
>> > then it makes sense to assume that CPU_V7 implies a 64-byte L1 cacheline
>> > size. For CPUs with smaller caches, this will result in some harmless
>> > padding but will help with single zImage work and avoid hitting subtle
>> > bugs with misaligned data structures.
>> >
>> > Signed-off-by: Will Deacon <will.deacon@arm.com>
>> > ---
>> > ?arch/arm/mm/Kconfig | ? ?2 +-
>> > ?1 files changed, 1 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/arch/arm/mm/Kconfig b/arch/arm/mm/Kconfig
>> > index 4cefb57..493e5ea5 100644
>> > --- a/arch/arm/mm/Kconfig
>> > +++ b/arch/arm/mm/Kconfig
>> > @@ -887,7 +887,7 @@ config ARM_L1_CACHE_SHIFT_6
>> >
>> > ?config ARM_L1_CACHE_SHIFT
>> > ? ? ? ?int
>> > - ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6
>> > + ? ? ? default 6 if ARM_L1_CACHE_SHIFT_6 || CPU_V7
>>
>> Will be really harmless on A9 ? We have L2 also to be
>> considered here which hard codes the line size as 32.
>
> Going for a larger cache line size is safe: this is used for aligning
> data structures and similar, and aligning to 64-byte means that it's
> also 32-byte aligned.

Got it. Thanks !!

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Stephen Boyd]

On 01/16/12 07:44, Will Deacon wrote:
> The linker script assumes a cacheline size of 32 bytes when aligning
> the .data..cacheline_aligned and .data..percpu sections.
>
> This patch updates the script to use L1_CACHE_BYTES, which should be set
> to 64 on platforms that require it.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>
> v2: incorporated suggestions from Stephen Boyd

What do you think about aligning the exception fixup table to the same
value?

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Will Deacon]

Hi Stephen,

On Mon, Jan 16, 2012 at 06:22:34PM +0000, Stephen Boyd wrote:
> On 01/16/12 07:44, Will Deacon wrote:
> > The linker script assumes a cacheline size of 32 bytes when aligning
> > the .data..cacheline_aligned and .data..percpu sections.
> >
> > This patch updates the script to use L1_CACHE_BYTES, which should be set
> > to 64 on platforms that require it.
> >
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >
> > v2: incorporated suggestions from Stephen Boyd
> 
> What do you think about aligning the exception fixup table to the same
> value?

Hmm, I'm not sure I see what that gains us over the current 32-byte
alignment. Are you seeing any measurable performance difference with it being
cacheline-aligned?

Will

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Stephen Boyd]

On 01/16/12 10:26, Will Deacon wrote:
> On Mon, Jan 16, 2012 at 06:22:34PM +0000, Stephen Boyd wrote:
>>
>> What do you think about aligning the exception fixup table to the same
>> value?
> Hmm, I'm not sure I see what that gains us over the current 32-byte
> alignment. Are you seeing any measurable performance difference with it being
> cacheline-aligned?
>

I haven't measured anything yet. I just see that it's another value
hard-coded to 32 in the linker script and that tile has decided to use
L1_CACHE_BYTES for it.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Will Deacon]

On Mon, Jan 16, 2012 at 06:30:23PM +0000, Stephen Boyd wrote:
> On 01/16/12 10:26, Will Deacon wrote:
> > On Mon, Jan 16, 2012 at 06:22:34PM +0000, Stephen Boyd wrote:
> >>
> >> What do you think about aligning the exception fixup table to the same
> >> value?
> > Hmm, I'm not sure I see what that gains us over the current 32-byte
> > alignment. Are you seeing any measurable performance difference with it being
> > cacheline-aligned?
> >
> 
> I haven't measured anything yet. I just see that it's another value
> hard-coded to 32 in the linker script and that tile has decided to use
> L1_CACHE_BYTES for it.

Hmm, that's intriguing. The exception table entries are 8 bytes on ARM and
are aligned as such by things like the USER macro, so I'm not sure why we
align the section to 32 bytes. I guess it must be for performance reasons,
but whether that's due to cacheline size, I don't know.

Nico, can you enlighten us please (it was introduced in 13b1f64c ("[ARM]
3008/1: the exception table is not read-only"))?

Cheers,

Will

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Nicolas Pitre]

On Mon, 16 Jan 2012, Will Deacon wrote:

> On Mon, Jan 16, 2012 at 06:30:23PM +0000, Stephen Boyd wrote:
> > On 01/16/12 10:26, Will Deacon wrote:
> > > On Mon, Jan 16, 2012 at 06:22:34PM +0000, Stephen Boyd wrote:
> > >>
> > >> What do you think about aligning the exception fixup table to the same
> > >> value?
> > > Hmm, I'm not sure I see what that gains us over the current 32-byte
> > > alignment. Are you seeing any measurable performance difference with it being
> > > cacheline-aligned?
> > >
> > 
> > I haven't measured anything yet. I just see that it's another value
> > hard-coded to 32 in the linker script and that tile has decided to use
> > L1_CACHE_BYTES for it.
> 
> Hmm, that's intriguing. The exception table entries are 8 bytes on ARM and
> are aligned as such by things like the USER macro, so I'm not sure why we
> align the section to 32 bytes. I guess it must be for performance reasons,
> but whether that's due to cacheline size, I don't know.
> 
> Nico, can you enlighten us please (it was introduced in 13b1f64c ("[ARM]
> 3008/1: the exception table is not read-only"))?

It probably was due to the fact that the surrounding data was also cache 
line aligned.  I don't think there is any particular reason why it 
should be aligned to a cache line boundary given that the list is 
accessed with a binary search.  The minimum required alignment would be 
__alignof__(struct exception_table_entry).


Nicolas

[PATCH 1/2] ARM: vmlinux.lds.S: do not hardcode cacheline size as 32 bytes

[Will Deacon]

On Mon, Jan 16, 2012 at 07:35:43PM +0000, Nicolas Pitre wrote:
> On Mon, 16 Jan 2012, Will Deacon wrote:
> > On Mon, Jan 16, 2012 at 06:30:23PM +0000, Stephen Boyd wrote:
> > > I haven't measured anything yet. I just see that it's another value
> > > hard-coded to 32 in the linker script and that tile has decided to use
> > > L1_CACHE_BYTES for it.
> > 
> > Hmm, that's intriguing. The exception table entries are 8 bytes on ARM and
> > are aligned as such by things like the USER macro, so I'm not sure why we
> > align the section to 32 bytes. I guess it must be for performance reasons,
> > but whether that's due to cacheline size, I don't know.
> > 
> > Nico, can you enlighten us please (it was introduced in 13b1f64c ("[ARM]
> > 3008/1: the exception table is not read-only"))?
> 
> It probably was due to the fact that the surrounding data was also cache 
> line aligned.  I don't think there is any particular reason why it 
> should be aligned to a cache line boundary given that the list is 
> accessed with a binary search.  The minimum required alignment would be 
> __alignof__(struct exception_table_entry).

Given that the struct just contains two unsigned longs, we should be alright
with 4 byte alignment here. I can add a follow-on patch to change that.

Thanks,

Will