Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-11-02 Thread William Tu 
On Wed, Nov 1, 2017 at 8:29 AM, Eric Garver  wrote:
> On Mon, Oct 30, 2017 at 10:36:29AM -0700, William Tu wrote:
>> Thanks for the review.
>>
>> Guru and I had some offline discussion. We have concern about possible
>> packet lost when unconditionally remove the interface and bring it
>> again. Currently if the userspace ovs-vswitchd dies, since the kernel
>> datapath module still remains, packets can still go through.
>> If restarting ovs-vswitch restarts the geneve device, then we might
>> see more packets lost. So I'm thinking about another way to fix it
>> without deleting the device.
>
> You can use dpif_netlink_rtnl_getlink() if the device already exists.
> Then check IFLA_INFO_KIND == "ovs_geneve".
>
Thanks Eric,
I followed your suggestion and submit v2 here:
https://mail.openvswitch.org/pipermail/ovs-dev/2017-November/340357.html

William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-11-01 Thread Eric Garver 
On Mon, Oct 30, 2017 at 10:36:29AM -0700, William Tu wrote:
> Thanks for the review.
> 
> Guru and I had some offline discussion. We have concern about possible
> packet lost when unconditionally remove the interface and bring it
> again. Currently if the userspace ovs-vswitchd dies, since the kernel
> datapath module still remains, packets can still go through.
> If restarting ovs-vswitch restarts the geneve device, then we might
> see more packets lost. So I'm thinking about another way to fix it
> without deleting the device.

You can use dpif_netlink_rtnl_getlink() if the device already exists.
Then check IFLA_INFO_KIND == "ovs_geneve".

