| Message ID | 1461221026-14900-1-git-send-email-johnson.li@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Wed, Apr 20, 2016 at 11:43 PM, Johnson.Li <johnson.li@intel.com> wrote: > From: Johnson Li <johnson.li@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@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. In addition, it appears that you are copying code from the Linux kernel. You cannot do this as the licenses are not compatible.
> -----Original Message----- > From: Jesse Gross [mailto:jesse@kernel.org] > Sent: Friday, April 22, 2016 12:44 AM > To: Li, Johnson <johnson.li@intel.com> > Cc: ovs dev <dev@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@intel.com> wrote: > > From: Johnson Li <johnson.li@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@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. > 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
On Thu, Apr 21, 2016 at 6:03 PM, Li, Johnson <johnson.li@intel.com> wrote: >> -----Original Message----- >> From: Jesse Gross [mailto:jesse@kernel.org] >> Sent: Friday, April 22, 2016 12:44 AM >> To: Li, Johnson <johnson.li@intel.com> >> Cc: ovs dev <dev@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@intel.com> wrote: >> > From: Johnson Li <johnson.li@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@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.
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index e398562..a7b5923 100644 --- a/lib/netdev-vport.c +++ b/lib/netdev-vport.c @@ -1286,6 +1286,7 @@ netdev_vxlan_pop_header(struct dp_packet *packet) struct flow_tnl *tnl = &md->tunnel; struct vxlanhdr *vxh; unsigned int hlen; + uint32_t vxh_flags; pkt_metadata_init_tnl(md); if (VXLAN_HLEN > dp_packet_l4_size(packet)) { @@ -1297,7 +1298,22 @@ netdev_vxlan_pop_header(struct dp_packet *packet) return EINVAL; } - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) || + /* VXLAN protocol header: + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * |G|R|R|R|I|R|R|C| Reserved | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * + * G = 1 Group Policy (VXLAN-GBP) + * I = 1 VXLAN Network Identifier (VNI) present + */ + vxh_flags = htonl(get_16aligned_be32(&vxh->vx_flags)); + if (vxh_flags & VXLAN_HF_GBP) { + tnl->gbp_id = ntohs(vxh_flags & VXLAN_GBP_ID_MASK); + vxh_flags &= (VXLAN_GBP_DONT_LEARN | VXLAN_GBP_POLICY_APPLIED); + tnl->gbp_flags = vxh_flags >> 16; + } + + if (!(get_16aligned_be32(&vxh->vx_flags) & htonl(VXLAN_HF_VNI)) || (get_16aligned_be32(&vxh->vx_vni) & htonl(0xff))) { VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x vni=%#x\n", ntohl(get_16aligned_be32(&vxh->vx_flags)), @@ -1312,6 +1328,27 @@ netdev_vxlan_pop_header(struct dp_packet *packet) return 0; } +static void +netdev_vxlan_build_gbp_header(struct vxlanhdr *vxh, + const struct flow *tnl_flow) +{ + /* VNI must be valid, so I bit should be set. */ + uint32_t vxh_flags = VXLAN_HF_VNI; + + /* G bit to indicates that the source TSI Group membership is being + * carried within the Group Policy ID field. */ + vxh_flags |= VXLAN_HF_GBP; + + /* Only D bit and A bit are valid in gbp_flags. Other bits which are + * set should be ignored. */ + vxh_flags |= (tnl_flow->tunnel.gbp_flags << 16) + & (VXLAN_GBP_DONT_LEARN | VXLAN_GBP_POLICY_APPLIED); + + vxh_flags |= ntohs(tnl_flow->tunnel.gbp_id); + + put_16aligned_be32(&vxh->vx_flags, htonl(vxh_flags)); +} + static int netdev_vxlan_build_header(const struct netdev *netdev, struct ovs_action_push_tnl *data, @@ -1328,7 +1365,11 @@ netdev_vxlan_build_header(const struct netdev *netdev, vxh = udp_build_header(tnl_cfg, tnl_flow, data, &hlen); - put_16aligned_be32(&vxh->vx_flags, htonl(VXLAN_FLAGS)); + if (tnl_cfg->exts & (1 << OVS_VXLAN_EXT_GBP)) { + netdev_vxlan_build_gbp_header(vxh, tnl_flow); + } else { + put_16aligned_be32(&vxh->vx_flags, htonl(VXLAN_HF_VNI)); + } put_16aligned_be32(&vxh->vx_vni, htonl(ntohll(tnl_flow->tunnel.tun_id) << 8)); ovs_mutex_unlock(&dev->mutex); diff --git a/lib/packets.h b/lib/packets.h index 8139a6b..23e964d 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -995,13 +995,49 @@ struct gre_base_hdr { #define GRE_FLAGS 0x00F8 #define GRE_VERSION 0x0007 -/* VXLAN protocol header */ +/* + * VXLAN Group Based Policy Extension (VXLAN_F_GBP): + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * |G|R|R|R|I|R|R|R|R|D|R|R|A|R|R|R| Group Policy ID | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | VXLAN Network Identifier (VNI) | Reserved | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * + * G = Group Policy ID present. + * + * D = Don't Learn bit. When set, this bit indicates that the egress + * VTEP MUST NOT learn the source address of the encapsulated frame. + * + * A = Indicates that the group policy has already been applied to + * this packet. Policies MUST NOT be applied by devices when the + * A bit is set. + * + * https://tools.ietf.org/html/draft-smith-vxlan-group-policy + */ +#define VXLAN_GBP_DONT_LEARN (1 << 22) +#define VXLAN_GBP_POLICY_APPLIED (1 << 19) +#define VXLAN_GBP_ID_MASK (0xFFFF) + +/* VXLAN protocol header: + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * |G|R|R|R|I|R|R|C| Reserved | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * | VXLAN Network Identifier (VNI) | Reserved | + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ + * + * G = 1 Group Policy (VXLAN-GBP) + * I = 1 VXLAN Network Identifier (VNI) present + * C = 1 Remote checksum offload (RCO) + */ struct vxlanhdr { ovs_16aligned_be32 vx_flags; ovs_16aligned_be32 vx_vni; }; -#define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */ +/* VXLAN header flags. */ +#define VXLAN_HF_RCO (1 << 21) +#define VXLAN_HF_VNI (1 << 27) +#define VXLAN_HF_GBP (1 << 31) void ipv6_format_addr(const struct in6_addr *addr, struct ds *); void ipv6_format_addr_bracket(const struct in6_addr *addr, struct ds *, diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at index a7909d3..5bd7096 100644 --- a/tests/tunnel-push-pop.at +++ b/tests/tunnel-push-pop.at @@ -12,6 +12,8 @@ AT_CHECK([ovs-vsctl add-port int-br t2 -- set Interface t2 type=vxlan \ options:remote_ip=1.1.2.93 options:out_key=flow options:csum=true ofport_request=4\ -- add-port int-br t4 -- set Interface t4 type=geneve \ options:remote_ip=flow options:key=123 ofport_request=5\ + -- add-port int-br t5 -- set Interface t5 type=vxlan \ + options:remote_ip=1.1.2.94 options:key=456 options:exts=gbp ofport_request=6\ ], [0]) AT_CHECK([ovs-appctl dpif/show], [0], [dnl @@ -25,6 +27,7 @@ dummy@ovs-dummy: hit:0 missed:0 t2 2/4789: (vxlan: key=123, remote_ip=1.1.2.92) t3 4/4789: (vxlan: csum=true, out_key=flow, remote_ip=1.1.2.93) t4 5/6081: (geneve: key=123, remote_ip=flow) + t5 6/4789: (vxlan: key=456, remote_ip=1.1.2.94) ]) dnl First setup dummy interface IP address, then add the route @@ -55,12 +58,14 @@ ffffffffffffaa55aa55000008060001080006040001aa55aa550000010102580000000000000101 dnl Check ARP Snoop AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b6,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.92,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b6,tha=00:00:00:00:00:00)']) AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b7,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.93,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b7,tha=00:00:00:00:00:00)']) +AT_CHECK([ovs-appctl netdev-dummy/receive br0 'recirc_id(0),in_port(100),eth(src=f8:bc:12:44:34:b8,dst=ff:ff:ff:ff:ff:ff),eth_type(0x0806),arp(sip=1.1.2.94,tip=1.1.2.88,op=2,sha=f8:bc:12:44:34:b8,tha=00:00:00:00:00:00)']) AT_CHECK([ovs-appctl tnl/neigh/show], [0], [dnl IP MAC Bridge ========================================================================== 1.1.2.92 f8:bc:12:44:34:b6 br0 1.1.2.93 f8:bc:12:44:34:b7 br0 +1.1.2.94 f8:bc:12:44:34:b8 br0 ]) AT_CHECK([ovs-appctl tnl/ports/show |sort], [0], [dnl @@ -102,6 +107,13 @@ AT_CHECK([tail -1 stdout], [0], [Datapath actions: tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:b7,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.93,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0xffff),vxlan(flags=0x8000000,vni=0x7c)),out_port(100)) ]) +dnl Check VXLAN-GBP tunnel push +AT_CHECK([ovs-ofctl add-flow int-br "actions=load:0x100->NXM_NX_TUN_GBP_ID[[]],output:6"]) +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], + [Datapath actions: tnl_push(tnl_port(4789),header(size=50,type=4,eth(dst=f8:bc:12:44:34:b8,src=aa:55:aa:55:00:00,dl_type=0x0800),ipv4(src=1.1.2.88,dst=1.1.2.94,proto=17,tos=0,ttl=64,frag=0x4000),udp(src=0,dst=4789,csum=0x0),vxlan(flags=0x88000100,vni=0x1c8)),out_port(100)) +]) + dnl Check GRE tunnel push AT_CHECK([ovs-ofctl add-flow int-br action=3]) AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(2),eth_type(0x0800),ipv4(src=1.1.3.88,dst=1.1.3.112,proto=47,tos=0,ttl=64,frag=no)'], [0], [stdout])