Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Siwei Liu]

On Thu, May 31, 2018 at 11:35 AM, Michael S. Tsirkin  wrote:
> On Wed, May 30, 2018 at 10:06:35PM -0400, Stephen Hemminger wrote:
>> On Thu, 24 May 2018 09:55:14 -0700
>> Sridhar Samudrala  wrote:
>>
>> > Use the registration/notification framework supported by the generic
>> > failover infrastructure.
>> >
>> > Signed-off-by: Sridhar Samudrala 
>>
>> Why was this merged? It was never signed off by any of the netvsc 
>> maintainers,
>> and there were still issues unresolved.
>>
>> There are also namespaces issues I am fixing and this breaks them.
>> Will start my patch set with a revert for this. Sorry
>
> As long as you finish the patch set with re-integrating with failover,
> that's fine IMHO.
>
> I suspect it's easier to add the code to failover though - namespace
> things likely affect virtio as well. Lookup by ID would be an optional
> feature for virtio, but probably a useful one - I won't ask you
I would think for production uses this is a required feature and
should be enabled by default.

Venu (cc'ed) is working on the group ID stuff currently for pairing
virtio and passthrough devices. Would appreciate feedback on the group
ID proposal on virtio-dev.

-Siwei

> to add it to virtio but it could be a mode in failover
> that virtio will activate down the road. And reducing the number of
> times we look cards up based on ID can only be a good thing.
>
> --
> MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Wed, May 30, 2018 at 10:06:35PM -0400, Stephen Hemminger wrote:
> On Thu, 24 May 2018 09:55:14 -0700
> Sridhar Samudrala  wrote:
> 
> > Use the registration/notification framework supported by the generic
> > failover infrastructure.
> > 
> > Signed-off-by: Sridhar Samudrala 
> 
> Why was this merged? It was never signed off by any of the netvsc maintainers,
> and there were still issues unresolved.
> 
> There are also namespaces issues I am fixing and this breaks them.
> Will start my patch set with a revert for this. Sorry

As long as you finish the patch set with re-integrating with failover,
that's fine IMHO.

I suspect it's easier to add the code to failover though - namespace
things likely affect virtio as well. Lookup by ID would be an optional
feature for virtio, but probably a useful one - I won't ask you
to add it to virtio but it could be a mode in failover
that virtio will activate down the road. And reducing the number of
times we look cards up based on ID can only be a good thing.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Michael S. Tsirkin]

On Thu, May 31, 2018 at 08:58:12AM -0400, Stephen Hemminger wrote:
> On Wed, 30 May 2018 20:03:11 -0700
> "Samudrala, Sridhar"  wrote:
> 
> > On 5/30/2018 7:06 PM, Stephen Hemminger wrote:
> > > On Thu, 24 May 2018 09:55:14 -0700
> > > Sridhar Samudrala  wrote:
> > >  
> > >> Use the registration/notification framework supported by the generic
> > >> failover infrastructure.
> > >>
> > >> Signed-off-by: Sridhar Samudrala   
> > > Why was this merged? It was never signed off by any of the netvsc 
> > > maintainers,
> > > and there were still issues unresolved.
> > >
> > > There are also namespaces issues I am fixing and this breaks them.
> > > Will start my patch set with a revert for this. Sorry  
> > 
> > I would appreciate if you can make the fixes on top of this patch series. I 
> > tried hard
> > to make sure that netvsc functionality and behavior doesn't change.
> > 
> > It is possible that there could be some bugs introduced, but they can be 
> > fixed.
> > Looks like Wei already found a bug and submitted a fix for that.
> > 
> 
> Ok, but several of these may clash with what you want for virtio.
> Like:
>   - VF should be moved to namespace of virt device
>   - VF should be associated based on message from host with serial # not
> registration notifier and MAC address.
>   - control operations should use master device reference rather than
> searching based on MAC.
> 
> As you can see these are structural changes.

We might want to do these for virtio as well, at least as
an option.

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Stephen Hemminger]

On Wed, 30 May 2018 20:03:11 -0700
"Samudrala, Sridhar"  wrote:

> On 5/30/2018 7:06 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:14 -0700
> > Sridhar Samudrala  wrote:
> >  
> >> Use the registration/notification framework supported by the generic
> >> failover infrastructure.
> >>
> >> Signed-off-by: Sridhar Samudrala   
> > Why was this merged? It was never signed off by any of the netvsc 
> > maintainers,
> > and there were still issues unresolved.
> >
> > There are also namespaces issues I am fixing and this breaks them.
> > Will start my patch set with a revert for this. Sorry  
> 
> I would appreciate if you can make the fixes on top of this patch series. I 
> tried hard
> to make sure that netvsc functionality and behavior doesn't change.
> 
> It is possible that there could be some bugs introduced, but they can be 
> fixed.
> Looks like Wei already found a bug and submitted a fix for that.
> 

Ok, but several of these may clash with what you want for virtio.
Like:
- VF should be moved to namespace of virt device
- VF should be associated based on message from host with serial # not
  registration notifier and MAC address.
- control operations should use master device reference rather than
  searching based on MAC.

As you can see these are structural changes.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/30/2018 7:06 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:


Use the registration/notification framework supported by the generic
failover infrastructure.

Signed-off-by: Sridhar Samudrala 

Why was this merged? It was never signed off by any of the netvsc maintainers,
and there were still issues unresolved.

There are also namespaces issues I am fixing and this breaks them.
Will start my patch set with a revert for this. Sorry


I would appreciate if you can make the fixes on top of this patch series. I 
tried hard
to make sure that netvsc functionality and behavior doesn't change.

It is possible that there could be some bugs introduced, but they can be fixed.
Looks like Wei already found a bug and submitted a fix for that.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Stephen Hemminger]

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:

> Use the registration/notification framework supported by the generic
> failover infrastructure.
> 
> Signed-off-by: Sridhar Samudrala 

Why was this merged? It was never signed off by any of the netvsc maintainers,
and there were still issues unresolved.

There are also namespaces issues I am fixing and this breaks them.
Will start my patch set with a revert for this. Sorry

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

'On 5/26/2018 12:51 AM, Jiri Pirko wrote:

Sat, May 26, 2018 at 09:22:18AM CEST, sridhar.samudr...@intel.com wrote:

On 5/25/2018 4:28 PM, Stephen Hemminger wrote:

On Fri, 25 May 2018 16:11:47 -0700
"Samudrala, Sridhar"  wrote:


On 5/25/2018 3:34 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:

--- a/drivers/net/hyperv/Kconfig
+++ b/drivers/net/hyperv/Kconfig
@@ -2,5 +2,6 @@ config HYPERV_NET
tristate "Microsoft Hyper-V virtual network driver"
depends on HYPERV
select UCS2_STRING
+   select FAILOVER

When I take a working kernel config, add the patches then do
make oldconfig

It is not autoselecting FAILOVER, it prompts me for it. This means
if user says no then a non-working netvsc device is made.

I see
  Generic failover module (FAILOVER) [M/y/?] (NEW)

So the user is given an option to either build as a Module or part of the
kernel. 'n' is not an option.

With most libraries there is no prompt at all.

Not sure what you meant by this.
Without any patches applied, i had a .config file with HYPERV_NET configured
as a module.
Then after applying the first 2 patches in this series, i did a
  make oldconfig
and i see the above prompt.

Are you saying that on some distros, 'make oldconfig creates a .config
file without any prompt and FAILOVER is not getting selected even when 
HYPERV_NET
is enabled?



Well the thing is that for a user, it makes no sense to select
"FAILOVER" by hand. It is a lib, so it should be only select it by a
user. It has no sense to have it turned on by hand - no lib user.
You can achieve that by simply removing "help" for the Kconfig
item. Same thing for "NET_FAILOVER".


I played around with the CONFIG options and i see that FAILOVER options do
get selected correctly when virtio-net/netvsc are enabled.  Even if the FAILOVER
is turned off by the user before the hyperv-net/virtio-net patches are applied,
it gets selected automatically when hyperv-net/virtio-net patches are applied 
and
enabled in config.

If we don't want to allow the user to see these options, then i think we need to
remove them from Kconfig files.  Just removing "help" doesn't seem to make a
difference.

Can we address any config issues (i don't see any at this point) as a bug-fix 
on top
of this series?


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Jiri Pirko]

Sat, May 26, 2018 at 09:22:18AM CEST, sridhar.samudr...@intel.com wrote:
>On 5/25/2018 4:28 PM, Stephen Hemminger wrote:
>> On Fri, 25 May 2018 16:11:47 -0700
>> "Samudrala, Sridhar"  wrote:
>> 
>> > On 5/25/2018 3:34 PM, Stephen Hemminger wrote:
>> > > On Thu, 24 May 2018 09:55:14 -0700
>> > > Sridhar Samudrala  wrote:
>> > > > --- a/drivers/net/hyperv/Kconfig
>> > > > +++ b/drivers/net/hyperv/Kconfig
>> > > > @@ -2,5 +2,6 @@ config HYPERV_NET
>> > > >tristate "Microsoft Hyper-V virtual network driver"
>> > > >depends on HYPERV
>> > > >select UCS2_STRING
>> > > > +  select FAILOVER
>> > > When I take a working kernel config, add the patches then do
>> > > make oldconfig
>> > > 
>> > > It is not autoselecting FAILOVER, it prompts me for it. This means
>> > > if user says no then a non-working netvsc device is made.
>> > I see
>> >  Generic failover module (FAILOVER) [M/y/?] (NEW)
>> > 
>> > So the user is given an option to either build as a Module or part of the
>> > kernel. 'n' is not an option.
>> With most libraries there is no prompt at all.
>
>Not sure what you meant by this.
>Without any patches applied, i had a .config file with HYPERV_NET configured
>as a module.
>Then after applying the first 2 patches in this series, i did a
>  make oldconfig
>and i see the above prompt.
>
>Are you saying that on some distros, 'make oldconfig creates a .config
>file without any prompt and FAILOVER is not getting selected even when 
>HYPERV_NET
>is enabled?
>
>

