On Fri, Jun 26, 2015 at 12:47 PM, Ben Pfaff <b...@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 <je...@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.
> "git am" says:
>
> Applying: tunneling: Userspace datapath support for Geneve options.
> /home/blp/nicira/ovs/.git/rebase-apply/patch:89: trailing whitespace.
> gnh->critical = crit_opt ? 1 : 0;
> /home/blp/nicira/ovs/.git/rebase-apply/patch:127: trailing whitespace.
>
> warning: 2 lines add whitespace errors.
Fixed.
> In ovs_parse_tnl_push(), I would consider changing
> if (!ovs_scan_len(s, &n, "vni=0x%"SCNx32, &vni)) {
> to
> if (!ovs_scan_len(s, &n, "vni=%"SCNi32, &vni)) {
> to allow a handwritten vni to be expressed in decimal (this is not
> new code with this patch).
Thanks, that's a good idea.
> 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.
> 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
