Cache line size definition in arch/arm/mm/Kconfig

Cache line size definition in arch/arm/mm/Kconfig

[Mason]

Hello everyone,

AFAICT, L1 cache line size is specified in arch/arm/mm/Kconfig

config ARM_L1_CACHE_SHIFT_6
	bool
	default y if CPU_V7
	help
	  Setting ARM L1 cache line size to 64 Bytes.

config ARM_L1_CACHE_SHIFT
	int
	default 6 if ARM_L1_CACHE_SHIFT_6
	default 5


I'm using a Cortex A9 MPCore. If I'm not mistaken, the cache line size
is 32 bytes, even though this CPU is ARMv7.

http://infocenter.arm.com/help/topic/com.arm.doc.ddi0388g/Caccifbd.html

> The Cortex-A9 processor has separate instruction and data caches.
> The caches have the following features:
>
>   Each cache can be disabled independently. See System Control Register.
>   Both caches are 4-way set-associative.
>   The cache line length is eight words.
>   On a cache miss, critical word first filling of the cache is performed.
>   You can configure the instruction and data caches independently during implementation to sizes of 16KB, 32KB, or 64KB.
>   To reduce power consumption, the number of full cache reads is reduced by taking advantage of the sequential nature of many cache operations. If a cache read is sequential to the previous cache read, and the read is within the same cache line, only the data RAM set that was previously read is accessed.


How do I set ARM_L1_CACHE_SHIFT_6 to 'n' in my platform Kconfig?

Or perhaps I should "override" ARM_L1_CACHE_SHIFT to 5 (again in
my platform Kconfig). I don't know the syntax to do that.

Could someone point out the correct way?

Regards.

Cache line size definition in arch/arm/mm/Kconfig

[Mason]

On 25/03/2015 15:35, Mason wrote:

> AFAICT, L1 cache line size is specified in arch/arm/mm/Kconfig
>
> config ARM_L1_CACHE_SHIFT_6
>      bool
>      default y if CPU_V7
>      help
>        Setting ARM L1 cache line size to 64 Bytes.
>
> config ARM_L1_CACHE_SHIFT
>      int
>      default 6 if ARM_L1_CACHE_SHIFT_6
>      default 5
>
>
> I'm using a Cortex A9 MPCore. If I'm not mistaken, the cache line size
> is 32 bytes, even though this CPU is ARMv7.
>
> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0388g/Caccifbd.html
>
>> The Cortex-A9 processor has separate instruction and data caches.
>> The caches have the following features:
>>
>>   Each cache can be disabled independently. See System Control Register.
>>   Both caches are 4-way set-associative.
>>   The cache line length is eight words.
>>   On a cache miss, critical word first filling of the cache is performed.
>>   You can configure the instruction and data caches independently during implementation to sizes of 16KB, 32KB, or 64KB.
>>   To reduce power consumption, the number of full cache reads is reduced by taking advantage of the sequential nature of many cache operations. If a cache read is sequential to the previous cache read, and the read is within the same cache line, only the data RAM set that was previously read is accessed.
>
> How do I set ARM_L1_CACHE_SHIFT_6 to 'n' in my platform Kconfig?
>
> Or perhaps I should "override" ARM_L1_CACHE_SHIFT to 5 (again in
> my platform Kconfig). I don't know the syntax to do that.
>
> Could someone point out the correct way?

Would someone care to comment? :-)

Someone mentioned that some implementations prefetch 64 bytes from
L2D into L1D, so it might make sense to align "hot" data to 64 bytes
to benefit from this mechanism?

It seems we might waste a few hundred/thousand bytes if aligning to
cache line is a frequent operation, though.

On a tangential note, it seems likely that Linux could determine the
size of L1 at run-time, by poking the right implementation-specific
register. Is it not better to get the information at run-time, rather
than as a compile-time constant?

(I have the same general question about PERIPH_BASE, patch incoming.)

Thoughts?

Regards.

Cache line size definition in arch/arm/mm/Kconfig

[Russell King - ARM Linux]

On Fri, Mar 27, 2015 at 12:42:54PM +0100, Mason wrote:
> On 25/03/2015 15:35, Mason wrote:
> 
> >AFAICT, L1 cache line size is specified in arch/arm/mm/Kconfig
> >
> >config ARM_L1_CACHE_SHIFT_6
> >     bool
> >     default y if CPU_V7
> >     help
> >       Setting ARM L1 cache line size to 64 Bytes.
> >
> >config ARM_L1_CACHE_SHIFT
> >     int
> >     default 6 if ARM_L1_CACHE_SHIFT_6
> >     default 5
> >
> >
> >I'm using a Cortex A9 MPCore. If I'm not mistaken, the cache line size
> >is 32 bytes, even though this CPU is ARMv7.
> >
> >http://infocenter.arm.com/help/topic/com.arm.doc.ddi0388g/Caccifbd.html
> >
> >>The Cortex-A9 processor has separate instruction and data caches.
> >>The caches have the following features:
> >>
> >>  Each cache can be disabled independently. See System Control Register.
> >>  Both caches are 4-way set-associative.
> >>  The cache line length is eight words.
> >>  On a cache miss, critical word first filling of the cache is performed.
> >>  You can configure the instruction and data caches independently during implementation to sizes of 16KB, 32KB, or 64KB.
> >>  To reduce power consumption, the number of full cache reads is reduced by taking advantage of the sequential nature of many cache operations. If a cache read is sequential to the previous cache read, and the read is within the same cache line, only the data RAM set that was previously read is accessed.
> >
> >How do I set ARM_L1_CACHE_SHIFT_6 to 'n' in my platform Kconfig?
> >
> >Or perhaps I should "override" ARM_L1_CACHE_SHIFT to 5 (again in
> >my platform Kconfig). I don't know the syntax to do that.
> >
> >Could someone point out the correct way?
> 
> Would someone care to comment? :-)

No :)

What you've found is the _static_ L1 cache line size setting, which is
used at _compile_ time to align structures while building.  To allow
maximum flexibility - and because there are 64-byte cache line ARMv7
implementations around - we have decided that the _compile time_
cache line size will be 64 bytes.

As far as cache operations are concerned, they will know the correct
cache line size for the CPU which they're running on, so the code
will adapt.

It has the side effect that some allocators also assume that the L1
cache line size is 64 bytes.

It's better to have a larger than necessary cache line size than a
smaller one, because a larger one is automatically aligned to the
smaller sizes.

In other words, this is totally intentional.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Cache line size definition in arch/arm/mm/Kconfig

[Mason]

On 27/03/2015 13:06, Russell King - ARM Linux wrote:
> On Fri, Mar 27, 2015 at 12:42:54PM +0100, Mason wrote:
>> On 25/03/2015 15:35, Mason wrote:
>>
>>> AFAICT, L1 cache line size is specified in arch/arm/mm/Kconfig
>>>
>>> config ARM_L1_CACHE_SHIFT_6
>>>      bool
>>>      default y if CPU_V7
>>>      help
>>>        Setting ARM L1 cache line size to 64 Bytes.
>>>
>>> config ARM_L1_CACHE_SHIFT
>>>      int
>>>      default 6 if ARM_L1_CACHE_SHIFT_6
>>>      default 5
>>>
>>>
>>> I'm using a Cortex A9 MPCore. If I'm not mistaken, the cache line size
>>> is 32 bytes, even though this CPU is ARMv7.
>>>
>>> http://infocenter.arm.com/help/topic/com.arm.doc.ddi0388g/Caccifbd.html
>>>
>>>> The Cortex-A9 processor has separate instruction and data caches.
>>>> The caches have the following features:
>>>>
>>>>   Each cache can be disabled independently. See System Control Register.
>>>>   Both caches are 4-way set-associative.
>>>>   The cache line length is eight words.
>>>>   On a cache miss, critical word first filling of the cache is performed.
>>>>   You can configure the instruction and data caches independently during implementation to sizes of 16KB, 32KB, or 64KB.
>>>>   To reduce power consumption, the number of full cache reads is reduced by taking advantage of the sequential nature of many cache operations. If a cache read is sequential to the previous cache read, and the read is within the same cache line, only the data RAM set that was previously read is accessed.
>>>
>>> How do I set ARM_L1_CACHE_SHIFT_6 to 'n' in my platform Kconfig?
>>>
>>> Or perhaps I should "override" ARM_L1_CACHE_SHIFT to 5 (again in
>>> my platform Kconfig). I don't know the syntax to do that.
>>>
>>> Could someone point out the correct way?
>>
>> Would someone care to comment? :-)
>
> No :)

