From: Yue Zhang <[email protected]>
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 <[email protected]>
Cc: K. Y. Srinivasan <[email protected]>
Signed-off-by: Yue Zhang <[email protected]>
---
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();
+ }
if (notify)
netdev_notify_peers(net);
--
1.9.1
On 07/18/2014 04:25 PM, Yue Zhang wrote:
> @@ -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
> + */
This should be networking comment style..
/* bla bla..
* bla
*/
--
Regards,
Varka Bhadram.
On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <[email protected]> wrote:
> From: Yue Zhang <[email protected]>
>
> 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 <[email protected]>
> Cc: K. Y. Srinivasan <[email protected]>
>
> Signed-off-by: Yue Zhang <[email protected]>
> ---
> 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
> + */
Why 10? Is this a random number which works by accident for ifplugd?
What about other networking implementations, is 10 also ok for them?
> + for (delay = 0; delay < 10; delay++) {
> + rtnl_lock();
> + netif_carrier_off(net);
> + rtnl_unlock();
> + ssleep(1);
> + }
> + rtnl_lock();
> + netif_carrier_on(net);
> + rtnl_unlock();
> + }
>
> if (notify)
> netdev_notify_peers(net);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Thanks,
//richard
> From: Richard Weinberger [mailto:[email protected]]
> Sent: Friday, July 18, 2014 9:41 PM
>
> On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <[email protected]> wrote:
> > From: Yue Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: K. Y. Srinivasan <[email protected]>
> >
> > Signed-off-by: Yue Zhang <[email protected]>
> > ---
> > 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
> > + */
>
> Why 10? Is this a random number which works by accident for ifplugd?
> What about other networking implementations, is 10 also ok for them?
> --
> Thanks,
> //richard
Hi, Richard
I checked ifplugd's code. The deferring time is 5 seconds. That's how comes
the "10s". I agree with you this is a magic number and should be avoid. However,
this is the only feasible solution right now. If there is a better solution, I will be
glad to switch to it.
I tested the fix in Redhat, Ubuntu and SUSE and it works in all of them.
Thanks
Yie
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
> From: Varka Bhadram [mailto:[email protected]]
> Sent: Friday, July 18, 2014 6:13 PM
>
> On 07/18/2014 04:25 PM, Yue Zhang wrote:
> > @@ -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
> > + */
>
> This should be networking comment style..
>
> /* bla bla..
> * bla
> */
>
> --
> Regards,
> Varka Bhadram.
I will fix this.
Thanks
Yue
Yie,
Am 21.07.2014 04:44, schrieb Yue Zhang (OSTC DEV):
>> From: Richard Weinberger [mailto:[email protected]]
>> Why 10? Is this a random number which works by accident for ifplugd?
>> What about other networking implementations, is 10 also ok for them?
>> --
>> Thanks,
>> //richard
>
> Hi, Richard
>
> I checked ifplugd's code. The deferring time is 5 seconds. That's how comes
> the "10s". I agree with you this is a magic number and should be avoid. However,
> this is the only feasible solution right now. If there is a better solution, I will be
> glad to switch to it.
>
> I tested the fix in Redhat, Ubuntu and SUSE and it works in all of them.
The problem I see is that there is no good way to trigger a DHCP renew from
a network device drivers. You're on the wrong layer.
10 seconds may work but this is IMHO a hack which can easily break.
There are also more networking implementations than ifplugd.
Specially the systemd implementation looks promising.
Can't you propagate the RNDIS_STATUS_NETWORK_CHANGE event to userspace?
IIRC on HyperV guests already have a guest daemon. Let the daemon handle
the event such that distros can install their own hooks...
Thanks,
//richard
> From: Richard Weinberger [mailto:[email protected]]
> Sent: Monday, July 21, 2014 2:55 PM
>
> Yue,
>
> Am 21.07.2014 04:44, schrieb Yue Zhang (OSTC DEV):
> >> From: Richard Weinberger [mailto:[email protected]]
> >> Why 10? Is this a random number which works by accident for ifplugd?
> >> What about other networking implementations, is 10 also ok for them?
> >> --
> >> Thanks,
> >> //richard
> >
> > Hi, Richard
> >
> > I checked ifplugd's code. The deferring time is 5 seconds. That's how
> comes
> > the "10s". I agree with you this is a magic number and should be avoid.
> However,
> > this is the only feasible solution right now. If there is a better solution, I will
> be
> > glad to switch to it.
> >
> > I tested the fix in Redhat, Ubuntu and SUSE and it works in all of them.
>
> The problem I see is that there is no good way to trigger a DHCP renew from
> a network device drivers. You're on the wrong layer.
> 10 seconds may work but this is IMHO a hack which can easily break.
> There are also more networking implementations than ifplugd.
> Specially the systemd implementation looks promising.
>
> Can't you propagate the RNDIS_STATUS_NETWORK_CHANGE event to
> userspace?
> IIRC on HyperV guests already have a guest daemon. Let the daemon handle
> the event such that distros can install their own hooks...
>
> Thanks,
> //richard
Hi, Richard
The problem of systemd implementation is that in different distros, the ways to
restart service are different. Propagating the event to userspace also doesn't help
for this issue.
The advantage of current solution is that it simulates a cable plugging in/out event.
IMHO, in all the distros, this simulated event has already been well handled. It is a
dup effect to implement new hooks.
Thanks
----
Yue
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Yue,
Am 21.07.2014 10:05, schrieb Yue Zhang (OSTC DEV):
> The problem of systemd implementation is that in different distros, the ways to
> restart service are different. Propagating the event to userspace also doesn't help
> for this issue.
This way each distro can provide their own restart script.
Same as every distro has custom start scripts, etc...
> The advantage of current solution is that it simulates a cable plugging in/out event.
> IMHO, in all the distros, this simulated event has already been well handled. It is a
> dup effect to implement new hooks.
Iff the current solution works for _all_ networking implementations.
Thanks,
//richard
> From: Richard Weinberger [mailto:[email protected]]
> Sent: Monday, July 21, 2014 4:18 PM
>
> Yue,
>
> Am 21.07.2014 10:05, schrieb Yue Zhang (OSTC DEV):
> > The problem of systemd implementation is that in different distros, the ways
> > to restart service are different. Propagating the event to userspace also doesn't
> > help for this issue.
>
> This way each distro can provide their own restart script.
> Same as every distro has custom start scripts, etc...
>
> > The advantage of current solution is that it simulates a cable plugging
> > in/out event. IMHO, in all the distros, this simulated event has already been well
> > handled. It is a dup effect to implement new hooks.
>
> Iff the current solution works for _all_ networking implementations.
>
> Thanks,
> //richard
Hi, Richard
IMHO, all networking implementations should handle the cable offline event. Consider
this situation. I unplugged the network cable and connect it to a new network switch
after 10 seconds. If the DHCP renew is not triggered, the network will break. I think in
normal cases, it should already been handled properly. Unless there is a strong
justification for not doing this. In that case, we shouldn't renew DHCP anyway.
Thanks
----
Yue
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
Yue,
Am 21.07.2014 10:44, schrieb Yue Zhang (OSTC DEV):
> Hi, Richard
>
> IMHO, all networking implementations should handle the cable offline event. Consider
> this situation. I unplugged the network cable and connect it to a new network switch
> after 10 seconds. If the DHCP renew is not triggered, the network will break. I think in
> normal cases, it should already been handled properly. Unless there is a strong
> justification for not doing this. In that case, we shouldn't renew DHCP anyway.
I agree that they should handle the cable offline event.
My concern is that 10 seconds is maybe not a the right choice.
(As we cannot know all implementations)
Thanks,
//richard
On Mon, Jul 21, Richard Weinberger wrote:
> My concern is that 10 seconds is maybe not a the right choice.
> (As we cannot know all implementations)
Until someone reports an issue with it, 10 is fine. Just like 20 or 666.
Olaf
On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <[email protected]> wrote:
> From: Yue Zhang <[email protected]>
>
> 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 <[email protected]>
> Cc: K. Y. Srinivasan <[email protected]>
>
> Signed-off-by: Yue Zhang <[email protected]>
> ---
> 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
> if (notify)
> netdev_notify_peers(net);
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Tom Gundersen [mailto:[email protected]]
> Sent: Monday, July 21, 2014 5:42 PM
>
> On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <[email protected]> wrote:
> > From: Yue Zhang <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: K. Y. Srinivasan <[email protected]>
> >
> > Signed-off-by: Yue Zhang <[email protected]>
> > ---
> > 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.
Thanks
---
Yue
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Mon, 21.07.14 10:21, Yue Zhang (OSTC DEV) ([email protected]) wrote:
> 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.
ifplugd doesn't renew DHCP leases anyway, one of the scripts it invokes
does.
ifplugd is obsolete software. I wrote it more than 10 years ago, and
haven't really updated it since. it's sounds seriously wrong to add
multi-second waits to the kernel just to make this crappy, obsolete
software work.
Please fix this properly, and work with the PM guys, so that we get a
sane userspace how the kernel can notify userspace about
suspends/hibernations triggered from the outside, so that userspace
daemons can subscribe to that and then refresh the DHCP leases on their
own.
Lennart
--
Lennart Poettering, Red Hat
On Mon, Jul 21, 2014 at 12:21 PM, Yue Zhang (OSTC DEV)
<[email protected]> wrote:
>> From: Tom Gundersen [mailto:[email protected]]
>> Sent: Monday, July 21, 2014 5:42 PM
>>
>> On Fri, Jul 18, 2014 at 12:55 PM, Yue Zhang <[email protected]> wrote:
>> > From: Yue Zhang <[email protected]>
>> >
>> > 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 <[email protected]>
>> > Cc: K. Y. Srinivasan <[email protected]>
>> >
>> > Signed-off-by: Yue Zhang <[email protected]>
>> > ---
>> > 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
On Mon, 2014-07-21 at 15:11 +0200, Lennart Poettering wrote:
> On Mon, 21.07.14 10:21, Yue Zhang (OSTC DEV) ([email protected]) wrote:
>
> > 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.
>
> ifplugd doesn't renew DHCP leases anyway, one of the scripts it invokes
> does.
>
> ifplugd is obsolete software. I wrote it more than 10 years ago, and
> haven't really updated it since. it's sounds seriously wrong to add
> multi-second waits to the kernel just to make this crappy, obsolete
> software work.
>
> Please fix this properly, and work with the PM guys, so that we get a
> sane userspace how the kernel can notify userspace about
> suspends/hibernations triggered from the outside, so that userspace
> daemons can subscribe to that and then refresh the DHCP leases on their
> own.
Yeah, like I've said before, there have been other cases where a "hey,
my L3 address might be wrong now, so please confirm it" message would be
useful. Carrier on/off doesn't necessarily mean that, but even if it
did, the off interval is a really bad mechanism for that too. So I'd
really like some kind of event for this that's distinct from carrier
that userspace could use.
Dan
From: Olaf Hering <[email protected]>
Date: Mon, 21 Jul 2014 11:18:51 +0200
> On Mon, Jul 21, Richard Weinberger wrote:
>
>> My concern is that 10 seconds is maybe not a the right choice.
>> (As we cannot know all implementations)
>
> Until someone reports an issue with it, 10 is fine. Just like 20 or 666.
Wrong, this is policy and belongs in userspace.