Hi Harald,
On 7/13/17, 12:26 AM, "Harald Welte" laforge@gnumonks.org wrote: ·
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?
- // 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.
- 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.
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