Re: [PATCH net-next v1 1/3] gtp: refactor to support flow-based gtp encap and decap

[Jiannan Ouyang <ouyangj-b10kYP2dOMg@public.gmane.org>]

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

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