[-next] openvswitch BUILD_BUG_ON failed

[-next] openvswitch BUILD_BUG_ON failed

[Geert Uytterhoeven]

On m68k, where the alignment of 32-bit words is 2 bytes:

net/openvswitch/flow.c:1984:2: error: call to
'__compiletime_assert_1984' declared with attribute error:
BUILD_BUG_ON failed: sizeof(struct sw_flow_key) % sizeof(long)

(http://kisskb.ellerman.id.au/kisskb/buildresult/9422860/)

This was introduced by commit 5828cd9a68873df1340b420371c02c47647878fb
Author: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
Date:   Tue Aug 27 13:02:21 2013 -0700

    openvswitch: optimize flow compare and mask functions

    Make sure the sw_flow_key structure and valid mask boundaries are always
    machine word aligned. Optimize the flow compare and mask operations
    using machine word size operations. This patch improves throughput on
    average by 15% when CPU is the bottleneck of forwarding packets.

    This patch is inspired by ideas and code from a patch submitted by Peter
    Klausler titled "replace memcmp() with specialized comparator".
    However, The original patch only optimizes for architectures
    support unaligned machine word access. This patch optimizes for all
    architectures.

A quick fix to satisfy the build check is to make the padding explicit
(gmail-whitespace-damaged diff):

diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index b65f885..15f08d9 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -78,6 +78,7 @@ struct sw_flow_key {
                u32     priority;       /* Packet QoS priority. */
                u32     skb_mark;       /* SKB mark. */
                u16     in_port;        /* Input switch port (or DP_MAX_PORTS).
+               u16     pad;
        } phy;
        struct {
                u8     src[ETH_ALEN];   /* Ethernet source address. */

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.
I guess you want (at least) 4 byte alignment on 32-bit, and prefer 8 byte
alignment on 64-bit?
Not specifying any alignment constraint will give you most of that (except
on 64-bit platforms where 64-bit words must be only 4-byte aligned).

There's another build check "BUILD_BUG_ON(sizeof(long) % sizeof(u32))".
Isn't this always true on Linux, as "long" is never smaller than 4 bytes?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

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

Re: [-next] openvswitch BUILD_BUG_ON failed

[Andy Zhou]

[-- Attachment #1.1: Type: text/plain, Size: 3350 bytes --]

Yes, fengguan.Wu has reported the issue. Just sent out a patch for review.
You are welcome to review it.

http://openvswitch.org/pipermail/dev/2013-August/031247.html


On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>wrote:

> On m68k, where the alignment of 32-bit words is 2 bytes:
>
> net/openvswitch/flow.c:1984:2: error: call to
> '__compiletime_assert_1984' declared with attribute error:
> BUILD_BUG_ON failed: sizeof(struct sw_flow_key) % sizeof(long)
>
> (http://kisskb.ellerman.id.au/kisskb/buildresult/9422860/)
>
> This was introduced by commit 5828cd9a68873df1340b420371c02c47647878fb
> Author: Andy Zhou <azhou-l0M0P4e3n4LQT0dZR+AlfA@public.gmane.org>
> Date:   Tue Aug 27 13:02:21 2013 -0700
>
>     openvswitch: optimize flow compare and mask functions
>
>     Make sure the sw_flow_key structure and valid mask boundaries are
> always
>     machine word aligned. Optimize the flow compare and mask operations
>     using machine word size operations. This patch improves throughput on
>     average by 15% when CPU is the bottleneck of forwarding packets.
>
>     This patch is inspired by ideas and code from a patch submitted by
> Peter
>     Klausler titled "replace memcmp() with specialized comparator".
>     However, The original patch only optimizes for architectures
>     support unaligned machine word access. This patch optimizes for all
>     architectures.
>
> A quick fix to satisfy the build check is to make the padding explicit
> (gmail-whitespace-damaged diff):
>
> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index b65f885..15f08d9 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
> @@ -78,6 +78,7 @@ struct sw_flow_key {
>                 u32     priority;       /* Packet QoS priority. */
>                 u32     skb_mark;       /* SKB mark. */
>                 u16     in_port;        /* Input switch port (or
> DP_MAX_PORTS).
> +               u16     pad;
>         } phy;
>         struct {
>                 u8     src[ETH_ALEN];   /* Ethernet source address. */
>
> 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.
> I guess you want (at least) 4 byte alignment on 32-bit, and prefer 8 byte
> alignment on 64-bit?
> Not specifying any alignment constraint will give you most of that (except
> on 64-bit platforms where 64-bit words must be only 4-byte aligned).
>
> There's another build check "BUILD_BUG_ON(sizeof(long) % sizeof(u32))".
> Isn't this always true on Linux, as "long" is never smaller than 4 bytes?
>

Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 --
> geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org
>
> 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
>

[-- Attachment #1.2: Type: text/html, Size: 4410 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [-next] openvswitch BUILD_BUG_ON failed

[Jesse Gross]

On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
<geert@linux-m68k.org> 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.

Re: [-next] openvswitch BUILD_BUG_ON failed

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Thu, 29 Aug 2013 14:42:22 -0700

> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
> <geert@linux-m68k.org> 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.

Re: [-next] openvswitch BUILD_BUG_ON failed

[Jesse Gross]

On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Thu, 29 Aug 2013 14:42:22 -0700
>
>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>> <geert@linux-m68k.org> 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.

Re: [-next] openvswitch BUILD_BUG_ON failed

[Geert Uytterhoeven]

On Fri, Aug 30, 2013 at 3:11 AM, Jesse Gross <jesse@nicira.com> wrote:
> On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
>> From: Jesse Gross <jesse@nicira.com>
>> Date: Thu, 29 Aug 2013 14:42:22 -0700
>>
>>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>>> <geert@linux-m68k.org> 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.

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.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [-next] openvswitch BUILD_BUG_ON failed

[Jesse Gross]

On Sat, Aug 31, 2013 at 5:11 AM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Fri, Aug 30, 2013 at 3:11 AM, Jesse Gross <jesse@nicira.com> wrote:
>> On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
>>> From: Jesse Gross <jesse@nicira.com>
>>> Date: Thu, 29 Aug 2013 14:42:22 -0700
>>>
>>>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>>>> <geert@linux-m68k.org> 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.

Re: [-next] openvswitch BUILD_BUG_ON failed

[Geert Uytterhoeven]

On Tue, Sep 3, 2013 at 11:44 PM, Jesse Gross <jesse@nicira.com> wrote:
> On Sat, Aug 31, 2013 at 5:11 AM, Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Fri, Aug 30, 2013 at 3:11 AM, Jesse Gross <jesse@nicira.com> wrote:
>>> On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
>>>> From: Jesse Gross <jesse@nicira.com>
>>>> Date: Thu, 29 Aug 2013 14:42:22 -0700
>>>>
>>>>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>>>>> <geert@linux-m68k.org> 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@linux-m68k.org

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

Re: [-next] openvswitch BUILD_BUG_ON failed

[Jesse Gross]

On Tue, Sep 3, 2013 at 11:55 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Sep 3, 2013 at 11:44 PM, Jesse Gross <jesse@nicira.com> wrote:
>> On Sat, Aug 31, 2013 at 5:11 AM, Geert Uytterhoeven
>> <geert@linux-m68k.org> wrote:
>>> On Fri, Aug 30, 2013 at 3:11 AM, Jesse Gross <jesse@nicira.com> wrote:
>>>> On Thu, Aug 29, 2013 at 3:10 PM, David Miller <davem@davemloft.net> wrote:
>>>>> From: Jesse Gross <jesse@nicira.com>
>>>>> Date: Thu, 29 Aug 2013 14:42:22 -0700
>>>>>
>>>>>> On Thu, Aug 29, 2013 at 2:21 PM, Geert Uytterhoeven
>>>>>> <geert@linux-m68k.org> 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.