[ovs-dev] [PATCH v2 1/2] userspace datapath: Add GTP-U tunnel support

Yang, Yi yi.y.yang at intel.com
Thu May 17 02:22:20 UTC 2018


On Thu, May 17, 2018 at 05:11:24AM +0800, Aaron Conole wrote:
> Yi Yang <yi.y.yang at intel.com> writes:
> 
> Hi Yi,
> 
> Thanks for the patch!  Just a brief review.
> 

Aaron, thank you so much for your quick review.

> >
> > Signed-off-by: Feng Yang <feng.yang at intel.com>
> > Signed-off-by: Jiannan Ouyang <ouyangj at fb.com>
> > Signed-off-by: Yi Yang <yi.y.yang at intel.com>
> 
> Needs Co-author tags.

Ok, will add it in v3.

> >  OXM_CLASSES = {"NXM_OF_":        (0,          0x0000),
> >                 "NXM_NX_":        (0,          0x0001),
> >                 "NXOXM_NSH_":     (0x005ad650, 0xffff),
> > +               "NXOXM_GTPU_":    (0x005ad651, 0xffff),
> >                 "OXM_OF_":        (0,          0x8000),
> >                 "OXM_OF_PKT_REG": (0,          0x8001),
> >                 "ONFOXM_ET_":     (0x4f4e4600, 0xffff),
> > diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h
> > index 84ebcaf..ad6ea64 100644
> > --- a/datapath/linux/compat/include/linux/openvswitch.h
> > +++ b/datapath/linux/compat/include/linux/openvswitch.h
> 
> I don't see this in the upstream code.  This is introducing another
> place for kernel and ovs to diverge.  Please get this type accepted
> upstream first.

You're partially right :-), there was the case OVS accepted first, then
kernel did so. I think OVS community is very open for this.

> > diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> > index 40c4569..a628b72 100644
> > --- a/lib/dpif-netlink-rtnl.c
> > +++ b/lib/dpif-netlink-rtnl.c
> 
> Again, you'll need to get the netlink datapath changes through kernel
> process.  At least try.

Yeah, I will, because Facebook guy Jiannan Ouyang did that before, they
won't push this, so I'll try pushing this, but you know this is a slow
process.

> >          break;
> > @@ -814,6 +817,8 @@ netdev_to_ovs_vport_type(const char *type)
> >          return OVS_VPORT_TYPE_VXLAN;
> >      } else if (!strcmp(type, "lisp")) {
> >          return OVS_VPORT_TYPE_LISP;
> > +    } else if (strstr(type, "gtpu")) {
> 
> Why isn't this using "!strcmp"?

good catch, it should be "!strcmp", I'll change it in v3.

> >  
> > +    /* It won't be parsed if the packet contains application layer data only.
> > +     * These include, but not limited to GTP-U messages, GTP-C packets.
> > +     */
> 
> Why treat GTP-U and C separate here?

GTP-C is very complicated and it is for control plane, so it won't be
helpful to have it in OVS datapath, Joe mentioned this in v1 comments, we discussed
this, GTP-U is what we care in real user scenarios.

In addition, GPT-C and GTP-U use different UDP port, GTP-U tunnel can't
handle GTP-C, here the comment may be misleading you. It means ovs won't
parse the packet if it is GTP-U signaling message but not normal GTP-U
PDU.


More information about the dev mailing list