[ovs-dev] [PATCH] datapath: do not add Geneve attributes if vport does not exist

[Jesse Gross]

On Tue, Jul 22, 2014 at 9:34 AM, Jesse Gross <jesse at nicira.com> wrote:
> On Mon, Jul 21, 2014 at 5:11 PM, Ansis Atteka <aatteka at nicira.com> wrote:
>> On Thu, Jul 17, 2014 at 4:08 PM, Jesse Gross <jesse at nicira.com> wrote:
>>> One thing that I worry about is that this has the possibility to
>>> change how the flow is reported - before the flow deletion it has
>>> Geneve options but immediately after the flow still exists but without
>>> options. OVS will likely deal with this but it doesn't seem like a
>>> great thing.
>> I talked with Pravin on how to solve this "flow changing while vport
>> gets added" issue and he seemed to prefer the idea where we simply
>> look into tun_opts_len to figure out whether to append Geneve
>> attributes.
>>
>> Basically, the new patch makes assumption that if tun_opts_len>0 then
>> that is Geneve port that could potentially have Geneve options. Also,
>> I remember you mentioning that it might be good to distinguish between
>> the case when there are no tunnel options at all and the case when
>> Geneve attributes contain an empty set. Patch v2 does not address
>> that. How important is that in your opinion?
>
> The biggest concern that I have is if there is a flow that matches on
> Geneve packets without any options. If we it changed to look at
> tun_opts_len only, when the upcall goes to userspace there would be no
> OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS. Userspace would generally like to
> just echo the key back, so that means that we would have a mask
> without a key since it needs to specify a mask to match.
>
> This is currently allowed but not really ideal - we have this already
> for some existing netlink attributes like the overall
> OVS_KEY_ATTR_TUNNEL but it's been a source of bugs and special casing.
> The other choice is that we force userspace to insert the key even if
> the kernel didn't provide it for Geneve flows but that's somewhat
> complicated.
>
> One other possible solution is to tuck a bit into the flow to
> differentiate between the cases of an empty option set and no options
> but I was somewhat hoping to avoid that.

Pravin, Andy, and I just talked about this and I think that we reached
a conclusion.

It seems like the best thing to do is to use a flag in the key that
indicates whether options were originally present in either the flow
(because userspace passed it down) or header (because it came from a
Geneve port). Essentially, this would act like the current port number
check but with less crashing.

So what we would do is:
 * Add a TUNNEL_OPTIONS_PRESENT flag to the existing list of
TUNNEL_CSUM, TUNNEL_KEY, etc.
 * Geneve ports would always set this when initializing flags in
geneve_rcv(), other tunnel ports never would.
 * When receiving a flow setup from userspace, we would set this flag
if there is a GENEVE_OPTIONS key attribute present.
 * When sending a flow from the kernel to userspace, we would change
the current check based on the port number to one based on this flag.

Does that sound reasonable to you?