On Wed, Apr 20, 2016 at 12:39 AM, Johnson.Li <johnson...@intel.com> wrote:
> From: Johnson Li <johnson...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