> 
> Thanks
> William
> 
> On Thu, Oct 26, 2017 at 1:20 PM, Eric Garver  wrote:
> > On Thu, Oct 26, 2017 at 12:31:03PM -0700, William Tu wrote:
> >> When using the out-of-tree (openvswitch compat) geneve module,
> >> the first time oot tunnel probing returns true (correct).
> >> Without unloading the geneve module, if the userspace ovs-vswitchd
> >> restarts, because the 'geneve_sys_6081' still exists, the probing
> >> incorrectly returns false and loads the in-tree (upstream kernel)
> >> geneve module.  The issue also exists the other way around, where
> >> existing geneve module is in-tree and ovs-vswitchd restarts.
> >>
> >> The patch fixes it by unconditionally removing the tunnel device
> >> before the probing.  To reproduce the issue, start the ovs
> >> > /etc/init.d/openvswitch-switch start
> >> > creat a bridge and attach a geneve port using out-of-tree geneve
> >> > /etc/init.d/openvswitch-switch restart
> >>
> >> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
> >> used interface")
> >> Signed-off-by: William Tu 
> >> Cc: Eric Garver 
> >> Cc: Gurucharan Shetty 
> >> ---
> >>  lib/dpif-netlink-rtnl.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> >> index 0c32e7d8ccb4..148ce5ff3a3d 100644
> >> --- a/lib/dpif-netlink-rtnl.c
> >> +++ b/lib/dpif-netlink-rtnl.c
> >> @@ -448,6 +448,8 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
> >>  }
> >>
> >>  name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof 
> >> namebuf);
> >> +dpif_netlink_rtnl_destroy(name);
> >> +
> >>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
> >> OVS_VPORT_TYPE_GENEVE,
> >>   "ovs_geneve",
> >>   (NLM_F_REQUEST | NLM_F_ACK
> >> --
> >> 2.7.4
> >
> > Thanks for the fix!
> >
> > Acked-by: Eric Garver 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-30 Thread William Tu 
Thanks for the review.

Guru and I had some offline discussion. We have concern about possible
packet lost when unconditionally remove the interface and bring it
again. Currently if the userspace ovs-vswitchd dies, since the kernel
datapath module still remains, packets can still go through.
If restarting ovs-vswitch restarts the geneve device, then we might
see more packets lost. So I'm thinking about another way to fix it
without deleting the device.

Thanks
William

On Thu, Oct 26, 2017 at 1:20 PM, Eric Garver  wrote:
> On Thu, Oct 26, 2017 at 12:31:03PM -0700, William Tu wrote:
>> When using the out-of-tree (openvswitch compat) geneve module,
>> the first time oot tunnel probing returns true (correct).
>> Without unloading the geneve module, if the userspace ovs-vswitchd
>> restarts, because the 'geneve_sys_6081' still exists, the probing
>> incorrectly returns false and loads the in-tree (upstream kernel)
>> geneve module.  The issue also exists the other way around, where
>> existing geneve module is in-tree and ovs-vswitchd restarts.
>>
>> The patch fixes it by unconditionally removing the tunnel device
>> before the probing.  To reproduce the issue, start the ovs
>> > /etc/init.d/openvswitch-switch start
>> > creat a bridge and attach a geneve port using out-of-tree geneve
>> > /etc/init.d/openvswitch-switch restart
>>
>> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
>> used interface")
>> Signed-off-by: William Tu 
>> Cc: Eric Garver 
>> Cc: Gurucharan Shetty 
>> ---
>>  lib/dpif-netlink-rtnl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
>> index 0c32e7d8ccb4..148ce5ff3a3d 100644
>> --- a/lib/dpif-netlink-rtnl.c
>> +++ b/lib/dpif-netlink-rtnl.c
>> @@ -448,6 +448,8 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>>  }
>>
>>  name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>> +dpif_netlink_rtnl_destroy(name);
>> +
>>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
>> OVS_VPORT_TYPE_GENEVE,
>>   "ovs_geneve",
>>   (NLM_F_REQUEST | NLM_F_ACK
>> --
>> 2.7.4
>
> Thanks for the fix!
>
> Acked-by: Eric Garver 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-26 Thread Eric Garver 
On Thu, Oct 26, 2017 at 12:31:03PM -0700, William Tu wrote:
> When using the out-of-tree (openvswitch compat) geneve module,
> the first time oot tunnel probing returns true (correct).
> Without unloading the geneve module, if the userspace ovs-vswitchd
> restarts, because the 'geneve_sys_6081' still exists, the probing
> incorrectly returns false and loads the in-tree (upstream kernel)
> geneve module.  The issue also exists the other way around, where
> existing geneve module is in-tree and ovs-vswitchd restarts.
> 
> The patch fixes it by unconditionally removing the tunnel device
> before the probing.  To reproduce the issue, start the ovs
> > /etc/init.d/openvswitch-switch start
> > creat a bridge and attach a geneve port using out-of-tree geneve
> > /etc/init.d/openvswitch-switch restart
> 
> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
> used interface")
> Signed-off-by: William Tu 
> Cc: Eric Garver 
> Cc: Gurucharan Shetty 
> ---
>  lib/dpif-netlink-rtnl.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..148ce5ff3a3d 100644
> --- a/lib/dpif-netlink-rtnl.c
> +++ b/lib/dpif-netlink-rtnl.c
> @@ -448,6 +448,8 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>  }
>  
>  name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
> +dpif_netlink_rtnl_destroy(name);
> +
>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
> OVS_VPORT_TYPE_GENEVE,
>   "ovs_geneve",
>   (NLM_F_REQUEST | NLM_F_ACK
> -- 
> 2.7.4

Thanks for the fix!

Acked-by: Eric Garver 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-26 Thread William Tu 
When using the out-of-tree (openvswitch compat) geneve module,
the first time oot tunnel probing returns true (correct).
Without unloading the geneve module, if the userspace ovs-vswitchd
restarts, because the 'geneve_sys_6081' still exists, the probing
incorrectly returns false and loads the in-tree (upstream kernel)
geneve module.  The issue also exists the other way around, where
existing geneve module is in-tree and ovs-vswitchd restarts.

The patch fixes it by unconditionally removing the tunnel device
before the probing.  To reproduce the issue, start the ovs
> /etc/init.d/openvswitch-switch start
> creat a bridge and attach a geneve port using out-of-tree geneve
> /etc/init.d/openvswitch-switch restart

Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides used 
interface")
Signed-off-by: William Tu 
Cc: Eric Garver 
Cc: Gurucharan Shetty 
---
 lib/dpif-netlink-rtnl.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
index 0c32e7d8ccb4..148ce5ff3a3d 100644
--- a/lib/dpif-netlink-rtnl.c
+++ b/lib/dpif-netlink-rtnl.c
@@ -448,6 +448,8 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
 }
 
 name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
