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

[Jesse Gross]

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?

>>
>>  * 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?

>>
>> 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.

>> >  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?