Well the thing is that for a user, it makes no sense to select
"FAILOVER" by hand. It is a lib, so it should be only select it by a
user. It has no sense to have it turned on by hand - no lib user.
You can achieve that by simply removing "help" for the Kconfig
item. Same thing for "NET_FAILOVER".
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/25/2018 4:28 PM, Stephen Hemminger wrote:

On Fri, 25 May 2018 16:11:47 -0700
"Samudrala, Sridhar"  wrote:


On 5/25/2018 3:34 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:
  

--- a/drivers/net/hyperv/Kconfig
+++ b/drivers/net/hyperv/Kconfig
@@ -2,5 +2,6 @@ config HYPERV_NET
tristate "Microsoft Hyper-V virtual network driver"
depends on HYPERV
select UCS2_STRING
+   select FAILOVER

When I take a working kernel config, add the patches then do
make oldconfig

It is not autoselecting FAILOVER, it prompts me for it. This means
if user says no then a non-working netvsc device is made.

I see
 Generic failover module (FAILOVER) [M/y/?] (NEW)

So the user is given an option to either build as a Module or part of the
kernel. 'n' is not an option.

With most libraries there is no prompt at all.


Not sure what you meant by this.
Without any patches applied, i had a .config file with HYPERV_NET configured
as a module.
Then after applying the first 2 patches in this series, i did a
  make oldconfig
and i see the above prompt.

Are you saying that on some distros, 'make oldconfig creates a .config
file without any prompt and FAILOVER is not getting selected even when 
HYPERV_NET
is enabled?


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Stephen Hemminger]

On Fri, 25 May 2018 16:11:47 -0700
"Samudrala, Sridhar"  wrote:

> On 5/25/2018 3:34 PM, Stephen Hemminger wrote:
> > On Thu, 24 May 2018 09:55:14 -0700
> > Sridhar Samudrala  wrote:
> >  
> >> --- a/drivers/net/hyperv/Kconfig
> >> +++ b/drivers/net/hyperv/Kconfig
> >> @@ -2,5 +2,6 @@ config HYPERV_NET
> >>tristate "Microsoft Hyper-V virtual network driver"
> >>depends on HYPERV
> >>select UCS2_STRING
> >> +  select FAILOVER  
> > When I take a working kernel config, add the patches then do
> > make oldconfig
> >
> > It is not autoselecting FAILOVER, it prompts me for it. This means
> > if user says no then a non-working netvsc device is made.  
> 
> I see
> Generic failover module (FAILOVER) [M/y/?] (NEW)
> 
> So the user is given an option to either build as a Module or part of the
> kernel. 'n' is not an option.

With most libraries there is no prompt at all.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Samudrala, Sridhar]

On 5/25/2018 3:34 PM, Stephen Hemminger wrote:

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:


--- a/drivers/net/hyperv/Kconfig
+++ b/drivers/net/hyperv/Kconfig
@@ -2,5 +2,6 @@ config HYPERV_NET
tristate "Microsoft Hyper-V virtual network driver"
depends on HYPERV
select UCS2_STRING
+   select FAILOVER

When I take a working kernel config, add the patches then do
make oldconfig

It is not autoselecting FAILOVER, it prompts me for it. This means
if user says no then a non-working netvsc device is made.


I see
   Generic failover module (FAILOVER) [M/y/?] (NEW)

So the user is given an option to either build as a Module or part of the
kernel. 'n' is not an option.


___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Stephen Hemminger]

On Thu, 24 May 2018 09:55:14 -0700
Sridhar Samudrala  wrote:

> --- a/drivers/net/hyperv/Kconfig
> +++ b/drivers/net/hyperv/Kconfig
> @@ -2,5 +2,6 @@ config HYPERV_NET
>   tristate "Microsoft Hyper-V virtual network driver"
>   depends on HYPERV
>   select UCS2_STRING
> + select FAILOVER

When I take a working kernel config, add the patches then do
make oldconfig

It is not autoselecting FAILOVER, it prompts me for it. This means
if user says no then a non-working netvsc device is made.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework

[Sridhar Samudrala]

Use the registration/notification framework supported by the generic
failover infrastructure.

