[ovs-dev] [PATCH v3] Add VxLAN-GBP support for user space data path

[Jesse Gross]

On Thu, Apr 21, 2016 at 6:03 PM, Li, Johnson <johnson.li at intel.com> wrote:
>> -----Original Message-----
>> From: Jesse Gross [mailto:jesse at kernel.org]
>> Sent: Friday, April 22, 2016 12:44 AM
>> To: Li, Johnson <johnson.li at intel.com>
>> Cc: ovs dev <dev at openvswitch.org>
>> Subject: Re: [ovs-dev] [PATCH v3] Add VxLAN-GBP support for user space
>> data path
>>
>> On Wed, Apr 20, 2016 at 11:43 PM, Johnson.Li <johnson.li at intel.com> wrote:
>> > From: Johnson Li <johnson.li at intel.com>
>> >
>> > In user space, only standard VxLAN was support. This patch will add
>> > the VxLAN-GBP support for the user space data path.
>> >
>> > How to use:
>> > 1> Create VxLAN port with GBP extension
>> >   $ovs-vsctl add-port br-int vxlan0 -- set interface vxlan0 \
>> >            type=vxlan options:dst_port=4789 \
>> >            options:remote_ip=192.168.60.22 \
>> >            options:key=1000 options:exts=gbp
>> > 2> Add flow for transmitting
>> >   $ovs-ofctl add-flow br-int "table=0, priority=260, \
>> >              in_port=LOCAL actions=load:0x100->NXM_NX_TUN_GBP_ID[], \
>> >              output:1"
>> > 3> Add flow for receiving
>> >   $ovs-ofctl add-flow br-int "table=0, priority=260, \
>> >              in_port=1,tun_gbp_id=0x100 actions=output:LOCAL"
>> >
>> > Check data path flow rules:
>> > $ovs-appctl dpif/dump-flows br-int
>> >   recirc_id(0),in_port(1),eth_type(0x0800),ipv4(tos=0/0x3,frag=no),
>> >   packets:0, bytes:0, used:never, actions:tnl_push(tnl_port(2),
>> >   header(size=50,type=4,eth(dst=90:e2:ba:48:7f:a4,src=90:e2:ba:48:7e:1c,
>> >   dl_type=0x0800),ipv4(src=192.168.60.21,dst=192.168.60.22,proto=17,
>> >   tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),
>> >   vxlan(flags=0x88000100,vni=0x3e8)),out_port(3))
>> >   tunnel(tun_id=0x3e8,src=192.168.60.22,dst=192.168.60.21,
>> >   vxlan(gbp(id=256)),flags(-df-csum+key)),skb_mark(0),recirc_id(0),
>> >   in_port(2),eth(dst=ae:1b:ed:1e:e3:4e),eth_type(0x0800),
>> >   ipv4(dst=172.168.60.21,proto=1/0x10,frag=no), packets:0, bytes:0,
>> >   used:never, actions:1
>> >
>> > ---
>> > Change Log:
>> >   v3: Change Macro definition, add more comments, add unit test.
>> >   v2: Set Each enabled bit for the VxLAN-GBP.
>> >
>> > Signed-off-by: Johnson Li <johnson.li at intel.com>
>>
>> Please do not submit a new version of a patch without addressing the
>> existing comments. I have asked you several times to not interpret bits from
>> an extension without checking whether that extension has explicitly been
>> enabled.
>>
> The code in the kernel data path checks the vxlan_sock.flags:
> (vs->flags & VXLAN_F_GBP).
> It could do this because kernel space implementation has the basis of UDP socket.
> But for the userspace, no such socket exists,  and the codes are in tunnel's pop_header
> Callback, no other flags is set to indicate that the tunnel is GBP is enable.
> So from my understanding, in the user space, we must trust the flags in the packets.
> And from my understanding, G bit is only used in the GBP from the drafts I have read,
> Other drafts keep this bit as Reserved, so it must set 0.
> The codes are in the tunnel's pop_header callback, the only parameter is
> struct dp_packet *packet
> we cannot get additional flags from this input.
> That why I don't check additional flags.

It is true that the code is structured differently between OVS and Linux.

However, that does not mean that you can ignore correctness simply
because it isn't convenient. If you need to restructure things in
order for your changes to work, then you must do that. There are many,
many changes in both OVS and Linux that require this type of
infrastructure work before patches can go in. In neither case are
patches applied before this is done.

>> In addition, it appears that you are copying code from the Linux kernel. You
>> cannot do this as the licenses are not compatible.
> No codes are copied from kernel, but I copied the comment which is VxLAN-GBP header
> Definition from the vxlan.h

Comments are copyrighted too. Please do not copy anything from the
Linux kernel into OVS.

In addition, the way that I could tell that this is from Linux you
copied references to things that only exist in Linux and don't make
sense in the context of OVS.