On Mon, Jul 21, 2014 at 12:21 PM, Yue Zhang (OSTC DEV) <yuezha@xxxxxxxxxxxxx> wrote: >> From: Tom Gundersen [mailto:teg@xxxxxxx] >> Sent: Monday, July 21, 2014 5:42 PM >> >> On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <yuezha@xxxxxxxxxxxxx> wrote: >> > From: Yue Zhang <yuezha@xxxxxxxxxxxxx> >> > >> > 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 <haiyangz@xxxxxxxxxxxxx> >> > Cc: K. Y. Srinivasan <kys@xxxxxxxxxxxxx> >> > >> > Signed-off-by: Yue Zhang <yuezha@xxxxxxxxxxxxx> >> > --- >> > 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 <linux/if_vlan.h> >> > #include <linux/in.h> >> > #include <linux/slab.h> >> > +#include <linux/delay.h> >> > #include <net/arp.h> >> > #include <net/route.h> >> > #include <net/sock.h> >> > @@ -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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
