On Mon, Jun 01, 2015 at 02:22:48PM -0700, Jesse Gross wrote: > On Thu, May 14, 2015 at 11:10 AM, Jiri Benc <[email protected]> wrote: > > diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h > > index 2af6ffbf2f2e..78e96a120120 100644 > > --- a/net/openvswitch/flow.h > > +++ b/net/openvswitch/flow.h > > -struct ovs_key_ipv4_tunnel { > > +struct ovs_key_ip_tunnel { > > __be64 tun_id; > > __be32 ipv4_src; > > __be32 ipv4_dst; > > + struct in6_addr ipv6_src; > > + struct in6_addr ipv6_dst; > > The v6 addresses should really be a union with the v4 addresses since > this is going into the flow key that is used on a per-packet basis.
Agreed, but using union would break patch 7 which uses ipv4_dst to find out if the tunnel is v4 or v6, for instance. So, moving to a union would require also storing the addr family. > > @@ -79,6 +81,9 @@ static inline void __ovs_flow_tun_info_init(struct > > ovs_tunnel_info *tun_info, > > tun_info->tunnel.tun_id = tun_id; > > tun_info->tunnel.ipv4_src = saddr; > > tun_info->tunnel.ipv4_dst = daddr; > > + memset(&tun_info->tunnel.ipv6_src, 0, > > + offsetof(struct ovs_key_ip_tunnel, tun_flags) - > > + offsetof(struct ovs_key_ip_tunnel, ipv6_src)); > > I would just memset() both the IPv6 address fields individually. > Trying to be clever with the struct layout seems a little risky. +1 as it improves readability, reduces the risk of an error and gcc is smart enough to optimize that code. fbl > > diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c > > index 624e41c4267f..890a6cf4ec67 100644 > > --- a/net/openvswitch/flow_netlink.c > > +++ b/net/openvswitch/flow_netlink.c > > @@ -273,7 +273,10 @@ size_t ovs_tun_key_attr_size(void) > > * OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS and covered by it. > > */ > > + nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_SRC */ > > - + nla_total_size(2); /* OVS_TUNNEL_KEY_ATTR_TP_DST */ > > + + nla_total_size(2) /* OVS_TUNNEL_KEY_ATTR_TP_DST */ > > + + nla_total_size(16) /* OVS_TUNNEL_KEY_ATTR_IPV6_SRC */ > > + + nla_total_size(16) /* OVS_TUNNEL_KEY_ATTR_IPV6_DST */ > > This size calculation should probably also not double count v4 and v6 > addresses, although this is more just general hygiene than > performance. > _______________________________________________ > dev mailing list > [email protected] > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
