[PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Haiyang Zhang]

We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE message to
trigger DHCP renew. User daemons may need multiple seconds to trigger the
link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
this link down period to 10 sec to properly trigger DHCP renew.

Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
 drivers/net/hyperv/netvsc_drv.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 1d3a665..6f23973 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -43,6 +43,8 @@
 
 #define RING_SIZE_MIN 64
 #define LINKCHANGE_INT (2 * HZ)
+/* Extra delay for RNDIS_STATUS_NETWORK_CHANGE: */
+#define LINKCHANGE_DELAY (8 * HZ)
 static int ring_size = 128;
 module_param(ring_size, int, S_IRUGO);
 MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)");
@@ -964,6 +966,7 @@ static void netvsc_link_change(struct work_struct *w)
 		return;
 	}
 	ndev_ctx->last_reconfig = jiffies;
+	delay = LINKCHANGE_INT;
 
 	spin_lock_irqsave(&ndev_ctx->lock, flags);
 	if (!list_empty(&ndev_ctx->reconfig_events)) {
@@ -1009,8 +1012,11 @@ static void netvsc_link_change(struct work_struct *w)
 			netif_tx_stop_all_queues(net);
 			event->event = RNDIS_STATUS_MEDIA_CONNECT;
 			spin_lock_irqsave(&ndev_ctx->lock, flags);
-			list_add_tail(&event->list, &ndev_ctx->reconfig_events);
+			list_add(&event->list, &ndev_ctx->reconfig_events);
 			spin_unlock_irqrestore(&ndev_ctx->lock, flags);
+
+			ndev_ctx->last_reconfig += LINKCHANGE_DELAY;
+			delay = LINKCHANGE_INT + LINKCHANGE_DELAY;
 			reschedule = true;
 		}
 		break;
@@ -1025,7 +1031,7 @@ static void netvsc_link_change(struct work_struct *w)
 	 * second, handle next reconfig event in 2 seconds.
 	 */
 	if (reschedule)
-		schedule_delayed_work(&ndev_ctx->dwork, LINKCHANGE_INT);
+		schedule_delayed_work(&ndev_ctx->dwork, delay);
 }
 
 static void netvsc_free_netdev(struct net_device *netdev)
-- 
1.7.4.1

Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Vitaly Kuznetsov]

Haiyang Zhang <haiyangz@microsoft.com> writes:

> We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE message to
> trigger DHCP renew. User daemons may need multiple seconds to trigger the
> link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
> this link down period to 10 sec to properly trigger DHCP renew.
>

I probably don't follow: why do we need sucha a delay? If (with real
hardware) you plug network cable out and in one second you plug it in
you'll get DHCP renewed, right?

When I introduced RNDIS_STATUS_NETWORK_CHANGE handling by emulating a
pair of up/down events I put 2 second delay to make link_watch happy (as
we only handle 1 event per second there) but 10 seconds sounds to much
to me.

> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> ---
>  drivers/net/hyperv/netvsc_drv.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
> index 1d3a665..6f23973 100644
> --- a/drivers/net/hyperv/netvsc_drv.c
> +++ b/drivers/net/hyperv/netvsc_drv.c
> @@ -43,6 +43,8 @@
>
>  #define RING_SIZE_MIN 64
>  #define LINKCHANGE_INT (2 * HZ)
> +/* Extra delay for RNDIS_STATUS_NETWORK_CHANGE: */
> +#define LINKCHANGE_DELAY (8 * HZ)
>  static int ring_size = 128;
>  module_param(ring_size, int, S_IRUGO);
>  MODULE_PARM_DESC(ring_size, "Ring buffer size (# of pages)");
> @@ -964,6 +966,7 @@ static void netvsc_link_change(struct work_struct *w)
>  		return;
>  	}
>  	ndev_ctx->last_reconfig = jiffies;
> +	delay = LINKCHANGE_INT;
>
>  	spin_lock_irqsave(&ndev_ctx->lock, flags);
>  	if (!list_empty(&ndev_ctx->reconfig_events)) {
> @@ -1009,8 +1012,11 @@ static void netvsc_link_change(struct work_struct *w)
>  			netif_tx_stop_all_queues(net);
>  			event->event = RNDIS_STATUS_MEDIA_CONNECT;
>  			spin_lock_irqsave(&ndev_ctx->lock, flags);
> -			list_add_tail(&event->list, &ndev_ctx->reconfig_events);
> +			list_add(&event->list, &ndev_ctx->reconfig_events);

Why? Adding to tail was here to not screw the order of events...

>  			spin_unlock_irqrestore(&ndev_ctx->lock, flags);
> +
> +			ndev_ctx->last_reconfig += LINKCHANGE_DELAY;
> +			delay = LINKCHANGE_INT + LINKCHANGE_DELAY;
>  			reschedule = true;
>  		}
>  		break;
> @@ -1025,7 +1031,7 @@ static void netvsc_link_change(struct work_struct *w)
>  	 * second, handle next reconfig event in 2 seconds.
>  	 */
>  	if (reschedule)
> -		schedule_delayed_work(&ndev_ctx->dwork, LINKCHANGE_INT);
> +		schedule_delayed_work(&ndev_ctx->dwork, delay);
>  }
>
>  static void netvsc_free_netdev(struct net_device *netdev)

-- 
  Vitaly

RE: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, February 3, 2016 8:06 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
> RNDIS_STATUS_NETWORK_CHANGE
> 
> Haiyang Zhang <haiyangz@microsoft.com> writes:
> 
> > We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE
> message to
> > trigger DHCP renew. User daemons may need multiple seconds to trigger
> the
> > link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
> > this link down period to 10 sec to properly trigger DHCP renew.
> >
> 
> I probably don't follow: why do we need sucha a delay? If (with real
> hardware) you plug network cable out and in one second you plug it in
> you'll get DHCP renewed, right?
> 
> When I introduced RNDIS_STATUS_NETWORK_CHANGE handling by
> emulating a
> pair of up/down events I put 2 second delay to make link_watch happy (as
> we only handle 1 event per second there) but 10 seconds sounds to much
> to me.

In the test on Hyper-V, our software on host side  wants to trigger DHCP 
renew by sending only a RNDIS_STATUS_NETWORK_CHANGE message to 
a guest without physically unplug the cable. But, this message didn't trigger 
DHCP renew within 2 seconds. The VM is Centos 7.1 using Networkmanager, 
which needs 4 seconds after link down to renew IP. Some daemon, like 
ifplugd, needs 5 sec to renew. That's why we increase the simulated link 
down time for RNDIS_STATUS_NETWORK_CHANGE message. 


> > @@ -1009,8 +1012,11 @@ static void netvsc_link_change(struct
> work_struct *w)
> >  			netif_tx_stop_all_queues(net);
> >  			event->event = RNDIS_STATUS_MEDIA_CONNECT;
> >  			spin_lock_irqsave(&ndev_ctx->lock, flags);
> > -			list_add_tail(&event->list, &ndev_ctx-
> >reconfig_events);
> > +			list_add(&event->list, &ndev_ctx->reconfig_events);
> 
> Why? Adding to tail was here to not screw the order of events...

The RNDIS_STATUS_MEDIA_CONNECT message triggers two "half" events -- 
link down & up. After "link down", we want the paired "link up" to be the 
immediate next event. Since the function picks the next event from the list 
head, so it should be inserted to list head. Otherwise, the possible existing 
events in the list will be processed in the middle of these two "half events" 
-- link down & up.

Thanks,
- Haiyang

Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Vitaly Kuznetsov]

