[ovs-dev] [RFC PATCH kernel 03/10] openvswitch: IPv6 tunnel flows

[Jiri Benc]

On Mon, 1 Jun 2015 14:22:48 -0700, Jesse Gross wrote:
> On Thu, May 14, 2015 at 11:10 AM, Jiri Benc <jbenc at redhat.com> 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.

That was my original approach but it turned out not to be so great.

I would have to add a flag indicating whether the address is v4 or v6
(either a single bit, or a L2 protocol field). The problem is there's no
prerequisite on the underlying protocol for tunnel source and
destination address. The current code just expects all tunnels are IPv4.

This could be solved by treating the underlying protocol being unset as
IPv4 and not allowing wildcard matching on the 'underlying protocol'
field. Looked as hackish as the solution I chose but with more code. In
addition, having the addresses in an union is not an option for the
user space part and it seemed to be better to have this consistent.

Please let me know if you insist on adding an underlying protocol field
(or a bit flag; but if we're doing this, it's better to go the more
general way, even though it brings its own complications: we'll need to
bail out if the protocol is neither ipv4 nor ipv6) and putting the
addresses into an union, I'll rewrite the patchset.

> > @@ -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.

Okay, no problem, there's no padding between the fields.

> > 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.

This is how it's done in other parts of the kernel, just sum up all
attributes even if some mixtures of them are not possible. I can remove
the v4 fields, though.

Thanks for the review,

 Jiri

-- 
Jiri Benc