Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932830AbaGUOHo (ORCPT ); Mon, 21 Jul 2014 10:07:44 -0400 Received: from mail-yk0-f171.google.com ([209.85.160.171]:63388 "EHLO mail-yk0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932406AbaGUOHJ (ORCPT ); Mon, 21 Jul 2014 10:07:09 -0400 MIME-Version: 1.0 X-Originating-IP: [188.151.160.142] In-Reply-To: References: <1405680903-28176-1-git-send-email-yuezha@microsoft.com> From: Tom Gundersen Date: Mon, 21 Jul 2014 16:06:48 +0200 Message-ID: Subject: Re: [PATCH] Hyperv: Trigger DHCP renew after host hibernation To: "Yue Zhang (OSTC DEV)" Cc: netdev , "driverdev-devel@linuxdriverproject.org" , LKML , Greg KH , "olaf@aepfle.de" , "jasowang@redhat.com" , David Miller , Haiyang Zhang , KY Srinivasan , Thomas Shao , Dexuan Cui , Lennart Poettering Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 21, 2014 at 12:21 PM, Yue Zhang (OSTC DEV) wrote: >> From: Tom Gundersen [mailto:teg@jklm.no] >> Sent: Monday, July 21, 2014 5:42 PM >> >> On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang wrote: >> > From: Yue Zhang >> > >> > This patch addresses the comment from Olaf Hering and Greg KH >> > for a previous commit 3a494e710367 ("hyperv: Add handler for >> > RNDIS_STATUS_NETWORK_CHANGE event") >> > >> > In previous solution, the driver calls "network restart" to >> > force a DHCP renew when the host is back from hibernation. >> > >> > In this fix, the driver will keep network carrier offline for >> > 10 seconds and then bring it back. So that ifplugd daemon will >> > notice this change and refresh DHCP lease. >> > >> > Cc: Haiyang Zhang >> > Cc: K. Y. Srinivasan >> > >> > Signed-off-by: Yue Zhang >> > --- >> > drivers/net/hyperv/netvsc_drv.c | 21 +++++++++++++++++---- >> > 1 file changed, 17 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/net/hyperv/netvsc_drv.c >> b/drivers/net/hyperv/netvsc_drv.c >> > index a9c5eaa..559c97d 100644 >> > --- a/drivers/net/hyperv/netvsc_drv.c >> > +++ b/drivers/net/hyperv/netvsc_drv.c >> > @@ -33,6 +33,7 @@ >> > #include >> > #include >> > #include >> > +#include >> > #include >> > #include >> > #include >> > @@ -792,8 +793,7 @@ static void netvsc_link_change(struct work_struct >> *w) >> > struct netvsc_device *net_device; >> > struct rndis_device *rdev; >> > bool notify, refresh = false; >> > - char *argv[] = { "/etc/init.d/network", "restart", NULL }; >> > - char *envp[] = { "HOME=/", "PATH=/sbin:/usr/sbin:/bin:/usr/bin", >> NULL }; >> > + int delay; >> > >> > rtnl_lock(); >> > >> > @@ -816,8 +816,21 @@ static void netvsc_link_change(struct work_struct >> *w) >> > >> > rtnl_unlock(); >> > >> > - if (refresh) >> > - call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC); >> > + if (refresh) { >> > + /* >> > + * Keep the carrier offline for 10 seconds >> > + * to notify ifplugd daemon network change >> > + */ >> > + for (delay = 0; delay < 10; delay++) { >> > + rtnl_lock(); >> > + netif_carrier_off(net); >> > + rtnl_unlock(); >> > + ssleep(1); >> > + } >> > + rtnl_lock(); >> > + netif_carrier_on(net); >> > + rtnl_unlock(); >> > + } >> >> Why is it necessary to wait for ten seconds? Why not just: >> >> if (refresh) { >> rtnl_lock(); >> netif_carrier_off(net); >> netif_carrier_on(net); >> rtnl_unlock(); >> } >> >> At least systemd-networkd will renew the dhcp lease as long as it gets >> NEWLINK messages indicating that the carrier was lost and regained, >> regardless of the time between events. Is there any reason not to do >> this? >> >> Cheers, >> >> Tom >> > > Hi, Tom > > Some network monitoring daemon, like ifplugd has a deferring mechanism. > When it detects carriers is offline, it doesn't trigger DHCP renew immediately. > Instead it will wait for another 5 seconds to check whether carrier is back to > online status. In that case, it will avoid renew DHCP lease. > > And also there is some optimization in Linux's network stack. If link state > flipped so quickly, like the code you proposed. It is very likely the event won't > be delivered to user space. Ah, ok, I don't know the kernel side of this too well, you may need some sort of flush or sync between the calls I suggested. At any rate, I would say that the solution should be to send a "lower down" followed immediately by "lower up" and never have any sort of timeout. All the drivers I have tried send out such events immediately when the machine is resumed, so I guess most network software should know how to deal with that. I really think the policy of what to do in response to the various events should be left to userspace to figure out. Cheers, Tom -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/