On Mon, Jul 28, 2014 at 7:54 PM, Jesse Gross <je...@nicira.com> wrote:
> On Tue, Jul 22, 2014 at 9:34 AM, Jesse Gross <je...@nicira.com> wrote:
>> On Mon, Jul 21, 2014 at 5:11 PM, Ansis Atteka <aatt...@nicira.com> wrote:
>>> On Thu, Jul 17, 2014 at 4:08 PM, Jesse Gross <je...@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?
Yes, that sounds reasonable. I sent it out for review here -
http://openvswitch.org/pipermail/dev/2014-July/043220.html
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
