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-next" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
