Re: [PATCH 6/6] openvswitch: Support VXLAN Group Policy extension

On Mon, Jan 12, 2015 at 5:02 PM, Thomas Graf [off-list ref] wrote:

On 01/12/15 at 01:54pm, Jesse Gross wrote:

quoted

On Mon, Jan 12, 2015 at 4:26 AM, Thomas Graf [off-list ref] wrote:

quoted

+       if (!is_mask)
+               SW_FLOW_KEY_PUT(match, tun_opts_len, sizeof(opts), false);
+       else
+               SW_FLOW_KEY_PUT(match, tun_opts_len, 0xff, true);

Have you thought carefully about how the masking model work as other
extensions are potentially added? This was a little tricky with Geneve
because I wanted to be able to match on both "no options present" as
well as wildcard all options. The other interesting thing is how you
serialize them back correctly to userspace, which was the genesis of
the TUNNEL_OPTIONS_PRESENT flag.

My guess is that this may basically work fine now that there is only
one extension present but it is important to think about how it might
work with multiple independent extensions in the future. (I haven't
thought about it, I'm just asking.)

I currently don't see a reason why adding another extension would be
a problem. It should work like Geneve options except that the order
of the options in the flow is given (struct vxlan_opts).

Matching on "no options present" is supported in the datapath by
via the TUNNEL_VXLAN_OPT flag although there is no way in user space
to express this intent yet. I haven't come across a need to support it
yet.

Since the Netlink API is decoupled from the datapath flow
representation, all of this can be changed if needed without breaking
the Netlink ABI.

OK, it seems fine for now.

I agree that "not present" is probably less interesting for VXLAN than
Geneve given the fixed sized header. It would seem to only have
benefit in the event that the port configuration is decoupled from
flow processing in userspace.

quoted

If you set Geneve options and output to a VXLAN port (or vice versa),
you will get garbage, right? Is there any way that we can sanity check
that?

What about if we only apply tun_info->options on Geneve if
TUNNEL_GENEVE_OPT is set and vice versa?

That seems nice and simple to me.