Hi Harald,
> On 7/13/17, 12:26 AM, "Harald Welte" <lafo...@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
