[ovs-dev] [PATCH 02/13] Upstream GRE : Refactor code GRE code.

Pravin Shelar pshelar at nicira.com
Tue Dec 25 07:53:12 UTC 2012


On Sat, Dec 15, 2012 at 2:27 AM, Jesse Gross <jesse at nicira.com> wrote:

> On Fri, Dec 14, 2012 at 11:40 AM, Pravin Shelar <pshelar at nicira.com>
> wrote:
> > On Wed, Dec 12, 2012 at 8:04 AM, Jesse Gross <jesse at nicira.com> wrote:
> >>
> >> Thanks and sorry that I took so long to get back to you.
> >>
> >> I thought about the layering a fair amount and am still somewhat
> >> bothered by the flow of things.  I think if we make a couple small
> >> modifications then it will fit more cleanly:
> >>  * I don't think that we really need ip_tunnel_xmit().  It's primarily
> >> there to handle offloads but if we get in the GSO changes before
> >> allowing upper layer protocols to send these offloaded packets then we
> >> don't have to worry about it.  Also, the send_frags() function is
> >> there due to CAPWAP's protocol level fragmentation, which we don't
> >> have to worry about here.  Without these two we can call directly into
> >> the GRE data plane code and have it do the complete process without
> >> much in the way of code duplication.
> >
> >
> > OK, we can drop capwap related support in xmit, but IPIP still needs it,
> so
> > I think we can keep it in ip_tunnel.
>
> You mean IPIP needs the offload functionality?  Can't we just not
> support offloading at the device level for it?
>
> There is other common code. But I think we can have separate function for
it.

>  >>
> >>  * Registering to receive packets should probably into the GRE data
> >> plane module instead of directly to the IP tunnel code.  There's
> >> potentially a module dependency problem otherwise - if all you do is
> >> register to receive packets then there's nothing that will actually
> >> cause the GRE module to get loaded.  This is resolved at the moment by
> >> the use of the transmit function but it seems a little fragile.  We
> >> can definitely still use the IP tunnel code and share structures, etc.
> >> but we would just add an extra hop.
> >>
> >
> > we can have direct dependance of gre module on ip_tunnel module by
> pulling
> > hash-lookup into ip_tunnel. ip_gre has dependency on gre. So am I not
> sure
> > abt fragile dependency.
>
> The dependency that I'm thinking about is the latter one - ip_gre (or
> OVS) on gre.  Isn't the only dependency currently a result of passing
> the gre_build_header() function as a pointer so that if I just
> registered to receive then I wouldn't actually receive any packets?
>
>
ok, gre specific handler can be moved to gre data plane module.

> >>
> >> At this point the upper tunneling interface would look something like
> >> this:
> >> gre_register()
> >> gre_unregister()
> >
> >
> > what about vxlan and future protocols?
>
> I think we would end up with vxlan_* equivalents as well.  I
> originally was hoping that we could do it in a completely generic way
> but if we need to pass in constants and function pointers then I'm not
> sure that it actually makes it any more generic.
>
> OK, I will add protocol specific demux.


> >> >  static netdev_tx_t ipgre_tunnel_xmit(struct sk_buff *skb, struct
> >> > net_device *dev)
> >> [...]
> >> > +       err = ip_tunnel_build_iphdr(skb, dev, tiph, gre_hlen, &iph);
> >> > +       if (err)
> >> > +               return NETDEV_TX_OK;
> >> > +
> >> > +       tpi.flags = tunnel->parms.o_flags;
> >> > +       tpi.proto = proto;
> >> > +       tpi.key = tunnel->parms.o_key;
> >> > +       tpi.seq =  htonl(tunnel->o_seqno);
> >> > +       tpi.hdr_len = tunnel->hlen;
> >> > +       if (skb_cow_head(skb, dev->needed_headroom)) {
> >> > +               dev->stats.tx_dropped++;
> >> >                 dev_kfree_skb(skb);
> >> > -               skb = new_skb;
> >> > -               old_iph = ip_hdr(skb);
> >> > -       }
> >>
> >> Can we push this down into the encapsulation code?  It seems like a
> >> common data plane piece.
> >>
> > do u meanpush it to ip_tunnel_xmit()? I am using same function in ip_gre
> and
> > ovs-gre thats why i kept it here rather than ip_tunnel.
>
> Yes, I meant to push the skb_cow_head() into either ip_tunnel_xmit()
> or gre_build_header() depending on what we do with the former.  I'm
> not sure what you mean by wanting to keep it separate.  Won't both
> callers need it and, if so, isn't a reason to push it down?
>

ok. I will post updated patches.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20121224/b5d122f0/attachment-0004.html>


More information about the dev mailing list