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

[Joe Stringer]

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?


Question, I see OVS_STT #ifdef is removed from datapath/vport-stt.c,
although it's still not supported on <3.5 kernels - is the vport-stt
module just compiled as a stub module which does nothing on kernels <
3.5? Just wondering what it might look like from the user perspective
if they attempted to build,load,use STT on an earlier kernel.

The fragment below is missing from the backport, introduced in
upstream commit 34ae932a4036 ("openvswitch: Make tunnel set action
attach a metadata dst"). When it is missing, it causes memory leakage
in cmd_execute path:

diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index 25c796fb6a11..27bfc771c39e 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
@@ -18,6 +18,7 @@

 #include "flow.h"
 #include "datapath.h"
+#include "flow_netlink.h"
 #include <linux/uaccess.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -151,7 +152,8 @@ static void flow_free(struct sw_flow *flow)

        if (ovs_identifier_is_key(&flow->id))
                kfree(flow->id.unmasked_key);
-       kfree(rcu_dereference_raw(flow->sf_acts));
+       if (flow->sf_acts)
+               ovs_nla_free_flow_actions((struct sw_flow_actions
__force *)flow->sf_acts);
        for_each_node(node)
                if (flow->stats[node])
                        kmem_cache_free(flow_stats_cache,


Please also consider these incremental spelling fixes:

diff --git a/datapath/vport-geneve.c b/datapath/vport-geneve.c
index b220a063d8e8..3b5c1ab32e6b 100644
--- a/datapath/vport-geneve.c
+++ b/datapath/vport-geneve.c
@@ -146,6 +146,6 @@ static void __exit ovs_geneve_tnl_exit(void)
 module_init(ovs_geneve_tnl_init);
 module_exit(ovs_geneve_tnl_exit);

-MODULE_DESCRIPTION("OVS: Geneve swiching port");
+MODULE_DESCRIPTION("OVS: Geneve switching port");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("vport-type-5");
diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
index ff9ec8470229..186a78550ce8 100644
--- a/datapath/vport-lisp.c
+++ b/datapath/vport-lisp.c
@@ -146,6 +146,6 @@ static void __exit ovs_lisp_tnl_exit(void)
 module_init(ovs_lisp_tnl_init);
 module_exit(ovs_lisp_tnl_exit);

-MODULE_DESCRIPTION("OVS: Lisp swiching port");
+MODULE_DESCRIPTION("OVS: LISP switching port");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("vport-type-105");
diff --git a/datapath/vport-stt.c b/datapath/vport-stt.c
index 581c156bd367..76d4961bba31 100644
--- a/datapath/vport-stt.c
+++ b/datapath/vport-stt.c
@@ -147,6 +147,6 @@ static void __exit ovs_stt_tnl_exit(void)
 module_init(ovs_stt_tnl_init);
 module_exit(ovs_stt_tnl_exit);

-MODULE_DESCRIPTION("OVS: STT swiching port");
+MODULE_DESCRIPTION("OVS: STT switching port");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("vport-type-106");


Other than that, my Centos7 build was happy, Travis is happy, the
kernel testsuite with the ct backport is passing, so it looks good to
me. I assume you've tested with some of the older kernels; I've been
testing with redhat 3.10, ubuntu 3.13 and vanilla 4.3 and it seems to
be good on those.

Acked-by: Joe Stringer <joe at ovn.org>