[PATCH] openvswitch: fix sw_flow_key alignment

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 [off-list ref]

Signed-off-by: Andy Zhou <redacted>
---
 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

On Fri, Aug 30, 2013 at 10:32 AM, Andy Zhou [off-list ref] wrote:

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

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.

From: Jesse Gross <redacted>
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.

On Fri, Aug 30, 2013 at 11:42 AM, David Miller [off-list ref] wrote:

From: Jesse Gross <redacted>
Date: Fri, 30 Aug 2013 11:22:11 -0700

quoted

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.

From: Jesse Gross <redacted>
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.

On Fri, Aug 30, 2013 at 9:16 PM, David Miller [off-list ref] wrote:

From: Jesse Gross <redacted>
Date: Fri, 30 Aug 2013 16:20:25 -0700

quoted

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.

On Tue, Sep 3, 2013 at 3:06 PM, Jesse Gross [off-list ref] wrote:

On Fri, Aug 30, 2013 at 9:16 PM, David Miller [off-list ref] wrote:

quoted

From: Jesse Gross <redacted>
Date: Fri, 30 Aug 2013 16:20:25 -0700

quoted

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.