Hi Harald,

On 7/13/17, 12:26 AM, "Harald Welte" [off-list ref] wrote:
·

quoted

 static inline void gtp_set_pktinfo_ipv4(struct gtp_pktinfo *pktinfo,
          struct sock *sk, struct iphdr *iph,
-         struct pdp_ctx *pctx, struct rtable *rt,
-         struct flowi4 *fl4,
+         struct rtable *rt, struct flowi4 *fl4,
          struct net_device *dev)
 {
 [...]
+ __be32 tun_id;

·
you are breaking GTPv0 functionality here.  GTPv0 has 64 bit tunnel
identifiers, and this function is called both from GTPv1 and GTPv0
context.
·
This makes me wonder how you did verify that your changes do not break
the existing operation with both GTPv0 and GTPv1?
·

Good catch. I only fully tested the GTPv1 path against oai-cn. Will fix
this and test the GTPv0 path as well.

I had doubts on how this flow-based GTPv1 code path should fit in, 
which is why the GTPv0 and the GTPv1 code pieces are mixed in my changes. 

Should I explicitly claim that the flow-based change is for GTPv1 only? 

quoted

+ // flow-based GTP1U encap
+ info = skb_tunnel_info(skb);
+ if (gtp->collect_md && info && ntohs(info->key.tp_dst) == GTP1U_PORT) {

·
I think it's typically safe to assume that GTP is only operated on
standard ports, but it is something you chould/should think about, i.e.
whether you want that kind of restriction.  In the existing use case, we
have the v0/v1 information stored in the per-pdp context structure.
·

The reason I’m checking GTP1U_PORT here is to filter GTP1U traffic. 
It possible to pass a port number from ovs into the gtp module. I will 
investigate how to support programmable port. 

quoted

+   tun_id  = htonl(pctx->u.v1.o_tei);

·
here is where you're assuming GTPv1 in two ways from code that is called
from both v0 and v1.
* you're dereferencing a v1 specific element in the pctx union
* you're storing the result in a 32bit variable
·

Right, will fix this for GTPv0.

quoted

  gtp = netdev_priv(dev);
+ gtp->net = src_net;

·
Isn't this a generic change that's independent of your work on OVS GTP?

It is meant to be OVS independent. What makes it not? Should I leave 
this field un-initialized?

Thanks
-Jiannan

_______________________________________________
dev mailing list
dev@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev