On Wed, Nov 25, 2015 at 4:10 AM, Joe Stringer <j...@ovn.org> wrote:
> On 23 November 2015 at 21:30, Pravin B Shelar <pshe...@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 <pshe...@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.
>
STT port create returns error in this case. But I do not think we
should keep the code in that case. So I have added OVS_STT #ifdef in
vport_stt.c
> 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:
>
I want to check latest net-next to backport all patches merged later
on, So I am planing on sending these fixes later on. Following fix is
important fix, so I have folded it in the patch.
> 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);
...
I have included it in the patch.
>
> 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.
>
Thanks for the review.
> Acked-by: Joe Stringer <j...@ovn.org>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
