On Fri, Jun 26, 2015 at 2:16 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Jun 26, 2015 at 01:14:17PM -0700, Jesse Gross wrote:
>> 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.
>
> 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 <b...@nicira.com>
Thanks, applied to master.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