Haiyang Zhang <haiyangz@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Wednesday, February 3, 2016 8:06 AM
>> To: Haiyang Zhang <haiyangz@microsoft.com>
>> Cc: davem@davemloft.net; netdev@vger.kernel.org; KY Srinivasan
>> <kys@microsoft.com>; olaf@aepfle.de; linux-kernel@vger.kernel.org;
>> driverdev-devel@linuxdriverproject.org
>> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
>> RNDIS_STATUS_NETWORK_CHANGE
>> 
>> Haiyang Zhang <haiyangz@microsoft.com> writes:
>> 
>> > We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE
>> message to
>> > trigger DHCP renew. User daemons may need multiple seconds to trigger
>> the
>> > link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
>> > this link down period to 10 sec to properly trigger DHCP renew.
>> >
>> 
>> I probably don't follow: why do we need sucha a delay? If (with real
>> hardware) you plug network cable out and in one second you plug it in
>> you'll get DHCP renewed, right?
>> 
>> When I introduced RNDIS_STATUS_NETWORK_CHANGE handling by
>> emulating a
>> pair of up/down events I put 2 second delay to make link_watch happy (as
>> we only handle 1 event per second there) but 10 seconds sounds to much
>> to me.
>
> In the test on Hyper-V, our software on host side  wants to trigger DHCP 
> renew by sending only a RNDIS_STATUS_NETWORK_CHANGE message to 
> a guest without physically unplug the cable. But, this message didn't trigger 
> DHCP renew within 2 seconds. The VM is Centos 7.1 using Networkmanager, 
> which needs 4 seconds after link down to renew IP. Some daemon, like 
> ifplugd, needs 5 sec to renew. That's why we increase the simulated link 
> down time for RNDIS_STATUS_NETWORK_CHANGE message.

Yes, I understand the motivation but sorry if I was unclear with my
question. I meant to say that with physical network adapters it's
possible to trigger same two events by plugging your cable out and then
plugging it back in and it is certailnly doable in less than 10
seconds. NetworkManager or whoever is supposed to handle these events
and we don't really care how fast -- I think that 4 or 5 seconds
mentioned above is just an observation.

>
>> > @@ -1009,8 +1012,11 @@ static void netvsc_link_change(struct
>> work_struct *w)
>> >  			netif_tx_stop_all_queues(net);
>> >  			event->event = RNDIS_STATUS_MEDIA_CONNECT;
>> >  			spin_lock_irqsave(&ndev_ctx->lock, flags);
>> > -			list_add_tail(&event->list, &ndev_ctx-
>> >reconfig_events);
>> > +			list_add(&event->list, &ndev_ctx->reconfig_events);
>> 
>> Why? Adding to tail was here to not screw the order of events...
>
> The RNDIS_STATUS_MEDIA_CONNECT message triggers two "half" events -- 
> link down & up. After "link down", we want the paired "link up" to be the 
> immediate next event. Since the function picks the next event from the list 
> head, so it should be inserted to list head. Otherwise, the possible existing 
> events in the list will be processed in the middle of these two "half events" 
> -- link down & up.

Ah, yes, you're probably right.

-- 
  Vitaly

