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

This is a discussion on Re: [RFC-PATCH 1/5] unaligned: introduce common header - Kernel ; On Mon, Nov 10, 2008 at 4:22 AM, Harvey Harrison 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 ...

+ Reply to Thread
Results 1 to 5 of 5

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

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

    On Mon, Nov 10, 2008 at 4:22 AM, Harvey Harrison
    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
    >> 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:

    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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, 2008-11-10 at 11:49 +0000, Will Newton wrote:
    > On Mon, Nov 10, 2008 at 4:22 AM, Harvey Harrison
    > 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
    > >> 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.

    > 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.

    Harvey

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, Nov 10, 2008 at 4:51 PM, Harvey Harrison
    wrote:
    > On Mon, 2008-11-10 at 11:49 +0000, Will Newton wrote:
    >> On Mon, Nov 10, 2008 at 4:22 AM, Harvey Harrison
    >> 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
    >> >> 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.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, 2008-11-10 at 18:35 +0000, Will Newton wrote:
    > On Mon, Nov 10, 2008 at 4:51 PM, Harvey Harrison
    > wrote:
    > >
    > > 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:


    Actually it's documented that putting attribute(packed) on the struct
    is equivalent to putting attribute(packed) on _every_ member of the
    struct.

    > 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.


    Well, it's kind of the whole point of __packed isn't it? Otherwise the
    struct members get naturally (or some arch-dependent value) aligned,
    which the compiler can rely on unless you say __packed.

    So in my example above, the compiler _knows_ how it has aligned
    big_data and small_data and can use whatever access is most efficient,
    but it can't make any assumptions about medium_data, so access through
    a pointer _must_ be done unaligned.

    struct foo *bar;
    bar->medium_data; // compiler must do this unaligned

    Harvey

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

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

    On Mon, Nov 10, 2008 at 6:51 PM, Harvey Harrison
    wrote:
    > On Mon, 2008-11-10 at 18:35 +0000, Will Newton wrote:
    >> On Mon, Nov 10, 2008 at 4:51 PM, Harvey Harrison
    >> wrote:
    >> >
    >> > 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:

    >
    > Actually it's documented that putting attribute(packed) on the struct
    > is equivalent to putting attribute(packed) on _every_ member of the
    > struct.
    >
    >> 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.

    >
    > Well, it's kind of the whole point of __packed isn't it? Otherwise the
    > struct members get naturally (or some arch-dependent value) aligned,
    > which the compiler can rely on unless you say __packed.
    >
    > So in my example above, the compiler _knows_ how it has aligned
    > big_data and small_data and can use whatever access is most efficient,
    > but it can't make any assumptions about medium_data, so access through
    > a pointer _must_ be done unaligned.
    >
    > struct foo *bar;
    > bar->medium_data; // compiler must do this unaligned


    Agreed, but the packed struct unaligned access code does this:

    struct __una_u16 { u16 x __attribute__((packed)); };

    On an architecture with struct alignment > 1 this will not work. Which
    is why I believe that the memmove code must stay for those
    architectures.
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread