On Thu, May 17, 2018 at 05:11:24AM +0800, Aaron Conole wrote: > Yi Yang <yi.y.y...@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.y...@intel.com> > > Signed-off-by: Jiannan Ouyang <ouya...@fb.com> > > Signed-off-by: Yi Yang <yi.y.y...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev