Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1324702 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE Haiyang Zhang <haiyangz@microsoft.com> - 2016-02-02 23:50 +0100
    Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-02-03 14:10 +0100
      RE: [PATCH net-next] hv_netvsc: Increase delay for  RNDIS_STATUS_NETWORK_CHANGE Haiyang Zhang <haiyangz@microsoft.com> - 2016-02-03 17:10 +0100
        Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-02-03 17:10 +0100
          RE: [PATCH net-next] hv_netvsc: Increase delay for  RNDIS_STATUS_NETWORK_CHANGE Haiyang Zhang <haiyangz@microsoft.com> - 2016-02-03 17:50 +0100

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

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

[toc] | [next] | [standalone]

#1325394

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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [next] | [standalone]

#1325594

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

[toc] | [prev] | [next] | [standalone]

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

> -----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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web