On 25 May 2017 at 13:52, Ben Pfaff <b...@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 <j...@ovn.org>
> Signed-off-by: Ben Pfaff <b...@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 <j...@ovn.org>
> 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?
<snip>
> diff --git a/ofproto/bundles.h b/ofproto/bundles.h
> index dd64700aa348..b63681708d3b 100644
> --- a/ofproto/bundles.h
> +++ b/ofproto/bundles.h
> @@ -1,7 +1,7 @@
> /*
> * Copyright (c) 2013, 2014 Alexandru Copot <alex.miha...@gmail.com>, with
> support from IXIA.
> * Copyright (c) 2013, 2014 Daniel Baluta <dbal...@ixiacom.com>
> - * Copyright (c) 2014, 2015, 2016 Nicira, Inc.
> + * Copyright (c) 2014, 2015, 2016, 2017 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -49,7 +49,7 @@ struct ofp_bundle_entry {
> };
> };
>
> -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).
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
