> -----Original Message-----
> From: Jesse Gross [mailto:je...@kernel.org]
> Sent: Thursday, April 21, 2016 4:23 AM
> To: Li, Johnson <johnson...@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...@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.
>
> 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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
