| Message ID | 20170525205206.1700-1-blp@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
On 25 May 2017 at 13:52, Ben Pfaff <blp@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@ovn.org> > Signed-off-by: Ben Pfaff <blp@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@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.mihai.c@gmail.com>, with support from IXIA. > * Copyright (c) 2013, 2014 Daniel Baluta <dbaluta@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).
On Fri, May 26, 2017 at 02:04:23PM -0700, Joe Stringer wrote: > On 25 May 2017 at 13:52, Ben Pfaff <blp@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@ovn.org> > > Signed-off-by: Ben Pfaff <blp@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@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.
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 <blp@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@ovn.org> > > > Signed-off-by: Ben Pfaff <blp@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@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. I decided to apply this. I'm happy to revisit it if you have any concerns. Thanks again!
On 26/05/2017 15:14, "Ben Pfaff" <blp@ovn.org> wrote: On Fri, May 26, 2017 at 02:04:23PM -0700, Joe Stringer wrote: > On 25 May 2017 at 13:52, Ben Pfaff <blp@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@ovn.org> > > Signed-off-by: Ben Pfaff <blp@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@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__. Ah, requested that by reflex due to the file. Never mind. > > -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. Agreed.
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 diff --git a/lib/packets.h b/lib/packets.h index 7dbf6dd0b415..65ab8e8b5699 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -395,12 +395,11 @@ static inline bool eth_type_vlan(ovs_be16 eth_type) #define ETH_TOTAL_MIN (ETH_HEADER_LEN + ETH_PAYLOAD_MIN) #define ETH_TOTAL_MAX (ETH_HEADER_LEN + ETH_PAYLOAD_MAX) #define ETH_VLAN_TOTAL_MAX (ETH_HEADER_LEN + VLAN_HEADER_LEN + ETH_PAYLOAD_MAX) -OVS_PACKED( struct eth_header { struct eth_addr eth_dst; struct eth_addr eth_src; ovs_be16 eth_type; -}); +}; BUILD_ASSERT_DECL(ETH_HEADER_LEN == sizeof(struct eth_header)); void push_eth(struct dp_packet *packet, const struct eth_addr *dst, @@ -412,12 +411,11 @@ void pop_eth(struct dp_packet *packet); #define LLC_CNTL_SNAP 3 #define LLC_HEADER_LEN 3 -OVS_PACKED( struct llc_header { uint8_t llc_dsap; uint8_t llc_ssap; uint8_t llc_cntl; -}); +}; BUILD_ASSERT_DECL(LLC_HEADER_LEN == sizeof(struct llc_header)); /* LLC field values used for STP frames. */ @@ -484,14 +482,13 @@ struct vlan_header { BUILD_ASSERT_DECL(VLAN_HEADER_LEN == sizeof(struct vlan_header)); #define VLAN_ETH_HEADER_LEN (ETH_HEADER_LEN + VLAN_HEADER_LEN) -OVS_PACKED( struct vlan_eth_header { struct eth_addr veth_dst; struct eth_addr veth_src; ovs_be16 veth_type; /* Always htons(ETH_TYPE_VLAN). */ ovs_be16 veth_tci; /* Lowest 12 bits are VLAN ID. */ ovs_be16 veth_next_type; -}); +}; BUILD_ASSERT_DECL(VLAN_ETH_HEADER_LEN == sizeof(struct vlan_eth_header)); /* MPLS related definitions */ diff --git a/lib/stp.c b/lib/stp.c index 952d3ce434c3..d83d42521498 100644 --- a/lib/stp.c +++ b/lib/stp.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 2016 Nicira, Inc. + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013, 2014, 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. @@ -45,12 +45,11 @@ static struct vlog_rate_limit stp_rl = VLOG_RATE_LIMIT_INIT(60, 60); #define STP_TYPE_CONFIG 0x00 #define STP_TYPE_TCN 0x80 -OVS_PACKED( struct stp_bpdu_header { ovs_be16 protocol_id; /* STP_PROTOCOL_ID. */ uint8_t protocol_version; /* STP_PROTOCOL_VERSION. */ uint8_t bpdu_type; /* One of STP_TYPE_*. */ -}); +}; BUILD_ASSERT_DECL(sizeof(struct stp_bpdu_header) == 4); enum stp_config_bpdu_flags { @@ -73,10 +72,9 @@ struct stp_config_bpdu { }); BUILD_ASSERT_DECL(sizeof(struct stp_config_bpdu) == 35); -OVS_PACKED( struct stp_tcn_bpdu { struct stp_bpdu_header header; /* Type STP_TYPE_TCN. */ -}); +}; BUILD_ASSERT_DECL(sizeof(struct stp_tcn_bpdu) == 4); struct stp_timer { 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.mihai.c@gmail.com>, with support from IXIA. * Copyright (c) 2013, 2014 Daniel Baluta <dbaluta@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 };
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@ovn.org> Signed-off-by: Ben Pfaff <blp@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(-)