Re: [PATCH] openvswitch: Fix alignment of struct sw_flow_key.

On Thu, Sep 5, 2013 at 11:40 AM, David Miller [off-list ref] wrote:

From: Jesse Gross <redacted>
Date: Thu, 5 Sep 2013 11:36:19 -0700

quoted

On Thu, Sep 5, 2013 at 11:17 AM, David Miller [off-list ref] wrote:

quoted

From: Jesse Gross <redacted>
Date: Thu,  5 Sep 2013 10:41:27 -0700

quoted

-} __aligned(__alignof__(long));
+} __aligned(8); /* 8 byte alignment ensures this can be accessed as a long */

This kind of stuff drives me crazy.

If the issue is the type, therefore at least use an expression that
mentions the type explicitly.  And mention the actual type that
matters.  "long" isn't it.

'long' actually is the real type here.

When doing comparisons, this structure is being accessed as a byte
array in 'long' sized chunks, not by its members. Therefore, the
compiler's alignment does not necessarily correspond to anything for
this purpose. It could be a struct full of u16's and we would still
want to access it in chunks of 'long'.

To completely honest, I think the correct alignment should be
sizeof(long) because I know that 'long' is not always 8 bytes on all
architectures. However, you made the point before that this could
break the alignment of the 64-bit values on architectures where 'long'
is 32 bits wide, so 8 bytes is the generic solution.

Look at net/core/flow.c:flow_key_compare().

And then we annotate struct flowi with

} __attribute__((__aligned__(BITS_PER_LONG/8)));

Don't reinvent the wheel, either mimick how existing code does
this kind of thing or provide a justification for doing it differently
and update the existing cases to match and be consistent.

Sure, I'll send a new patch to use BITS_PER_LONG.