| Message ID | 1461137993-7521-1-git-send-email-johnson.li@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Apr 20, 2016 at 12:39 AM, 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 Can you please turn these into actual unit tests? Otherwise nobody will ever execute them. > Change Log: > v2: Set Each enabled bit for the VxLAN-GBP. Please put this below three dashes so it won't be included in the final commit message. > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c > index e398562..0ac449e 100644 > --- a/lib/netdev-vport.c > +++ b/lib/netdev-vport.c > @@ -1297,10 +1298,18 @@ netdev_vxlan_pop_header(struct dp_packet *packet) > return EINVAL; > } > > - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) || > - (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)), > + vxh_flags = get_16aligned_be32(&vxh->vx_flags); > + if (vxh_flags & htonl(VXLAN_HF_GBP)) { This check needs to be conditional on GBP actually being enabled in some form. Otherwise, you could misinterpret a different extension that uses overlapping bit combinations. > + tnl->gbp_id = htons(ntohl(vxh_flags) & VXLAN_GBP_ID_MASK); You can apply the byteswap to the mask instead of doing it twice on the flags - AND operations can be done in any byte order. > ovs_mutex_unlock(&dev->mutex); > diff --git a/lib/packets.h b/lib/packets.h > index 8139a6b..c78b053 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -1003,6 +1003,11 @@ struct vxlanhdr { > > #define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */ This is itself representing a flag in the VXLAN header, can you rename it to be consistent with the other header flags? > +#define VXLAN_HF_GBP 0x80000000 /* Group Based Policy, BIT(31) */ > +#define VXLAN_GBP_DONT_LEARN 0x400000 /* Don't Learn, (BIT(6) << 16) */ > +#define VXLAN_GBP_POLICY_APPLIED 0x80000 /* Policy Applied, (BIT(3) << 16) */ I think you can just represent this as bit shifts directly - no need to keep it in the comments.
> -----Original Message----- > From: Jesse Gross [mailto:jesse@kernel.org] > Sent: Thursday, April 21, 2016 4:23 AM > To: Li, Johnson <johnson.li@intel.com> > Cc: ovs dev <dev@openvswitch.org> > Subject: Re: [ovs-dev] [PATCH v2] Add VxLAN-GBP support for user space > data path > > On Wed, Apr 20, 2016 at 12:39 AM, 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. > > Can you please turn these into actual unit tests? Otherwise nobody will ever > execute them. Ok, I will try to add test cases. > > > Change Log: > > v2: Set Each enabled bit for the VxLAN-GBP. > > Please put this below three dashes so it won't be included in the final commit > message. > Ok, I will. > > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index > > e398562..0ac449e 100644 > > --- a/lib/netdev-vport.c > > +++ b/lib/netdev-vport.c > > @@ -1297,10 +1298,18 @@ netdev_vxlan_pop_header(struct dp_packet > *packet) > > return EINVAL; > > } > > > > - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) || > > - (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)), > > + vxh_flags = get_16aligned_be32(&vxh->vx_flags); > > + if (vxh_flags & htonl(VXLAN_HF_GBP)) { > > This check needs to be conditional on GBP actually being enabled in some > form. Otherwise, you could misinterpret a different extension that uses > overlapping bit combinations. From the spec at https://tools.ietf.org/html/draft-smith-vxlan-group-policy-01#section-2.1 it says " Bit 0 of the initial word is defined as the G (Group Based Policy Extension) bit". If the G bit is set, it indicates that the GBP ID is carried. Then I can conclude that if The header is VxLAN header(Special UDP destination port is detected), and the bit 0 Is set, then the header should be VxLAN-GBP header. > > > + tnl->gbp_id = htons(ntohl(vxh_flags) & VXLAN_GBP_ID_MASK); > > You can apply the byteswap to the mask instead of doing it twice on the flags > - AND operations can be done in any byte order. > > > ovs_mutex_unlock(&dev->mutex); > > diff --git a/lib/packets.h b/lib/packets.h index 8139a6b..c78b053 > > 100644 > > --- a/lib/packets.h > > +++ b/lib/packets.h > > @@ -1003,6 +1003,11 @@ struct vxlanhdr { > > > > #define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required > > value. */ > > This is itself representing a flag in the VXLAN header, can you rename it to be > consistent with the other header flags? This just indicates that the VNI is set and valid for the VxLAN [-GB P|-GPE] header. > > > +#define VXLAN_HF_GBP 0x80000000 /* Group Based Policy, BIT(31) */ > > +#define VXLAN_GBP_DONT_LEARN 0x400000 /* Don't Learn, (BIT(6) << > 16) > > +*/ #define VXLAN_GBP_POLICY_APPLIED 0x80000 /* Policy Applied, > > +(BIT(3) << 16) */ > > I think you can just represent this as bit shifts directly - no need to keep it in > the comments. Just followed the origin implementation. I can follow you advice and copy the definition From the implementation for Kernel data path.
On Wed, Apr 20, 2016 at 6:10 PM, Li, Johnson <johnson.li@intel.com> wrote: >> -----Original Message----- >> From: Jesse Gross [mailto:jesse@kernel.org] >> Sent: Thursday, April 21, 2016 4:23 AM >> To: Li, Johnson <johnson.li@intel.com> >> Cc: ovs dev <dev@openvswitch.org> >> Subject: Re: [ovs-dev] [PATCH v2] Add VxLAN-GBP support for user space >> data path >> > diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index >> > e398562..0ac449e 100644 >> > --- a/lib/netdev-vport.c >> > +++ b/lib/netdev-vport.c >> > @@ -1297,10 +1298,18 @@ netdev_vxlan_pop_header(struct dp_packet >> *packet) >> > return EINVAL; >> > } >> > >> > - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) || >> > - (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)), >> > + vxh_flags = get_16aligned_be32(&vxh->vx_flags); >> > + if (vxh_flags & htonl(VXLAN_HF_GBP)) { >> >> This check needs to be conditional on GBP actually being enabled in some >> form. Otherwise, you could misinterpret a different extension that uses >> overlapping bit combinations. > From the spec at > https://tools.ietf.org/html/draft-smith-vxlan-group-policy-01#section-2.1 > it says " Bit 0 of the initial word is defined as the G (Group Based Policy Extension) bit". > If the G bit is set, it indicates that the GBP ID is carried. Then I can conclude that if > The header is VxLAN header(Special UDP destination port is detected), and the bit 0 > Is set, then the header should be VxLAN-GBP header. This draft is an extension to VXLAN and so you can only follow it if you know that that extension is being used. There are other proposals with conflicting definitions. >> > + tnl->gbp_id = htons(ntohl(vxh_flags) & VXLAN_GBP_ID_MASK); >> >> You can apply the byteswap to the mask instead of doing it twice on the flags >> - AND operations can be done in any byte order. >> >> > ovs_mutex_unlock(&dev->mutex); >> > diff --git a/lib/packets.h b/lib/packets.h index 8139a6b..c78b053 >> > 100644 >> > --- a/lib/packets.h >> > +++ b/lib/packets.h >> > @@ -1003,6 +1003,11 @@ struct vxlanhdr { >> > >> > #define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required >> > value. */ >> >> This is itself representing a flag in the VXLAN header, can you rename it to be >> consistent with the other header flags? > This just indicates that the VNI is set and valid for the VxLAN [-GB P|-GPE] header. Yes, please define it that way.
diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c index e398562..0ac449e 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,10 +1298,18 @@ netdev_vxlan_pop_header(struct dp_packet *packet) return EINVAL; } - if (get_16aligned_be32(&vxh->vx_flags) != htonl(VXLAN_FLAGS) || - (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)), + vxh_flags = get_16aligned_be32(&vxh->vx_flags); + if (vxh_flags & htonl(VXLAN_HF_GBP)) { + tnl->gbp_id = htons(ntohl(vxh_flags) & VXLAN_GBP_ID_MASK); + vxh_flags &= (VXLAN_GBP_DONT_LEARN | VXLAN_GBP_POLICY_APPLIED); + tnl->gbp_flags = vxh_flags >> 16; + } else if (vxh_flags != htonl(VXLAN_FLAGS)) { + VLOG_WARN_RL(&err_rl, "invalid vxlan flags=%#x\n", + ntohl(get_16aligned_be32(&vxh->vx_flags))); + return EINVAL; + } + if (get_16aligned_be32(&vxh->vx_vni) & htonl(0xff)) { + VLOG_WARN_RL(&err_rl, "invalid vxlan vni=%#x\n", ntohl(get_16aligned_be32(&vxh->vx_vni))); return EINVAL; } @@ -1312,6 +1321,26 @@ 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) +{ + uint32_t vxh_flags = VXLAN_FLAGS; + + /* 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 is valid in gbp_flags. Other bit which is + * set will 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 +1357,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_FLAGS)); + } 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..c78b053 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1003,6 +1003,11 @@ struct vxlanhdr { #define VXLAN_FLAGS 0x08000000 /* struct vxlanhdr.vx_flags required value. */ +#define VXLAN_HF_GBP 0x80000000 /* Group Based Policy, BIT(31) */ +#define VXLAN_GBP_DONT_LEARN 0x400000 /* Don't Learn, (BIT(6) << 16) */ +#define VXLAN_GBP_POLICY_APPLIED 0x80000 /* Policy Applied, (BIT(3) << 16) */ +#define VXLAN_GBP_ID_MASK 0xFFFF /* GBP ID */ + void ipv6_format_addr(const struct in6_addr *addr, struct ds *); void ipv6_format_addr_bracket(const struct in6_addr *addr, struct ds *, bool bracket);