Re: [PATCH] Hyperv: Trigger DHCP renew after host hibernation

[Tom Gundersen]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/