On Tue, Jun 2, 2015 at 8:37 AM, Jiri Benc <jb...@redhat.com> wrote:
> On Mon, 1 Jun 2015 14:22:48 -0700, Jesse Gross wrote:
>> On Thu, May 14, 2015 at 11:10 AM, Jiri Benc <jb...@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.
I guess my inclination is to use a bit (presumably we can find some
hole if it isn't too ugly) - the flow key expands at a never ending
rate, so it would be nice to at least not unnecessarily expand it if
we can avoid it.
I'm not too worried about being general purpose or matching with
userspace - it's an internal optimization so we can always change it
later.
I'm not really hung up on this if it is really horrible to do
otherwise but it's worth at least thinking about.
>> > 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.
I know although I think in the OVS case it is particularly extreme
because there is a large degree of overlap between different
protocols. At least currently we only try to compute the rough max.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
