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

[Li, Johnson]

> -----Original Message-----
> From: Jesse Gross [mailto:jesse at kernel.org]
> Sent: Thursday, April 21, 2016 4:23 AM
> To: Li, Johnson <johnson.li at intel.com>
> Cc: ovs dev <dev at 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 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.
> 
> 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.