+dpif_netlink_rtnl_destroy(name);
+
 error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
  "ovs_geneve",
  (NLM_F_REQUEST | NLM_F_ACK
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-26 Thread William Tu 
On Thu, Oct 26, 2017 at 11:15 AM, Eric Garver  wrote:
> On Thu, Oct 26, 2017 at 09:12:14AM -0700, William Tu wrote:
>> On Wed, Oct 25, 2017 at 10:39 AM, Eric Garver  wrote:
>> > On Tue, Oct 24, 2017 at 02:43:31PM -0700, William Tu wrote:
>> >
>> > Hi William,
>> > Thanks for taking a look at this.
>> >
>> >> When using the out-of-tree (openvswitch compat) geneve module,
>> >> the first time oot tunnel probing returns true (correct).
>> >> Without unloading the geneve module, if the userspace ovs-vswitchd
>> >> restarts, because the 'geneve_sys_6081' still exists, the probing
>> >> incorrectly returns false and loads the in-tree (upstream kernel)
>> >> geneve module.
>> >
>> > The reason for this is because rtnl sees the existing device and tries
>> > to call ->changelink, but since the out-of-tree tunnel has no handler it
>> > returns EOPNOTSUPP.
>> >
>> >> The patch fixes it by adding NLM_F_EXCL flags, so if geneve already
>> >> exists, the dpif_netlink_rtnl_create return EEXIST and the oot tunnel
>> >> probing returns true.  To reproduce the issue, start the ovs
>> >
>> > While this fixes this scenario, it breaks another; an existing device
>> > in-tree/kernel geneve tunnel. We'll end up with the opposite occurring -
>> > it returns true when it should return false.
>> >
>> > So in addition to adding NLM_F_EXCL, this needs to also try to delete
>> > the existing device in the case of EEXISTS, then restart the probe.
>> >
>>
>> You're right, thanks!
>> How about we unconditionally delete the device before the probe?. I'm
>> simply adding one line without using NLM_F_EXCL:
>>
>> +   dpif_netlink_rtnl_destroy(name);
>>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
>> OVS_VPORT_TYPE_GENEVE,
>>   "ovs_geneve",
>>   (NLM_F_REQUEST | NLM_F_ACK
>>| NLM_F_CREATE));
>>
>> I'm testing using in-tree or out-of-tree and the result is consistent.
>>
>
> I think that will work.
thanks, I will send out v2.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-26 Thread William Tu 
On Wed, Oct 25, 2017 at 10:39 AM, Eric Garver  wrote:
> On Tue, Oct 24, 2017 at 02:43:31PM -0700, William Tu wrote:
>
> Hi William,
> Thanks for taking a look at this.
>
>> When using the out-of-tree (openvswitch compat) geneve module,
>> the first time oot tunnel probing returns true (correct).
>> Without unloading the geneve module, if the userspace ovs-vswitchd
>> restarts, because the 'geneve_sys_6081' still exists, the probing
>> incorrectly returns false and loads the in-tree (upstream kernel)
>> geneve module.
>
> The reason for this is because rtnl sees the existing device and tries
> to call ->changelink, but since the out-of-tree tunnel has no handler it
> returns EOPNOTSUPP.
>
>> The patch fixes it by adding NLM_F_EXCL flags, so if geneve already
>> exists, the dpif_netlink_rtnl_create return EEXIST and the oot tunnel
>> probing returns true.  To reproduce the issue, start the ovs
>
> While this fixes this scenario, it breaks another; an existing device
> in-tree/kernel geneve tunnel. We'll end up with the opposite occurring -
> it returns true when it should return false.
>
> So in addition to adding NLM_F_EXCL, this needs to also try to delete
> the existing device in the case of EEXISTS, then restart the probe.
>

You're right, thanks!
How about we unconditionally delete the device before the probe?. I'm
simply adding one line without using NLM_F_EXCL:

+   dpif_netlink_rtnl_destroy(name);
 error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
  "ovs_geneve",
  (NLM_F_REQUEST | NLM_F_ACK
   | NLM_F_CREATE));

I'm testing using in-tree or out-of-tree and the result is consistent.

Regards,
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-25 Thread Eric Garver 
On Tue, Oct 24, 2017 at 02:43:31PM -0700, William Tu wrote:

Hi William,
Thanks for taking a look at this.

> When using the out-of-tree (openvswitch compat) geneve module,
> the first time oot tunnel probing returns true (correct).
> Without unloading the geneve module, if the userspace ovs-vswitchd
> restarts, because the 'geneve_sys_6081' still exists, the probing
> incorrectly returns false and loads the in-tree (upstream kernel)
> geneve module.

The reason for this is because rtnl sees the existing device and tries
to call ->changelink, but since the out-of-tree tunnel has no handler it
returns EOPNOTSUPP.

> The patch fixes it by adding NLM_F_EXCL flags, so if geneve already
> exists, the dpif_netlink_rtnl_create return EEXIST and the oot tunnel
> probing returns true.  To reproduce the issue, start the ovs

While this fixes this scenario, it breaks another; an existing device
in-tree/kernel geneve tunnel. We'll end up with the opposite occurring -
it returns true when it should return false.

So in addition to adding NLM_F_EXCL, this needs to also try to delete
the existing device in the case of EEXISTS, then restart the probe.

> > /etc/init.d/openvswitch-switch start
> > creat a bridge and attach a geneve port using out-of-tree geneve
> > /etc/init.d/openvswitch-switch restart
> 
> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
> used interface")
> Signed-off-by: William Tu 
> Cc: Eric Garver 
> Cc: Gurucharan Shetty 
> ---
>  lib/dpif-netlink-rtnl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
> index 0c32e7d8ccb4..ab9737b8cc4b 100644
> --- a/lib/dpif-netlink-rtnl.c
> +++ b/lib/dpif-netlink-rtnl.c
> @@ -451,7 +451,7 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
> OVS_VPORT_TYPE_GENEVE,
>   "ovs_geneve",
>   (NLM_F_REQUEST | NLM_F_ACK
> -  | NLM_F_CREATE));
> +  | NLM_F_CREATE | NLM_F_EXCL));
>  if (error != EOPNOTSUPP) {
>  if (!error) {
>  dpif_netlink_rtnl_destroy(name);
> -- 
> 2.7.4
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

2017-10-24 Thread William Tu 
When using the out-of-tree (openvswitch compat) geneve module,
the first time oot tunnel probing returns true (correct).
Without unloading the geneve module, if the userspace ovs-vswitchd
restarts, because the 'geneve_sys_6081' still exists, the probing
incorrectly returns false and loads the in-tree (upstream kernel)
geneve module.

The patch fixes it by adding NLM_F_EXCL flags, so if geneve already
exists, the dpif_netlink_rtnl_create return EEXIST and the oot tunnel
probing returns true.  To reproduce the issue, start the ovs
> /etc/init.d/openvswitch-switch start
> creat a bridge and attach a geneve port using out-of-tree geneve
> /etc/init.d/openvswitch-switch restart

Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides used 
interface")
Signed-off-by: William Tu 
Cc: Eric Garver 
Cc: Gurucharan Shetty 
---
 lib/dpif-netlink-rtnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
index 0c32e7d8ccb4..ab9737b8cc4b 100644
--- a/lib/dpif-netlink-rtnl.c
+++ b/lib/dpif-netlink-rtnl.c
@@ -451,7 +451,7 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
 error = dpif_netlink_rtnl_create(tnl_cfg, name, OVS_VPORT_TYPE_GENEVE,
  "ovs_geneve",
  (NLM_F_REQUEST | NLM_F_ACK
-  | NLM_F_CREATE));
+  | NLM_F_CREATE | NLM_F_EXCL));
 if (error != EOPNOTSUPP) {
 if (!error) {
 dpif_netlink_rtnl_destroy(name);
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev