Re: [RFC-PATCH 1/5] unaligned: introduce common header

["Will Newton" <will.newton@gmail.com>]

On Mon, Nov 10, 2008 at 4:51 PM, Harvey Harrison
<harvey.harrison@gmail.com> wrote:
> On Mon, 2008-11-10 at 11:49 +0000, Will Newton wrote:
>> On Mon, Nov 10, 2008 at 4:22 AM, Harvey Harrison
>> <harvey.harrison@gmail.com> wrote:
>>
>> (add back lkml cc that I mistakenly dropped)
>>
>> > On Sat, 2008-11-08 at 12:47 +0000, Will Newton wrote:
>> >> On Wed, Nov 5, 2008 at 6:16 PM, Harvey Harrison
>> >> <harvey.harrison@gmail.com> wrote:
>> >>
>> >> > The memmove-based arches (m32r, xtensa, h8300) are likely going to be fine with this change
>> >> > barring compiler bugs that made them go with memmove in the first place.
>> >>
>> >> As I understand it the need for the memmove implementation is not
>> >> compiler bugs but default struct alignment. The packed struct
>> >> implementation will only work with compilers where structs can be
>> >> aligned on byte boundaries, it's fairly common for RISC architectures
>> >> to align structs to 4 or 8 byte boundaries.
>> >
>> > Which I believe is disabled entirely using __attribute__((packed)), no?
>>
>> As far as I am aware the packed attribute is handled in this way for
>> some toolchains (arm in particular). Not everybody does it, and for
>> good reasons. For example if I have this struct on an architecture
>> with 8 byte default struct alignment:
>>
>
> I should have been more careful with my wording here, I meant that no
> alignment assumptions are made when accessing a packed struct through
> a pointer, as is the case with the kernel version.

I am not aware that this is the case. Do you have any references?

>> struct foo {
>>     u64 big_data;
>>     u8 small_data;
>>     u32 medium_data;
>> } __attribute__((packed));
>>
>> Should big_data be accessed as 8 byte load instructions rather than
>> one 64bit load instruction? It's a pretty large performance penalty to
>> pay when all I really want is for medium_data to be accessed
>> correctly.
>
> In this particular case, packed isn't right as you know big_data is
> aligned (as long as you can guarantee the struct alignment), so you'd
> probably want:
>
> struct foo {
>        u64 big_data;
>        u8 small_data;
>        u32 medium_data __attribute__((__packed__));
> }
>
> But that's not what we're talking about in the kernel's case.

Perhaps that would be a neater way of expressing what is required in
my simple example, but it's fairly common to use packed on the whole
struct which could be because a field that is "packed" by default on
one architecture might not be on another. You could mark every field
as packed but few people seem to do that and as far as I am aware
there is no documented difference between packing all members and the
whole struct. The gcc documentation for packed is pretty short:

  The packed attribute specifies that a variable or structure field
should have the smallest
  possible alignment—one byte for a variable, and one bit for a field,
unless you specify a
  larger value with the aligned attribute.

I'd love to know if the pointer alignment behaviour is widespread and
then maybe write a patch for the gcc manual.