[PATCH] openvswitch: fix sw_flow_key alignment

[PATCH] openvswitch: fix sw_flow_key alignment

[Andy Zhou]

sw_flow_key alignment was declared as " __aligned(__alignof__(long))"
However, this breaks on m68k architecture where long is 32 bit in size
but 16 bit aligned by default. Use __aligned(sizeof(long) instead.

Reported by: Fengguang Wu <fengguan.wu@intel.com>

Signed-off-by: Andy Zhou <azhou@nicira.com>
---
 net/openvswitch/flow.c |    4 +---
 net/openvswitch/flow.h |    2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index ad1aeeb..fe7524c4 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -1009,9 +1009,6 @@ static u32 ovs_flow_hash(const struct sw_flow_key *key, int key_start,
 	u32 *hash_key = (u32 *)((u8 *)key + key_start);
 	int hash_u32s = (key_end - key_start) >> 2;
 
-	/* Make sure number of hash bytes are multiple of u32. */
-	BUILD_BUG_ON(sizeof(long) % sizeof(u32));
-
 	return jhash2(hash_key, hash_u32s, 0);
 }
 
@@ -1982,6 +1979,7 @@ nla_put_failure:
 int ovs_flow_init(void)
 {
 	BUILD_BUG_ON(sizeof(struct sw_flow_key) % sizeof(long));
+	BUILD_BUG_ON(sizeof(struct sw_flow_key) % __alignof__(long));
 
 	flow_cache = kmem_cache_create("sw_flow", sizeof(struct sw_flow), 0,
 					0, NULL);
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index b65f885..202c4c4 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -125,7 +125,7 @@ struct sw_flow_key {
 			} nd;
 		} ipv6;
 	};
-} __aligned(__alignof__(long));
+} __aligned(sizeof(long));
 
 struct sw_flow {
 	struct rcu_head rcu;
-- 
1.7.9.5

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[Jesse Gross]

On Fri, Aug 30, 2013 at 10:32 AM, Andy Zhou <azhou@nicira.com> wrote:
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index ad1aeeb..fe7524c4 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
>  int ovs_flow_init(void)
>  {
>         BUILD_BUG_ON(sizeof(struct sw_flow_key) % sizeof(long));
> +       BUILD_BUG_ON(sizeof(struct sw_flow_key) % __alignof__(long));

Should this be checking alignof struct sw_flow_key instead of the size?

>         flow_cache = kmem_cache_create("sw_flow", sizeof(struct sw_flow), 0,
>                                         0, NULL);
> diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
> index b65f885..202c4c4 100644
> --- a/net/openvswitch/flow.h
> +++ b/net/openvswitch/flow.h
> @@ -125,7 +125,7 @@ struct sw_flow_key {
>                         } nd;
>                 } ipv6;
>         };
> -} __aligned(__alignof__(long));
> +} __aligned(sizeof(long));

This is going to result in the alignment issue discussed yesterday
where on 32-bit machines the 64 bit members potentially become
misaligned. The suggestion that Geert made was to just drop this
entirely and rely on the natural alignment from these values.

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Fri, 30 Aug 2013 11:22:11 -0700

> The suggestion that Geert made was to just drop this entirely and
> rely on the natural alignment from these values.

Indeed, Geert's patch was 1,000 times superior to this one.

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[Jesse Gross]

On Fri, Aug 30, 2013 at 11:42 AM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Fri, 30 Aug 2013 11:22:11 -0700
>
>> The suggestion that Geert made was to just drop this entirely and
>> rely on the natural alignment from these values.
>
> Indeed, Geert's patch was 1,000 times superior to this one.

I looked through the struct definition and I think that the idea of
manually padding as Geert did in his patch will be difficult to
maintain over time (and actually there are a few that he missed) since
there are a number of different structs/unions contained in there.
Dropping the alignment specifier completely still has the same
potential problem on architectures where the size and alignment of
long are not the same.

Maybe the easiest thing to do at this point is just to always align it
to 8 bytes.

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[David Miller]

From: Jesse Gross <jesse@nicira.com>
Date: Fri, 30 Aug 2013 16:20:25 -0700

> I looked through the struct definition and I think that the idea of
> manually padding as Geert did in his patch will be difficult to
> maintain over time (and actually there are a few that he missed) since
> there are a number of different structs/unions contained in there.

You have to be mindful of the gaps and wasted space for performance
reasons anyways.

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[Jesse Gross]

On Fri, Aug 30, 2013 at 9:16 PM, David Miller <davem@davemloft.net> wrote:
> From: Jesse Gross <jesse@nicira.com>
> Date: Fri, 30 Aug 2013 16:20:25 -0700
>
>> I looked through the struct definition and I think that the idea of
>> manually padding as Geert did in his patch will be difficult to
>> maintain over time (and actually there are a few that he missed) since
>> there are a number of different structs/unions contained in there.
>
> You have to be mindful of the gaps and wasted space for performance
> reasons anyways.

Yes, although the approaches for performance and correctness are not
necessarily the same. For example, we're taking about potentially
packing the struct, in which case we would still have the same
alignment needs even without any gaps. If the correctness is clearly
handled (through an explicit align and build assert) then it's easier
to pack/rearrange/whatever for performance since the whole thing isn't
fragile.

Re: [PATCH] openvswitch: fix sw_flow_key alignment

[Jesse Gross]

On Tue, Sep 3, 2013 at 3:06 PM, Jesse Gross <jesse@nicira.com> wrote:
> On Fri, Aug 30, 2013 at 9:16 PM, David Miller <davem@davemloft.net> wrote:
>> From: Jesse Gross <jesse@nicira.com>
>> Date: Fri, 30 Aug 2013 16:20:25 -0700
>>
>>> I looked through the struct definition and I think that the idea of
>>> manually padding as Geert did in his patch will be difficult to
>>> maintain over time (and actually there are a few that he missed) since
>>> there are a number of different structs/unions contained in there.
>>
>> You have to be mindful of the gaps and wasted space for performance
>> reasons anyways.
>
> Yes, although the approaches for performance and correctness are not
> necessarily the same. For example, we're taking about potentially
> packing the struct, in which case we would still have the same
> alignment needs even without any gaps. If the correctness is clearly
> handled (through an explicit align and build assert) then it's easier
> to pack/rearrange/whatever for performance since the whole thing isn't
> fragile.

I'd like to get this build failure fixed soon, so I'll just send out a
minimal fix that seems best to me in a couple of minutes.