RE: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, February 3, 2016 11:06 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
> RNDIS_STATUS_NETWORK_CHANGE
> 
> Haiyang Zhang <haiyangz@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Wednesday, February 3, 2016 8:06 AM
> >> To: Haiyang Zhang <haiyangz@microsoft.com>
> >> Cc: davem@davemloft.net; netdev@vger.kernel.org; KY Srinivasan
> >> <kys@microsoft.com>; olaf@aepfle.de; linux-kernel@vger.kernel.org;
> >> driverdev-devel@linuxdriverproject.org
> >> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
> >> RNDIS_STATUS_NETWORK_CHANGE
> >>
> >> Haiyang Zhang <haiyangz@microsoft.com> writes:
> >>
> >> > We simulates a link down period for
> RNDIS_STATUS_NETWORK_CHANGE
> >> message to
> >> > trigger DHCP renew. User daemons may need multiple seconds to
> trigger
> >> the
> >> > link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
> >> > this link down period to 10 sec to properly trigger DHCP renew.
> >> >
> >>
> >> I probably don't follow: why do we need sucha a delay? If (with real
> >> hardware) you plug network cable out and in one second you plug it in
> >> you'll get DHCP renewed, right?
> >>
> >> When I introduced RNDIS_STATUS_NETWORK_CHANGE handling by
> >> emulating a
> >> pair of up/down events I put 2 second delay to make link_watch happy
> (as
> >> we only handle 1 event per second there) but 10 seconds sounds to much
> >> to me.
> >
> > In the test on Hyper-V, our software on host side  wants to trigger DHCP
> > renew by sending only a RNDIS_STATUS_NETWORK_CHANGE message to
> > a guest without physically unplug the cable. But, this message didn't trigger
> > DHCP renew within 2 seconds. The VM is Centos 7.1 using
> Networkmanager,
> > which needs 4 seconds after link down to renew IP. Some daemon, like
> > ifplugd, needs 5 sec to renew. That's why we increase the simulated link
> > down time for RNDIS_STATUS_NETWORK_CHANGE message.
> 
> Yes, I understand the motivation but sorry if I was unclear with my
> question. I meant to say that with physical network adapters it's
> possible to trigger same two events by plugging your cable out and then
> plugging it back in and it is certailnly doable in less than 10
> seconds. NetworkManager or whoever is supposed to handle these events
> and we don't really care how fast -- I think that 4 or 5 seconds
> mentioned above is just an observation.

(forgot mailing lists in my last reply.... re-sending...)

Our test failed (i.e. not triggering DHCP renew) with existing 2sec delay. According 
to the ifplugd man page, it ignores link down time <5sec:
  http://linux.die.net/man/8/ifplugd
    -d | --delay-down= SECS Specify delay for deconfiguring interface (default: 5)

Networkmanager also has a waiting period (4sec).

Thanks,
- Haiyang

Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Tue,  2 Feb 2016 16:15:56 -0800

> We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE message to
> trigger DHCP renew. User daemons may need multiple seconds to trigger the
> link down event. (e.g. ifplugd: 5sec, network-manager: 4sec.) So update
> this link down period to 10 sec to properly trigger DHCP renew.
> 
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>

Two things look really bad about this to me:

1) Any value you choose is arbitrary.  If some new network configuration daemon
   is slower, you will have to change this value again.

   This is _NOT_ sustainable in the long term.

2) It is completely unclear to me why this driver needs to delay at all or
   wait for anything.  I see no other driver having to deal with this issue.

Until you address both of these points I am not going to apply this patch.

Thanks.

RE: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Haiyang Zhang]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Tuesday, February 9, 2016 5:05 AM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> olaf@aepfle.de; vkuznets@redhat.com; linux-kernel@vger.kernel.org;
> driverdev-devel@linuxdriverproject.org
> Subject: Re: [PATCH net-next] hv_netvsc: Increase delay for
> RNDIS_STATUS_NETWORK_CHANGE
> 
> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Tue,  2 Feb 2016 16:15:56 -0800
> 
> > We simulates a link down period for RNDIS_STATUS_NETWORK_CHANGE
> > message to trigger DHCP renew. User daemons may need multiple seconds
> > to trigger the link down event. (e.g. ifplugd: 5sec, network-manager:
> > 4sec.) So update this link down period to 10 sec to properly trigger DHCP
> renew.
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> 
> Two things look really bad about this to me:
> 
> 1) Any value you choose is arbitrary.  If some new network configuration
> daemon
>    is slower, you will have to change this value again.
> 
>    This is _NOT_ sustainable in the long term.
> 
> 2) It is completely unclear to me why this driver needs to delay at all or
>    wait for anything.  I see no other driver having to deal with this issue.
> 
> Until you address both of these points I am not going to apply this patch.

1) I share your concern as well. Is there a universal way to immediately trigger 
DHCP renew of all current and future daemons with a single event from kernel? 
If not, can we put the delay (RNDIS_STATUS_NETWORK_CHANGE only) into a 
tunable variable of this driver?

2) We used to have the call_usermodehelper "/etc/init.d/network restart" to 
trigger DHCP renew. In commit 27a70af3f4, Vitaly has replaced it with the current 
code that updates the link status with at least 2 seconds interval, so that the 
"link_watch infrastructure" can send notification out. link_watch infrastructure 
only sends one notification per second.

Thanks,
- Haiyang

Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[David Miller]

From: Haiyang Zhang <haiyangz@microsoft.com>
Date: Tue, 9 Feb 2016 15:31:34 +0000

> 1) I share your concern as well. Is there a universal way to immediately trigger 
> DHCP renew of all current and future daemons with a single event from kernel? 
> If not, can we put the delay (RNDIS_STATUS_NETWORK_CHANGE only) into a 
> tunable variable of this driver?
> 
> 2) We used to have the call_usermodehelper "/etc/init.d/network restart" to 
> trigger DHCP renew. In commit 27a70af3f4, Vitaly has replaced it with the current 
> code that updates the link status with at least 2 seconds interval, so that the 
> "link_watch infrastructure" can send notification out. link_watch infrastructure 
> only sends one notification per second.

If the daemon is waiting for the link state change properly, there should be
no delay necessary at all.

Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE

[Vitaly Kuznetsov]

David Miller <davem@davemloft.net> writes:

> From: Haiyang Zhang <haiyangz@microsoft.com>
> Date: Tue, 9 Feb 2016 15:31:34 +0000
>
>> 1) I share your concern as well. Is there a universal way to immediately trigger 
>> DHCP renew of all current and future daemons with a single event from kernel? 
>> If not, can we put the delay (RNDIS_STATUS_NETWORK_CHANGE only) into a 
>> tunable variable of this driver?
>> 
>> 2) We used to have the call_usermodehelper "/etc/init.d/network restart" to 
>> trigger DHCP renew. In commit 27a70af3f4, Vitaly has replaced it with the current 
>> code that updates the link status with at least 2 seconds interval, so that the 
>> "link_watch infrastructure" can send notification out. link_watch infrastructure 
>> only sends one notification per second.
>
> If the daemon is waiting for the link state change properly, there should be
> no delay necessary at all.

The daemon won't get 2 state change notifications if they happen within
1 second, it will get the last state only so our link will transition
from 'UP' to 'UP'. Why is that? To signal link state change we call
netif_carrier_on()/netif_carrier_off() from the driver. These functions
do linkwatch_fire_event() which has the following code for non-urgent events:

        if (!test_and_set_bit(__LINK_STATE_LINKWATCH_PENDING, &dev->state)) {
		linkwatch_add_event(dev);
	} else if (!urgent)
		return;

	linkwatch_schedule_work(urgent);

So we'll add just one event (because of test_and_set_bit) for a pair of
consequent netif_carrier_off()/netif_carrier_on() calls.

linkwatch_schedule_work() does the following:
	unsigned long delay = linkwatch_nextevent - jiffies;

        ....

	/* If we wrap around we'll delay it by at most HZ. */
	if (delay > HZ)
		delay = 0;

so here is where mandatory ' > 1s' wait comes from.
        ....
        schedule_delayed_work(&linkwatch_work, delay);        

linkwatch_work is linkwatch_event() which calls __linkwatch_run_queue()
which does linkwatch_do_dev() for the list of events we have. But
linkwatch_do_dev() checks current carrier status (which in our case if
'UP' if we didn't wait for > 1s before doing /netif_carrier_on()).

Hyper-V driver is not the only one which has this delay. e1000 driver,
for example, has the following:

...
 * Need to wait a few seconds after link up to get diagnostic information from
 * the phy
 */
static void e1000_update_phy_info_task(struct work_struct *work)
...

which we schedule with
      schedule_delayed_work(&adapter->phy_info_task, 2 * HZ);

To my understanding this code serves the same purpose, so even if you're
super fast with unplugging and plugging back your cable you'll have 2
seconds between 'down' and 'up'. I don't think there is something wrong
with linkwatch, the '1s' protection against drivers going mad is fine so
'2s' workarounds in drivers seem legit.

-- 
  Vitaly