[ovs-dev] [PATCH v4] datapath: Add support for lwtunnel

[Pravin Shelar]

On Wed, Nov 25, 2015 at 6:55 AM, Jesse Gross <jesse at kernel.org> wrote:
> On Tue, Nov 24, 2015 at 2:40 PM, Joe Stringer <joe at ovn.org> wrote:
>> On 23 November 2015 at 21:30, Pravin B Shelar <pshelar at nicira.com> wrote:
>>> Following patch adds support for lwtunnel to OVS datapath.
>>> With this change OVS datapath detect lwtunnel support and
>>> make use of new APIs if available. On older kernel where the
>>> support is not there the backported tunnel modules are used.
>>> These backported tunnel devices acts as lwtunnel devices.
>>> I tried to keep backported module same as upstream for easier
>>> bug-fix backport. Since STT and LISP are not upstream OVS
>>> always needs to use respective modules from tunnel compat layer.
>>> To make it work on kernel 4.3 I have converted STT and LISP
>>> modules to lwtunnel API model.
>>>
>>> lwtunnel make use of skb-dst to pass tunnel information to the
>>> tunnel module. On older kernel this is not possible. So the in
>>> case of old kernel metadata ref is stored in OVS_CB and direct
>>> call to tunnel transmit function is made by respective tunnel
>>> vport modules. Similarly on receive side tunnel recv directly
>>> call netdev-vport-receive to pass the skb to OVS.
>>>
>>> Major backported components include:
>>> Geneve, GRE, VXLAN, ip_tunnel, udp-tunnels GRO.
>>>
>>> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>>
>> Jesse, I've provided some minor feedback below but overall it looks OK
>> to me. Did you have any thoughts on this?
>
> I'm basically happy with this. It seems more or less impossible to
> review this in detail on every kernel but I looked at the overall
> structure and some key areas and it seems generally reasonable. Given
> that Joe did some compile testing on different kernels, I think that's
> good enough and we can work out any problems as we encounter them.
>
> One specific thing that I noticed is that I believe that there is a
> regression in regards to compatibility with GRE devices outside of
> OVS. We used to not register the compat GRE protocol handler until we
> actually created a GRE tunnel but now we do it immediately on OVS
> kernel load, which will block the upstream module from loading.
>
Which upstream module are you referring to? In the patch gre cisco
protocol is registered which only blocks ovs vport-gre module. I do
not think we can insmod upstream OVS and out of tree OVS modules at
same time anyways.

> I think that we need to reinstate the offloaded vlan checks for
> tunnels, since we don't have a real device layer to handle them here.
>
ok.

> Does this work if userspace adds tunnel ports to the datapath without
> going through the compat code? It seems like creation, receive, etc.
> should all be fine but transmit might be a problem since it will just
> use the normal dev_queue_xmit() instead of the specified transmit
> routine. Otherwise, I think the only accommodation that userspace
> needs to make for using the compat code is to try to create the device
> using the ovs_* type over RTNL. Most other things should work with the
> exception of tcpdump, offloading settings,
> and up/down (of those, up/down seems the most feasible, maybe we
> should try to make it work).
>
I can only enable it for kernel which support metadata-dst. I have
fixed in the patch. In case of older kernel, I can not trust the
skb-cb.

> Another general question that came to mind is whether it makes sense
> to have separate kernel modules for each device type (as they are
> upstream) vs. bundling them all into the OVS module. I guess bundling
> everything together avoids having to export interfaces that are really
> for compatibility but is there any other reason to do it this way?
>
I am not sure if I can make it as module. Since few symbols depends on
OVS and OVS also needs symbols from individual tunnel modules. There
is circular dependency between there modules.

> Finally, as Joe mentioned, there are some additional commits in 4.3 on
> top of this (besides the conntracking patches). One that I noticed is
> the egress tunnel info fixes, there are possibly others. I don't want
> to add more code to this patch but we should do a sweep of the change
> log as a follow up.

sure I am planing on doing it.