On Fri, May 26, 2017 at 03:14:45PM -0700, Ben Pfaff wrote: > On Fri, May 26, 2017 at 02:04:23PM -0700, Joe Stringer wrote: > > On 25 May 2017 at 13:52, Ben Pfaff <[email protected]> wrote: > > > I know of two reasons to mark a structure as "packed". The first is > > > because the structure must match some defined interface and therefore > > > compiler-inserted padding between or after members would cause its layout > > > to diverge from that interface. This is not a problem in a structure that > > > follows the general alignment rules that are seen in ABIs for all the > > > architectures that OVS cares about: basically, that a struct member needs > > > to be aligned on a boundary that is a multiple of the member's size. > > > > > > The second reason is because instances of the struct tend to be at > > > misaligned addresses. > > > > > > struct eth_header and struct vlan_eth_header are normally aligned on > > > 16-bit boundaries (at least), and they contain only 16-bit members, so > > > there's no need to pack them. This commit removes the packed annotation. > > > > > > This commit also removes the packed annotation from struct llc_header. > > > Since that struct only contains 8-bit members, I don't know of any benefit > > > to packing it, period. > > > > > > This commit also removes a few more packed annotations that are much less > > > important. > > > > > > When these packed annotations were removed, it caused a few warnings > > > related to casts from 'uint8_t *' to more strictly aligned pointer types, > > > related to struct ovs_action_push_tnl. That's because that struct had a > > > trailing member used to store packet headers, that was declared as > > > a uint8_t[]. Before, when this was cast to 'struct eth_header *', there > > > was no change in alignment since eth_header was packed; now that > > > eth_header is not packed, the compiler considers it suspicious. This > > > commit avoids that problem by changing the member from uint8_t[] to > > > uint32_t[], which assures the compiler that it is properly aligned. > > > > > > CC: Joe Stringer <[email protected]> > > > Signed-off-by: Ben Pfaff <[email protected]> > > > --- > > > > Thanks for the patch, this fixes the primary issue I observed and > > looking at the "pahole" output on vswitchd compiled by clang, it looks > > mostly correct on my x86-64 dev box. Assuming you're not concerned by > > the below feedback: > > Acked-by: Joe Stringer <[email protected]> > > Thanks. > > > > datapath/linux/compat/include/linux/openvswitch.h | 2 +- > > > lib/packets.h | 9 +++------ > > > lib/stp.c | 8 +++----- > > > ofproto/bundles.h | 4 ++-- > > > 4 files changed, 9 insertions(+), 14 deletions(-) > > > > > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h > > > b/datapath/linux/compat/include/linux/openvswitch.h > > > index d22102e224a7..55ec6c13fbd2 100644 > > > --- a/datapath/linux/compat/include/linux/openvswitch.h > > > +++ b/datapath/linux/compat/include/linux/openvswitch.h > > > @@ -715,7 +715,7 @@ struct ovs_action_push_tnl { > > > uint32_t out_port; > > > uint32_t header_len; > > > uint32_t tnl_type; /* For logging. */ > > > - uint8_t header[TNL_PUSH_HEADER_SIZE]; > > > + uint32_t header[TNL_PUSH_HEADER_SIZE / 4]; > > > }; > > > #endif > > > > > > > Would you mind submitting this hunk upstream to net-next? > > It's inside #ifndef __KERNEL__. > > > > -enum OVS_PACKED_ENUM bundle_state { > > > +enum bundle_state { > > > BS_OPEN, > > > BS_CLOSED > > > }; > > > > This hunk actually appears to influence the layout of "struct > > ofp_bundle" which increases the size by 8 bytes on my system. That > > said, it still appears to be 2 cachelines long in both cases and I > > don't know whether we are specifically trying to tailor the size of > > this structure (eg, for performance reasons). > > I assumed that this was just cargo-culting from some other use of > OVS_PACKED_ENUM. I don't see how OpenFlow bundles are performance or > size sensitive.
I decided to apply this. I'm happy to revisit it if you have any concerns. Thanks again! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