I'm glad that you've decided to disagree with yourself! :-)

> What you've found is the _static_ L1 cache line size setting, which is
> used at _compile_ time to align structures while building.  To allow
> maximum flexibility - and because there are 64-byte cache line ARMv7
> implementations around - we have decided that the _compile time_
> cache line size will be 64 bytes.

Right, I had a complete brain malfunction there. Compiler needs to be
told the cache line size to properly align relevant objects.

> As far as cache operations are concerned, they will know the correct
> cache line size for the CPU which they're running on, so the code
> will adapt.
>
> It has the side effect that some allocators also assume that the L1
> cache line size is 64 bytes.
>
> It's better to have a larger than necessary cache line size than a
> smaller one, because a larger one is automatically aligned to the
> smaller sizes.
>
> In other words, this is totally intentional.

I don't understand why I should not override ARM_L1_CACHE_SHIFT to 5
in my platform-specific Kconfig, since I know I have a 32-byte cache
line size?


Oh and while I have your attention ;-) I have alignment-related
questions about clocksource_mmio_init() (commit 442c8176d2) wrt
Thomas Gleixner's 369db4c952 patch. (I think the two patches
do not play nice.)

369db4c952 moved some struct clocksource fields around to group
hot fields in a single cache line at the beginning of the struct,
and marked the struct as cache aligned. This works as expected
with static structures.

However, I don't think it works as expected with dynamically
allocated struct clocksource. It seems to me (and I may very
well be wrong!) that struct clocksource_mmio should have the
clksrc field at the beginning of the struct, and we should
use an allocation function that returns cache aligned memory?

struct clocksource_mmio {
	struct clocksource clksrc;
	void __iomem *reg;
};

   cs = kmagic_cache_alloc(sizeof *cs, GFP_KERNEL);

That way clksrc would effectively be cache aligned, right?


One thing that caught me off-guard: when CONFIG_ARCH_CLOCKSOURCE_DATA
and CONFIG_CLOCKSOURCE_WATCHDOG are undefined, struct clocksource
weighs 80 bytes on a 32-bit system. I would expect the "reg" field
at the end to "fit in the hole", but in fact, gcc seems to "stretch"
struct clocksource before considering other fields. This may be a
bug in gcc's extension?

Regards.

Cache line size definition in arch/arm/mm/Kconfig

[Mason]

On 27/03/2015 14:45, Mason wrote:

> arch/arm/mm/Kconfig
>
> config ARM_L1_CACHE_SHIFT_6
>      bool
>      default y if CPU_V7
>      help
>        Setting ARM L1 cache line size to 64 Bytes.
>
> config ARM_L1_CACHE_SHIFT
>      int
>      default 6 if ARM_L1_CACHE_SHIFT_6
>      default 5
>
> I don't understand why I should not override ARM_L1_CACHE_SHIFT to 5
> in my platform-specific Kconfig, since I know I have a 32-byte cache
> line size?

[large snip]

It seems to me it would make sense to override ARM_L1_CACHE_SHIFT in
the platform Kconfig. Or am I missing something obvious? (Perhaps it
is expected that the gains would be minimal?)

ARM_L1_CACHE_SHIFT = 6
   2 .rodata       0008ea58  c026a000  c026a000  0026a000  2**6
  20 .data         00027aa0  c033a000  c033a000  0034a000  2**6

ARM_L1_CACHE_SHIFT = 5
   2 .rodata       0008e608  c026b000  c026b000  0026b000  2**5
  20 .data         000252c0  c033a000  c033a000  0034a000  2**5

  1104 bytes saved in .rodata (0.2%)
10208 bytes saved in   .data (6.3%)

This is for a minimal kernel (no drivers, only core).
The space optimization seems to be not insignificant.

