| Message ID | 20171220223323.21125-1-sthemmin@microsoft.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | hv_netvsc: update VF after name has changed. | expand |
From: Stephen Hemminger <stephen@networkplumber.org> Date: Wed, 20 Dec 2017 14:33:23 -0800 > Since commit 6123c66854c1 ("netvsc: delay setup of VF device") > the automatic bring up of the VF is delayed to allow userspace (udev) > a chance to rename the device. This delay is problematic because > it delays boot and may not be long enough for some cases. > > Instead, use the rename can be used to trigger the next step > in setup to happen immediately. > > The VF initialization sequence now looks like: > * hotplug causes VF network device probe to create network device > * netvsc notifier joins VF with netvsc and schedules VF to be > setup after timer expires > * udev in userspace renames device > * if netvsc notifier detects rename, it can cancel timer > and do immediate setup > > The delay can also be increased to allow for slower rules. > Still need the delayed work to handle the case where rename is > not done. > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> I'm still seriously perplexed by this whole situation. Why can't you bring up a VF interface simply because of a text string used to reffered to it? If the VF interface were named "rabbit" I don't see why it can't be brought up until it is named "tiger" I'm not applying any patches in this area until we have a better understanding of what the real issue is, and have a real solution that can be applied to any other VF interface implemented in the tree.
On Tue, 26 Dec 2017 12:25:12 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Wed, 20 Dec 2017 14:33:23 -0800 > > > Since commit 6123c66854c1 ("netvsc: delay setup of VF device") > > the automatic bring up of the VF is delayed to allow userspace (udev) > > a chance to rename the device. This delay is problematic because > > it delays boot and may not be long enough for some cases. > > > > Instead, use the rename can be used to trigger the next step > > in setup to happen immediately. > > > > The VF initialization sequence now looks like: > > * hotplug causes VF network device probe to create network device > > * netvsc notifier joins VF with netvsc and schedules VF to be > > setup after timer expires > > * udev in userspace renames device > > * if netvsc notifier detects rename, it can cancel timer > > and do immediate setup > > > > The delay can also be increased to allow for slower rules. > > Still need the delayed work to handle the case where rename is > > not done. > > > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > > I'm still seriously perplexed by this whole situation. See state diagram below > Why can't you bring up a VF interface simply because of a text string > used to reffered to it? VF network device can not have its name changed if up. The network device kernel API does not allow name change of network device that is up. There was a patch from Vitaly to allow this, but it was deemed to be too risky. https://patchwork.ozlabs.org/patch/799646/ > If the VF interface were named "rabbit" I don't see why it can't be > brought up until it is named "tiger" > > I'm not applying any patches in this area until we have a better > understanding of what the real issue is, and have a real solution > that can be applied to any other VF interface implemented in the > tree. Netvsc VF Network Device Device | | | | udev network daemon | +<------------- PCI hotplug | NETDEV_REGISTER | + +<----------------------+ register_netdev | | | | | | +---------------------->+ | | netdev_rx_handler | | | | | +---------------------> | | | netdev_upper_dev_link | +<------------------+ | | udev device add | | | | | | | | | | | +-------------------+---------------------> | | rename eth1 to enp12s0 | | | NETDEV_CHANGENAME | | <-----------------------+ | | | | | | | +---------------------> | | | dev_set_mtu | | | | | +---------------------> | | | dev_open | | | | | | NETDEV_UP | | <-----------------------+ v | | | | | | +----------------------------------> Hyper-V host | RNDIS switch datapath | | v v
From: Stephen Hemminger <stephen@networkplumber.org> Date: Tue, 26 Dec 2017 13:51:43 -0800 > On Tue, 26 Dec 2017 12:25:12 -0500 (EST) > David Miller <davem@davemloft.net> wrote: > >> From: Stephen Hemminger <stephen@networkplumber.org> >> Date: Wed, 20 Dec 2017 14:33:23 -0800 >> >> > Since commit 6123c66854c1 ("netvsc: delay setup of VF device") >> > the automatic bring up of the VF is delayed to allow userspace (udev) >> > a chance to rename the device. This delay is problematic because >> > it delays boot and may not be long enough for some cases. >> > >> > Instead, use the rename can be used to trigger the next step >> > in setup to happen immediately. >> > >> > The VF initialization sequence now looks like: >> > * hotplug causes VF network device probe to create network device >> > * netvsc notifier joins VF with netvsc and schedules VF to be >> > setup after timer expires >> > * udev in userspace renames device >> > * if netvsc notifier detects rename, it can cancel timer >> > and do immediate setup >> > >> > The delay can also be increased to allow for slower rules. >> > Still need the delayed work to handle the case where rename is >> > not done. >> > >> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> >> >> I'm still seriously perplexed by this whole situation. > > See state diagram below > >> Why can't you bring up a VF interface simply because of a text string >> used to reffered to it? > > VF network device can not have its name changed if up. > The network device kernel API does not allow name change of network > device that is up. There was a patch from Vitaly to allow this, but it > was deemed to be too risky. > > https://patchwork.ozlabs.org/patch/799646/ Thank you for the state diagram. I still, unfortunately, don't see the problem. If userland changes the name of the VF device, then they will change it before bringing the VF device up. Thanks for your patience with me.
On Tue, 26 Dec 2017 17:11:19 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Tue, 26 Dec 2017 13:51:43 -0800 > > > On Tue, 26 Dec 2017 12:25:12 -0500 (EST) > > David Miller <davem@davemloft.net> wrote: > > > >> From: Stephen Hemminger <stephen@networkplumber.org> > >> Date: Wed, 20 Dec 2017 14:33:23 -0800 > >> > >> > Since commit 6123c66854c1 ("netvsc: delay setup of VF device") > >> > the automatic bring up of the VF is delayed to allow userspace (udev) > >> > a chance to rename the device. This delay is problematic because > >> > it delays boot and may not be long enough for some cases. > >> > > >> > Instead, use the rename can be used to trigger the next step > >> > in setup to happen immediately. > >> > > >> > The VF initialization sequence now looks like: > >> > * hotplug causes VF network device probe to create network device > >> > * netvsc notifier joins VF with netvsc and schedules VF to be > >> > setup after timer expires > >> > * udev in userspace renames device > >> > * if netvsc notifier detects rename, it can cancel timer > >> > and do immediate setup > >> > > >> > The delay can also be increased to allow for slower rules. > >> > Still need the delayed work to handle the case where rename is > >> > not done. > >> > > >> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > >> > >> I'm still seriously perplexed by this whole situation. > > > > See state diagram below > > > >> Why can't you bring up a VF interface simply because of a text string > >> used to reffered to it? > > > > VF network device can not have its name changed if up. > > The network device kernel API does not allow name change of network > > device that is up. There was a patch from Vitaly to allow this, but it > > was deemed to be too risky. > > > > https://patchwork.ozlabs.org/patch/799646/ > > Thank you for the state diagram. > > I still, unfortunately, don't see the problem. > > If userland changes the name of the VF device, then they will change > it before bringing the VF device up. The requirement is to support several unmodified naive distributions. All of these expect eth0 to be present and should not require configuration or manipulation of the VF network device. This is why the bonding script method never worked. Userland is not responsible for bringing up the VF device. Most distributions leave unknown devices alone, some use network manager which brings up everything it sees. The netvsc device is responsible for having the state of the VF device track the state of the master (netvsc) device. > > Thanks for your patience with me. No worries, this is not my favorite way of doing this either. The pure VF method used by some other cloud providers is easier for userspace; but changing the host (and the hardware FPGA) is not an option. This all just trying to work out something practical with the API's available.
On Tue, 26 Dec 2017 17:11:19 -0500 (EST) David Miller <davem@davemloft.net> wrote: > From: Stephen Hemminger <stephen@networkplumber.org> > Date: Tue, 26 Dec 2017 13:51:43 -0800 > > > On Tue, 26 Dec 2017 12:25:12 -0500 (EST) > > David Miller <davem@davemloft.net> wrote: > > > >> From: Stephen Hemminger <stephen@networkplumber.org> > >> Date: Wed, 20 Dec 2017 14:33:23 -0800 > >> > >> > Since commit 6123c66854c1 ("netvsc: delay setup of VF device") > >> > the automatic bring up of the VF is delayed to allow userspace (udev) > >> > a chance to rename the device. This delay is problematic because > >> > it delays boot and may not be long enough for some cases. > >> > > >> > Instead, use the rename can be used to trigger the next step > >> > in setup to happen immediately. > >> > > >> > The VF initialization sequence now looks like: > >> > * hotplug causes VF network device probe to create network device > >> > * netvsc notifier joins VF with netvsc and schedules VF to be > >> > setup after timer expires > >> > * udev in userspace renames device > >> > * if netvsc notifier detects rename, it can cancel timer > >> > and do immediate setup > >> > > >> > The delay can also be increased to allow for slower rules. > >> > Still need the delayed work to handle the case where rename is > >> > not done. > >> > > >> > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> > >> > >> I'm still seriously perplexed by this whole situation. > > > > See state diagram below > > > >> Why can't you bring up a VF interface simply because of a text string > >> used to reffered to it? > > > > VF network device can not have its name changed if up. > > The network device kernel API does not allow name change of network > > device that is up. There was a patch from Vitaly to allow this, but it > > was deemed to be too risky. > > > > https://patchwork.ozlabs.org/patch/799646/ > > Thank you for the state diagram. > > I still, unfortunately, don't see the problem. > > If userland changes the name of the VF device, then they will change > it before bringing the VF device up. > > Thanks for your patience with me. Coming back to this after meltdown/spectre hiatus... The patch optimizes the boot time by getting rid of the delay in bringing up VF network device. The current upstream code has the netvsc driver bring up VF device under the covers after 100ms. As you observed, this sucks; 100ms is a compromise value. The timeout is too short to ensure that udev rename rule has been done by then. With some underpowered machines (or really slow storage) it can take much longer than that. Conversely, many machines put out lots of packets during boot process and until the VF is brought up the packets have to go over the slow path. Perhaps the timeout should be made longer, but impossible to predict what the right value is. That was why the patch used an additional state transistion to short circuit it.
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index c5584c2d440e..7771d6c54b55 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -49,7 +49,7 @@ #define RING_SIZE_MIN 64 #define LINKCHANGE_INT (2 * HZ) -#define VF_TAKEOVER_INT (HZ / 10) +#define VF_TAKEOVER_INT (HZ / 4) static unsigned int ring_size __ro_after_init = 128; module_param(ring_size, uint, S_IRUGO); @@ -1894,6 +1894,26 @@ static int netvsc_vf_changed(struct net_device *vf_netdev) return NOTIFY_OK; } +/* If VF was renamed, then udev (or other tool) has done its + * work and the VF can be brought up. + */ +static int netvsc_vf_renamed(struct net_device *vf_netdev) +{ + struct net_device_context *ndev_ctx; + struct net_device *ndev; + + ndev = get_netvsc_byref(vf_netdev); + if (!ndev) + return NOTIFY_DONE; + + ndev_ctx = netdev_priv(ndev); + + if (cancel_delayed_work(&ndev_ctx->vf_takeover)) + __netvsc_vf_setup(ndev, vf_netdev); + + return NOTIFY_OK; +} + static int netvsc_unregister_vf(struct net_device *vf_netdev) { struct net_device *ndev; @@ -2110,6 +2130,8 @@ static int netvsc_netdev_event(struct notifier_block *this, case NETDEV_UP: case NETDEV_DOWN: return netvsc_vf_changed(event_dev); + case NETDEV_CHANGENAME: + return netvsc_vf_renamed(event_dev); default: return NOTIFY_DONE; }
Since commit 6123c66854c1 ("netvsc: delay setup of VF device") the automatic bring up of the VF is delayed to allow userspace (udev) a chance to rename the device. This delay is problematic because it delays boot and may not be long enough for some cases. Instead, use the rename can be used to trigger the next step in setup to happen immediately. The VF initialization sequence now looks like: * hotplug causes VF network device probe to create network device * netvsc notifier joins VF with netvsc and schedules VF to be setup after timer expires * udev in userspace renames device * if netvsc notifier detects rename, it can cancel timer and do immediate setup The delay can also be increased to allow for slower rules. Still need the delayed work to handle the case where rename is not done. Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com> --- drivers/net/hyperv/netvsc_drv.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)