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? _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
