Re: [-next] openvswitch BUILD_BUG_ON failed

[Jesse Gross]

On Tue, Sep 3, 2013 at 11:55 PM, Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
> 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?

It's just an in-memory hash table lookup. No one else ever sees it.
--
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/