[ovs-dev] [PATCH] tunneling: Userspace datapath support for Geneve options.

[Ben Pfaff]

On Fri, Jun 26, 2015 at 01:14:17PM -0700, Jesse Gross wrote:
> On Fri, Jun 26, 2015 at 12:47 PM, Ben Pfaff <blp at nicira.com> wrote:
> > On Thu, Jun 25, 2015 at 11:22:00AM -0700, Jesse Gross wrote:
> >> Currently the userspace datapath only supports Geneve in a
> >> basic mode - without options - since the rest of userspace
> >> previously didn't support options either. This enables the
> >> userspace datapath to send and receive options as well.
> >>
> >> The receive path for extracting the tunnel options isn't entirely
> >> optimal because it does a lookup on the options on a per-packet
> >> basis, rather than per-flow like the kernel does. This is not
> >> as straightforward to do in the userspace datapath since there
> >> is no translation step between packet formats used in packet vs.
> >> flow lookup. This can be optimized in the future and in the
> >> meantime option support is still useful for testing and simulation.
> >>
> >> Signed-off-by: Jesse Gross <jesse at nicira.com>
> >
> > That was fast!
> 
> Believe it or not, I didn't actually write it within 10 minutes of
> pushing the other patches :)
> 
> > This gives me a repeatable test failure (on i386), log attached.
> 
> This is due to the userspace tunneling patch that I applied yesterday
> afternoon, after this patch was sent out. I fixed it in my rebased
> version.

OK, great.

> > Also in ovs_parse_tnl_push(), it looks like the code now expects
> > 'geneve()' to always end with a doubled ), but I think that it should
> > only do that if there are options.
> 
> This isn't new - before we used to scan for double parentheses as part
> of the VNI, it's just broken out now. The second one is closing the
> tunnel block (which is probably not great layering but that's how the
> code is setup now). We have tests for both option and non-option
> variants as well.

OK.

> > The {} syntax looks a little odd nested inside so many (), did you
> > consider using something like
> >         geneve(crit,vni=0x1c7,option(class=0xffff,type=0x80,len=4,0xa))
> > and then just allowing multiple "option"s directly inside geneve()?
> 
> I guess I'm a little hesitant to do this because it currently matches
> the kernel actions, which is nice from a consistency and code reuse
> perspective. In turn, the kernel output is closely related to the
> netlink formatting, which is nice for debugging if things are somehow
> mangled.

OK.

Acked-by: Ben Pfaff <blp at nicira.com>