On Sat, Dec 15, 2012 at 2:27 AM, Jesse Gross <je...@nicira.com> wrote:
> On Fri, Dec 14, 2012 at 11:40 AM, Pravin Shelar <pshe...@nicira.com>
> wrote:
> > On Wed, Dec 12, 2012 at 8:04 AM, Jesse Gross <je...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