> Oh and while I have your attention ;-) I have alignment-related
> questions about clocksource_mmio_init() (commit 442c8176d2) wrt
> Thomas Gleixner's 369db4c952 patch. (I think the two patches
> do not play nice.)
>
> 369db4c952 moved some struct clocksource fields around to group
> hot fields in a single cache line at the beginning of the struct,
> and marked the struct as cache aligned. This works as expected
> with static structures.

Example patch for illustration purposes (only compile-tested)

(There is probably a much more elegant way to get 32-byte aligned
memory allocations.)

Regards.


diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index c0e2512..77b91c5 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -10,34 +10,24 @@
  #include <linux/init.h>
  #include <linux/slab.h>
  
-struct clocksource_mmio {
-       void __iomem *reg;
-       struct clocksource clksrc;
-};
-
-static inline struct clocksource_mmio *to_mmio_clksrc(struct clocksource *c)
-{
-       return container_of(c, struct clocksource_mmio, clksrc);
-}
-
  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
  {
-       return readl_relaxed(to_mmio_clksrc(c)->reg);
+       return readl_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readl_down(struct clocksource *c)
  {
-       return ~readl_relaxed(to_mmio_clksrc(c)->reg);
+       return ~readl_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readw_up(struct clocksource *c)
  {
-       return readw_relaxed(to_mmio_clksrc(c)->reg);
+       return readw_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readw_down(struct clocksource *c)
  {
-       return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
+       return ~(unsigned)readw_relaxed(c->reg);
  }
  
  /**
@@ -53,21 +43,22 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
         unsigned long hz, int rating, unsigned bits,
         cycle_t (*read)(struct clocksource *))
  {
-       struct clocksource_mmio *cs;
+       struct clocksource *cs;
  
         if (bits > 32 || bits < 16)
                 return -EINVAL;
  
-       cs = kzalloc(sizeof(struct clocksource_mmio), GFP_KERNEL);
+       cs = kzalloc(sizeof *cs + 31, GFP_KERNEL);
         if (!cs)
                 return -ENOMEM;
+       cs = PTR_ALIGN(cs, 32);
  
         cs->reg = base;
-       cs->clksrc.name = name;
-       cs->clksrc.rating = rating;
-       cs->clksrc.read = read;
-       cs->clksrc.mask = CLOCKSOURCE_MASK(bits);
-       cs->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+       cs->name = name;
+       cs->rating = rating;
+       cs->read = read;
+       cs->mask = CLOCKSOURCE_MASK(bits);
+       cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
  
-       return clocksource_register_hz(&cs->clksrc, hz);
+       return clocksource_register_hz(cs, hz);
  }
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 879065d..8b1d689 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -189,6 +189,9 @@ struct clocksource {
         unsigned long flags;
         void (*suspend)(struct clocksource *cs);
         void (*resume)(struct clocksource *cs);
+#ifdef CONFIG_CLKSRC_MMIO
+       void __iomem *reg;
+#endif
  
         /* private: */
  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG

Cache line size definition in arch/arm/mm/Kconfig

[Russell King - ARM Linux]

On Wed, Apr 01, 2015 at 01:42:55PM +0200, Mason wrote:
> Example patch for illustration purposes (only compile-tested)
> 
> (There is probably a much more elegant way to get 32-byte aligned
> memory allocations.)
> 
> Regards.
> 
> 
> diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
> index c0e2512..77b91c5 100644
> --- a/drivers/clocksource/mmio.c
> +++ b/drivers/clocksource/mmio.c
> @@ -10,34 +10,24 @@
>  #include <linux/init.h>
>  #include <linux/slab.h>
> -struct clocksource_mmio {
> -       void __iomem *reg;
> -       struct clocksource clksrc;

Just swap the order of these.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Cache line size definition in arch/arm/mm/Kconfig

[Mason]

On 01/04/2015 13:50, Russell King - ARM Linux wrote:
> On Wed, Apr 01, 2015 at 01:42:55PM +0200, Mason wrote:
>> Example patch for illustration purposes (only compile-tested)
>>
>> (There is probably a much more elegant way to get 32-byte aligned
>> memory allocations.)
>>
>> diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
>> index c0e2512..77b91c5 100644
>> --- a/drivers/clocksource/mmio.c
>> +++ b/drivers/clocksource/mmio.c
>> @@ -10,34 +10,24 @@
>>   #include <linux/init.h>
>>   #include <linux/slab.h>
>> -struct clocksource_mmio {
>> -       void __iomem *reg;
>> -       struct clocksource clksrc;
>
> Just swap the order of these.

(Assuming arm32 && ARM_L1_CACHE_SHIFT = 6)

Because struct clocksource has attribute ____cacheline_aligned,
sizeof(struct clocksource) = 128

Thus the layout for
struct { void __iomem *reg; struct clocksource clksrc; }
would be
reg(4) pad1(60) clksrc(80) pad2(48)

and the layout for
struct { struct clocksource clksrc; void __iomem *reg; }
would be
clksrc(80) pad2(48) reg(4) pad1(60)

If kmalloc returns  32-byte aligned pointer (which seems to be
the case) then clksrc is correctly aligned in both cases.

The reason for stuffing reg inside struct clocksource would be
to avoid the 64-byte overhead from the padding. Also (but this
only a matter of taste) it seems the code is marginally clearer
without the "container_of" gymnastics.

Regards.


diff --git a/drivers/clocksource/mmio.c b/drivers/clocksource/mmio.c
index c0e2512..d6583ed 100644
--- a/drivers/clocksource/mmio.c
+++ b/drivers/clocksource/mmio.c
@@ -10,34 +10,24 @@
  #include <linux/init.h>
  #include <linux/slab.h>
  
-struct clocksource_mmio {
-       void __iomem *reg;
-       struct clocksource clksrc;
-};
-
-static inline struct clocksource_mmio *to_mmio_clksrc(struct clocksource *c)
-{
-       return container_of(c, struct clocksource_mmio, clksrc);
-}
-
  cycle_t clocksource_mmio_readl_up(struct clocksource *c)
  {
-       return readl_relaxed(to_mmio_clksrc(c)->reg);
+       return readl_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readl_down(struct clocksource *c)
  {
-       return ~readl_relaxed(to_mmio_clksrc(c)->reg);
+       return ~readl_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readw_up(struct clocksource *c)
  {
-       return readw_relaxed(to_mmio_clksrc(c)->reg);
+       return readw_relaxed(c->reg);
  }
  
  cycle_t clocksource_mmio_readw_down(struct clocksource *c)
  {
-       return ~(unsigned)readw_relaxed(to_mmio_clksrc(c)->reg);
+       return ~(unsigned)readw_relaxed(c->reg);
  }
  
  /**
@@ -53,21 +43,21 @@ int __init clocksource_mmio_init(void __iomem *base, const char *name,
         unsigned long hz, int rating, unsigned bits,
         cycle_t (*read)(struct clocksource *))
  {
-       struct clocksource_mmio *cs;
+       struct clocksource *cs;
  
         if (bits > 32 || bits < 16)
                 return -EINVAL;
  
-       cs = kzalloc(sizeof(struct clocksource_mmio), GFP_KERNEL);
+       cs = kzalloc(sizeof *cs, GFP_KERNEL);
         if (!cs)
                 return -ENOMEM;
  
         cs->reg = base;
-       cs->clksrc.name = name;
-       cs->clksrc.rating = rating;
-       cs->clksrc.read = read;
-       cs->clksrc.mask = CLOCKSOURCE_MASK(bits);
-       cs->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS;
+       cs->name = name;
+       cs->rating = rating;
+       cs->read = read;
+       cs->mask = CLOCKSOURCE_MASK(bits);
+       cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
  
-       return clocksource_register_hz(&cs->clksrc, hz);
+       return clocksource_register_hz(cs, hz);
  }
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 879065d..9fdb5cf 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -189,6 +189,9 @@ struct clocksource {
         unsigned long flags;
         void (*suspend)(struct clocksource *cs);
         void (*resume)(struct clocksource *cs);
+#ifdef CONFIG_CLKSRC_MMIO
+       void __iomem *reg;
+#endif
  
         /* private: */
  #ifdef CONFIG_CLOCKSOURCE_WATCHDOG