| Message ID | 1377883940-8995-1-git-send-email-azhou@nicira.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)