[ovs-dev] [PATCH] packets: Remove unnecessary "packed" annotations.

Ben Pfaff blp at ovn.org
Fri May 26 22:14:45 UTC 2017


On Fri, May 26, 2017 at 02:04:23PM -0700, Joe Stringer wrote:
> On 25 May 2017 at 13:52, Ben Pfaff <blp at ovn.org> 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 <joe at ovn.org>
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> 
> 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 <joe at ovn.org>

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.


More information about the dev mailing list