Re: [PATCH net-next v12 2/5] netvsc: refactor notifier/event handling code to use the failover framework
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
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
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
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
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
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
'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
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
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
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
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
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
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(&init_net, dev) { - if (dev->netdev_ops != &device_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(&init_net, dev) { - struct net_device_context *net_device_ctx; - - if (dev->netdev_ops != &device_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(&ndev_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 use