Re: [-next] openvswitch BUILD_BUG_ON failed

[Geert Uytterhoeven]

On Tue, Sep 3, 2013 at 11:44 PM, Jesse Gross <jesse@xxxxxxxxxx> wrote:
> On Sat, Aug 31, 2013 at 5:11 AM, Geert Uytterhoeven
> <geert@xxxxxxxxxxxxxx> wrote:
>> On Fri, Aug 30, 2013 at 3:11 AM, Jesse Gross <jesse@xxxxxxxxxx> wrote:
>>> On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@xxxxxxxxxxxxx> wrote:
>>>> From: Jesse Gross <jesse@xxxxxxxxxx>
>>>> Date: Thu, 29 Aug 2013 14:42:22 -0700
>>>>
>>>>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>>>>> <geert@xxxxxxxxxxxxxx> wrote:
>>>>>> However, I have some doubts about other alignment "enforcements":
>>>>>>
>>>>>> "__aligned(__alignof__(long))" makes the whole struct aligned to the
>>>>>> alignment rule for "long":
>>>>>>    1. This is only 2 bytes on m68k, i.e. != sizeof(long).
>>>>>>    2. This is 4 bytes on many 32-bit platforms, which may be less than the
>>>>>>       default alignment for "__be64" (cfr. some members of struct
>>>>>>       ovs_key_ipv4_tunnel), so this may make those 64-bit members unaligned.
>>>>>
>>>>> Do any of those 32-bit architectures actually care about alignment of
>>>>> 64 bit values? On 32-bit x86, a long is 32 bits but the alignment
>>>>> requirement of __be64 is also 32 bit.
>>>>
>>>> All except x86-32 do, it is in fact the odd man out with respect to this
>>>> issue.
>>>
>>> Thanks, good to know.
>>>
>>> Andy, do you want to modify your patch to just drop the alignment
>>> specification as Geert suggested (but definitely keep the new build
>>> assert that you added)? It's probably better to just send the patch to
>>> netdev (against net-next) as well since you'll likely get better
>>> comments there and we can fix this faster if you cut out the
>>> middleman.
>>
>> Why do you want to keep the build asserts?
>> Is this in-memory structure also transfered as-is over the network?
>> If yes, you definitely want the padding.
>
> Well they caught this bug and really don't cost anything.
>
>> Nevertheless, as the struct contains u32 and even __be64 members, the
>> size of the struct will always be a multiple of the alignment unit for
>> 64-bit quantities (and thus also for long), as per the C standard.
>> Hence the check
>>
>>     BUILD_BUG_ON(sizeof(struct sw_flow_key) % __alignof__(long));
>>
>> will only catch bad compiler bugs or people adding __packed to the struct.
>
> It's possible that we might want to pack the structure in the future.
> More generally though, the contents of the struct is really
> independent of the alignment requirements here because we're accessing
> it as an array of bytes in long-sized chunks so implicitly depending
> on the size of the members is not that great.

So you're accessing it as an array of bytes in long-sized chunks.
What are you doing with this accessed data?
Transfering over the network?
Storing on disk?
Then it must be portable across machines and architectures, right?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/