arm: Remaining issue with alignment of __log_buf in printk.c

arm: Remaining issue with alignment of __log_buf in printk.c

[Jonathan Cameron]

Hi All,

My stargate 2 board refused to start and after bisection I ended
up at the same patch that Stephen found an alignment issue in.
Unfortunately Stephen's patch doesn't seem to have fixed the
issue for me.
https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c

Increasing the alignement for 32 bit systems to 8 seems to do the
job but I can't immediately think why...

System is a pxa27x strong arm.

Tree was effectively today's mainline.

Saddly I have too many other bits to fit in today for futher
investigation (hence this information free email!)


Jonathan

arm: Remaining issue with alignment of __log_buf in printk.c

[Jonathan Cameron]

Hi All,

My stargate 2 board refused to start and after bisection I ended
up at the same patch that Stephen found an alignment issue in.
Unfortunately Stephen's patch doesn't seem to have fixed the
issue for me.
https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c

Increasing the alignement for 32 bit systems to 8 seems to do the
job but I can't immediately think why...

System is a pxa27x strong arm.

Tree was effectively today's mainline.

Saddly I have too many other bits to fit in today for futher
investigation (hence this information free email!)


Jonathan

arm: Remaining issue with alignment of __log_buf in printk.c

[Stephen Warren]

On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
> Hi All,
> 
> My stargate 2 board refused to start and after bisection I ended
> up at the same patch that Stephen found an alignment issue in.
> Unfortunately Stephen's patch doesn't seem to have fixed the
> issue for me.
> https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
> Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
> 
> Increasing the alignement for 32 bit systems to 8 seems to do the
> job but I can't immediately think why...
> 
> System is a pxa27x strong arm.

The first element in the structure type that's actually stored in the
__log_buf array is a u64; see struct log in kernel/printk.c. Depending
on alignment rules, a u64 and a struct containing it might require a
4-byte or 8-byte alignment. The following link implies this might have
changed over time:

http://wiki.debian.org/ArmEabiPort#Struct_packing_and_alignment

(see "64-bit data type alignment" a little below that anchor). I'm not
sure what ABI the kernel expects to use internally, or your compiler;
perhaps you need the new EABI 8-byte alignment requirement for a u64 and
hence the struct as a whole, but Tegra (or my toolchain?) is OK with the
older 4-byte alignment for a u64 or struct?

Further, I'm not sure if the following alignment selection logic:

> #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> #define LOG_ALIGN 4
> #else
> #define LOG_ALIGN 8
> #endif

... uses the CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS for its intended
purpose?

Russell, can you please comment here. Thanks.

arm: Remaining issue with alignment of __log_buf in printk.c

[Russell King - ARM Linux]

On Sun, May 27, 2012 at 10:03:20AM -0600, Stephen Warren wrote:
> On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
> > Hi All,
> > 
> > My stargate 2 board refused to start and after bisection I ended
> > up at the same patch that Stephen found an alignment issue in.
> > Unfortunately Stephen's patch doesn't seem to have fixed the
> > issue for me.
> > https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
> > Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
> > 
> > Increasing the alignement for 32 bit systems to 8 seems to do the
> > job but I can't immediately think why...
> > 
> > System is a pxa27x strong arm.

First thing to point out is that PXA27x is Xscale, not StrongARM.

> The first element in the structure type that's actually stored in the
> __log_buf array is a u64; see struct log in kernel/printk.c. Depending
> on alignment rules, a u64 and a struct containing it might require a
> 4-byte or 8-byte alignment. The following link implies this might have
> changed over time:
> 
> http://wiki.debian.org/ArmEabiPort#Struct_packing_and_alignment
> 
> (see "64-bit data type alignment" a little below that anchor). I'm not
> sure what ABI the kernel expects to use internally, or your compiler;
> perhaps you need the new EABI 8-byte alignment requirement for a u64 and
> hence the struct as a whole, but Tegra (or my toolchain?) is OK with the
> older 4-byte alignment for a u64 or struct?
> 
> Further, I'm not sure if the following alignment selection logic:
> 
> > #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> > #define LOG_ALIGN 4
> > #else
> > #define LOG_ALIGN 8
> > #endif
> 
> ... uses the CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS for its intended
> purpose?
> 
> Russell, can you please comment here. Thanks.

And most likely it's using EABI which does want 8 byte alignment.  So this
should probably be fixed for EABI builds.

arm: Remaining issue with alignment of __log_buf in printk.c

[Jonathan Cameron]

Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

>On Sun, May 27, 2012 at 10:03:20AM -0600, Stephen Warren wrote:
>> On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
>> > Hi All,
>> > 
>> > My stargate 2 board refused to start and after bisection I ended
>> > up at the same patch that Stephen found an alignment issue in.
>> > Unfortunately Stephen's patch doesn't seem to have fixed the
>> > issue for me.
>> > https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
>> > Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
>> > 
>> > Increasing the alignement for 32 bit systems to 8 seems to do the
>> > job but I can't immediately think why...
>> > 
>> > System is a pxa27x strong arm.
>
>First thing to point out is that PXA27x is Xscale, not StrongARM.
Sorry my mistake!
>
>> The first element in the structure type that's actually stored in the
>> __log_buf array is a u64; see struct log in kernel/printk.c.
>Depending
>> on alignment rules, a u64 and a struct containing it might require a
>> 4-byte or 8-byte alignment. The following link implies this might
>have
>> changed over time:
>> 
>> http://wiki.debian.org/ArmEabiPort#Struct_packing_and_alignment
>> 
>> (see "64-bit data type alignment" a little below that anchor). I'm
>not
>> sure what ABI the kernel expects to use internally, or your compiler;
>> perhaps you need the new EABI 8-byte alignment requirement for a u64
>and
>> hence the struct as a whole, but Tegra (or my toolchain?) is OK with
>the
>> older 4-byte alignment for a u64 or struct?
>> 
>> Further, I'm not sure if the following alignment selection logic:
>> 
>> > #if !defined(CONFIG_64BIT) ||
>defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>> > #define LOG_ALIGN 4
>> > #else
>> > #define LOG_ALIGN 8
>> > #endif
>> 
>> ... uses the CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS for its intended
>> purpose?
>> 
>> Russell, can you please comment here. Thanks.
>
>And most likely it's using EABI which does want 8 byte alignment. 
Indeed I am using eabi.
 So
>this
>should probably be fixed for EABI builds.

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

arm: Remaining issue with alignment of __log_buf in printk.c

[Stephen Warren]

On 05/27/2012 10:03 AM, Stephen Warren wrote:
> On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
>> Hi All,
>>
>> My stargate 2 board refused to start and after bisection I ended
>> up at the same patch that Stephen found an alignment issue in.
>> Unfortunately Stephen's patch doesn't seem to have fixed the
>> issue for me.
>> https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
>> Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
>>
>> Increasing the alignement for 32 bit systems to 8 seems to do the
>> job but I can't immediately think why...
>>
>> System is a pxa27x strong arm.
...
> #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> #define LOG_ALIGN 4
> #else
> #define LOG_ALIGN 8
> #endif

Actually, why not replace that with:

#define LOG_ALIGN (__alignof__(struct log_buf))

That way, the compiler will calculate the arch-/ABI-appropriate
alignment value automatically and correctly in all cases, so we won't
have to fix that ifdef above.

arm: Remaining issue with alignment of __log_buf in printk.c

[Kay Sievers]

On Tue, May 29, 2012 at 6:14 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/27/2012 10:03 AM, Stephen Warren wrote:
>> On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
>>> Hi All,
>>>
>>> My stargate 2 board refused to start and after bisection I ended
>>> up at the same patch that Stephen found an alignment issue in.
>>> Unfortunately Stephen's patch doesn't seem to have fixed the
>>> issue for me.
>>> https://lkml.org/lkml/2012/5/10/510 is the thread. ?Patch from
>>> Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
>>>
>>> Increasing the alignement for 32 bit systems to 8 seems to do the
>>> job but I can't immediately think why...
>>>
>>> System is a pxa27x strong arm.
> ...
>> #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>> #define LOG_ALIGN 4
>> #else
>> #define LOG_ALIGN 8
>> #endif
>
> Actually, why not replace that with:
>
> #define LOG_ALIGN (__alignof__(struct log_buf))
>
> That way, the compiler will calculate the arch-/ABI-appropriate
> alignment value automatically and correctly in all cases, so we won't
> have to fix that ifdef above.

__alignof__(u64) will be 8 on x86_64, while the current logic results
in 4. Not sure if x86_64 would somehow benefit from that, or if it's
just a waste of bytes.

Are you sure it results in 4 on some architectures?

Kay

arm: Remaining issue with alignment of __log_buf in printk.c

[Stephen Warren]

On 05/29/2012 10:32 AM, Kay Sievers wrote:
> On Tue, May 29, 2012 at 6:14 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/27/2012 10:03 AM, Stephen Warren wrote:
>>> On 05/27/2012 06:39 AM, Jonathan Cameron wrote:
>>>> Hi All,
>>>>
>>>> My stargate 2 board refused to start and after bisection I ended
>>>> up at the same patch that Stephen found an alignment issue in.
>>>> Unfortunately Stephen's patch doesn't seem to have fixed the
>>>> issue for me.
>>>> https://lkml.org/lkml/2012/5/10/510 is the thread.  Patch from
>>>> Stephen is : f8450fca6ecdea38b5a882fdf6cd097e3ec8651c
>>>>
>>>> Increasing the alignement for 32 bit systems to 8 seems to do the
>>>> job but I can't immediately think why...
>>>>
>>>> System is a pxa27x strong arm.
>> ...
>>> #if !defined(CONFIG_64BIT) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
>>> #define LOG_ALIGN 4
>>> #else
>>> #define LOG_ALIGN 8
>>> #endif
>>
>> Actually, why not replace that with:
>>
>> #define LOG_ALIGN (__alignof__(struct log_buf))
>>
>> That way, the compiler will calculate the arch-/ABI-appropriate
>> alignment value automatically and correctly in all cases, so we won't
>> have to fix that ifdef above.
> 
> __alignof__(u64) will be 8 on x86_64, while the current logic results
> in 4. Not sure if x86_64 would somehow benefit from that, or if it's
> just a waste of bytes.
> 
> Are you sure it results in 4 on some architectures?

I have no idea to be honest, but I'd tend towards making this reliable
first, and optimizing a few bytes later.

Perhaps something like the following would work though:

> #if (__alignof__(struct log_buf) == 4) || defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS)
> #define LOG_ALIGN 4
> #else
> #define LOG_ALIGN 8
> #endif