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

Groups > linux.kernel > #1330016 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH net-next] hv_netvsc: Increase delay for  RNDIS_STATUS_NETWORK_CHANGE David Miller <davem@davemloft.net> - 2016-02-09 11:10 +0100
    RE: [PATCH net-next] hv_netvsc: Increase delay for  RNDIS_STATUS_NETWORK_CHANGE Haiyang Zhang <haiyangz@microsoft.com> - 2016-02-09 16:50 +0100
      Re: [PATCH net-next] hv_netvsc: Increase delay for  RNDIS_STATUS_NETWORK_CHANGE David Miller <davem@davemloft.net> - 2016-02-16 21:30 +0100
        Re: [PATCH net-next] hv_netvsc: Increase delay for RNDIS_STATUS_NETWORK_CHANGE Vitaly Kuznetsov <vkuznets@redhat.com> - 2016-02-17 14:00 +0100

#1330016 — 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.

Thanks.

[toc] | [next] | [standalone]

#1330374

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

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

#1335797

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.

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web