Signed-off-by: Sridhar Samudrala 
---
 drivers/net/hyperv/Kconfig  |   1 +
 drivers/net/hyperv/hyperv_net.h |   2 +
 drivers/net/hyperv/netvsc_drv.c | 222 +++-
 3 files changed, 60 insertions(+), 165 deletions(-)

diff --git a/drivers/net/hyperv/Kconfig b/drivers/net/hyperv/Kconfig
index 0765d5f61714..23a2d145813a 100644
--- a/drivers/net/hyperv/Kconfig
+++ b/drivers/net/hyperv/Kconfig
@@ -2,5 +2,6 @@ config HYPERV_NET
tristate "Microsoft Hyper-V virtual network driver"
depends on HYPERV
select UCS2_STRING
+   select FAILOVER
help
  Select this option to enable the Hyper-V virtual network driver.
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 1be34d2e3563..99d8e7398a5b 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -932,6 +932,8 @@ struct net_device_context {
u32 vf_alloc;
/* Serial number of the VF to team with */
u32 vf_serial;
+
+   struct failover *failover;
 };
 
 /* Per channel data */
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index da07ccdf84bf..62eb136c5e73 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -43,6 +43,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "hyperv_net.h"
 
@@ -1763,46 +1764,6 @@ static void netvsc_link_change(struct work_struct *w)
rtnl_unlock();
 }
 
-static struct net_device *get_netvsc_bymac(const u8 *mac)
-{
-   struct net_device *dev;
-
-   ASSERT_RTNL();
-
-   for_each_netdev(_net, dev) {
-   if (dev->netdev_ops != _ops)
-   continue;   /* not a netvsc device */
-
-   if (ether_addr_equal(mac, dev->perm_addr))
-   return dev;
-   }
-
-   return NULL;
-}
-
-static struct net_device *get_netvsc_byref(struct net_device *vf_netdev)
-{
-   struct net_device *dev;
-
-   ASSERT_RTNL();
-
-   for_each_netdev(_net, dev) {
-   struct net_device_context *net_device_ctx;
-
-   if (dev->netdev_ops != _ops)
-   continue;   /* not a netvsc device */
-
-   net_device_ctx = netdev_priv(dev);
-   if (!rtnl_dereference(net_device_ctx->nvdev))
-   continue;   /* device is removed */
-
-   if (rtnl_dereference(net_device_ctx->vf_netdev) == vf_netdev)
-   return dev; /* a match */
-   }
-
-   return NULL;
-}
-
 /* Called when VF is injecting data into network stack.
  * Change the associated network device from VF to netvsc.
  * note: already called with rcu_read_lock
@@ -1825,46 +1786,6 @@ static rx_handler_result_t netvsc_vf_handle_frame(struct 
sk_buff **pskb)
return RX_HANDLER_ANOTHER;
 }
 
-static int netvsc_vf_join(struct net_device *vf_netdev,
- struct net_device *ndev)
-{
-   struct net_device_context *ndev_ctx = netdev_priv(ndev);
-   int ret;
-
-   ret = netdev_rx_handler_register(vf_netdev,
-netvsc_vf_handle_frame, ndev);
-   if (ret != 0) {
-   netdev_err(vf_netdev,
-  "can not register netvsc VF receive handler (err = 
%d)\n",
-  ret);
-   goto rx_handler_failed;
-   }
-
-   ret = netdev_master_upper_dev_link(vf_netdev, ndev,
-  NULL, NULL, NULL);
-   if (ret != 0) {
-   netdev_err(vf_netdev,
-  "can not set master device %s (err = %d)\n",
-  ndev->name, ret);
-   goto upper_link_failed;
-   }
-
-   /* set slave flag before open to prevent IPv6 addrconf */
-   vf_netdev->flags |= IFF_SLAVE;
-
-   schedule_delayed_work(_ctx->vf_takeover, VF_TAKEOVER_INT);
-
-   call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
-
-   netdev_info(vf_netdev, "joined to %s\n", ndev->name);
-   return 0;
-
-upper_link_failed:
-   netdev_rx_handler_unregister(vf_netdev);
-rx_handler_failed:
-   return ret;
-}
-
 static void __netvsc_vf_setup(struct net_device *ndev,
  struct net_device *vf_netdev)
 {
@@ -1915,85 +1836,95 @@ static void netvsc_vf_setup(struct work_struct *w)
rtnl_unlock();
 }
 
-static int netvsc_register_vf(struct net_device *vf_netdev)
+static int netvsc_pre_register_vf(struct net_device *vf_netdev,
+ struct net_device *ndev)
 {
-   struct net_device *ndev;
struct net_device_context *net_device_ctx;
struct netvsc_device *netvsc_dev;
 
-   if (vf_netdev->addr_len != ETH_ALEN)
-   return NOTIFY_DONE;
-
-   /*
-* We will