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

[Yang, Feng]

Hi Aaron, Yi,
Good questions and explanation.
Regarding the GTP-U and GTP-C issue, the comment that "These include, but not limited to GTP-U messages, GTP-C packets" sounds misleading indeed.
We will remove "GTP-C packets" in v3.  

Regards, Feng
2018-5-17
-----Original Message-----
From: Yang, Yi Y 
Sent: Thursday, May 17, 2018 10:22 AM
To: Aaron Conole <aconole at redhat.com>
Cc: dev at openvswitch.org; Yang, Feng <feng.yang at intel.com>
Subject: Re: [ovs-dev] [PATCH v2 1/2] userspace datapath: Add GTP-U tunnel support

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.