net: Disable queueing when carrier is lost (take 2)
Subsystems (3)
PATCH 1/1 net: Disable queueing when carrier is lost (take 2)
Some network drivers call netif_stop_queue() when detecting loss of carrier. This leads to packets being queued up at the qdisc level for an unbound period of time. In order to prevent this effect, the core networking stack will now seize to queue packets for any device, that is operationally down (i.e. the queue is flushed and disabled).
net/core/dev.c +1 -5
@@ -2203,14 +2203,10 @@unsigned flags; flags = (dev->flags & ~(IFF_PROMISC | -IFF_ALLMULTI | -IFF_RUNNING )) | +IFF_ALLMULTI )) | (dev->gflags & (IFF_PROMISC |IFF_ALLMULTI )); -if (netif_running (dev) &&netif_carrier_ok (dev)) - flags |=IFF_RUNNING ; -return flags; }
net/core/link_watch.c +7 -0
@@ -16,6 +16,7 @@#include <linux/netdevice.h> #include <linux/if.h> #include <net/sock.h> +#include <net/pkt_sched.h> #include <linux/rtnetlink.h> #include <linux/jiffies.h> #include <linux/spinlock.h>
@@ -74,6 +75,12 @@clear_bit (__LINK_STATE_LINKWATCH_PENDING, &dev->state);if (dev->flags &IFF_UP ) { +if (netif_carrier_ok (dev)) { +WARN_ON (dev->qdisc_sleeping == &noop_qdisc); +dev_activate (dev); + }else +dev_deactivate (dev); +netdev_state_change (dev); }
net/sched/sch_generic.c +17 -6
@@ -514,6 +514,9 @@void dev_activate (struct net_device *dev) { +if (dev->flags &IFF_RUNNING ) +return ; +/* No queueing discipline is attached to device; create default one i.e. pfifo_fast for devices, which need queueing and noqueue_qdisc for
@@ -539,6 +542,11 @@write_unlock_bh (&qdisc_tree_lock); } +if (!netif_carrier_ok (dev)) +/* Delay activation until next carrier-on event */ +return ; + dev->flags |=IFF_RUNNING ; +spin_lock_bh (&dev->queue_lock);rcu_assign_pointer (dev->qdisc, dev->qdisc_sleeping);if (dev->qdisc != &noqueue_qdisc) {
@@ -552,15 +560,18 @@ {struct Qdisc *qdisc; -spin_lock_bh (&dev->queue_lock); - qdisc = dev->qdisc; - dev->qdisc = &noop_qdisc; +if (dev->flags &IFF_RUNNING ) { +spin_lock_bh (&dev->queue_lock); + qdisc = dev->qdisc; + dev->qdisc = &noop_qdisc; -qdisc_reset (qdisc); +qdisc_reset (qdisc); -spin_unlock_bh (&dev->queue_lock); +spin_unlock_bh (&dev->queue_lock); -dev_watchdog_down (dev); +dev_watchdog_down (dev); + } + dev->flags &= ~IFF_RUNNING ;while (test_bit (__LINK_STATE_SCHED, &dev->state))yield ();
Hi Tommy:
Quoted 5 lines · Tommy Christensen
Some network drivers call
netif_stop_queue()when detecting loss of carrier. This leads to packets being queued up at the qdisc level for an unbound period of time. In order to prevent this effect, the core networking stack will now seize to queue packets for any device, that is operationally down (i.e. the queue is flushed and disabled).
This looks great.
Quoted 15 lines
net/sched/sch_generic.c +5
-4@@ -552,15 +560,18 @@ {struct Qdisc *qdisc; -spin_lock_bh (&dev->queue_lock); - qdisc = dev->qdisc; - dev->qdisc = &noop_qdisc; +if (dev->flags &IFF_RUNNING ) { +spin_lock_bh (&dev->queue_lock); + qdisc = dev->qdisc; + dev->qdisc = &noop_qdisc; -qdisc_reset (qdisc); +qdisc_reset (qdisc);
net/sched/sch_generic.c line 577
Quoted 10 lines
net/sched/sch_generic.c +4
-2-spin_unlock_bh (&dev->queue_lock); +spin_unlock_bh (&dev->queue_lock); -dev_watchdog_down (dev); +dev_watchdog_down (dev); + } + dev->flags &= ~IFF_RUNNING ;while (test_bit (__LINK_STATE_SCHED, &dev->state))yield ();
Doing the wait when IFF_RUNNING is off isn't necessary though. If IFF_RUNNING isn't set, then either the device has never been activated or we've already carried out those waits the last time we were in dev_deactivate.
I understand your preference for defensive programming. However, in cases like this it's better to do the obvious thing because:
1) We don't cover up bugs that may come back to bite us elsewhere.
2) We don't give people misconceptions. If they're unfamiliar with the system they may infer things from this code that aren't necessarily the case.
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt* Herbert Xu <[email protected]> 2005-05-03 20:03
Quoted 32 lines · Tommy Christensen
Quoted 5 lines
Some network drivers call
netif_stop_queue()when detecting loss of carrier. This leads to packets being queued up at the qdisc level for an unbound period of time. In order to prevent this effect, the core networking stack will now seize to queue packets for any device, that is operationally down (i.e. the queue is flushed and disabled).This looks great.
Quoted 23 lines
net/sched/sch_generic.c +9
-6@@ -552,15 +560,18 @@ {struct Qdisc *qdisc; -spin_lock_bh (&dev->queue_lock); - qdisc = dev->qdisc; - dev->qdisc = &noop_qdisc; +if (dev->flags &IFF_RUNNING ) { +spin_lock_bh (&dev->queue_lock); + qdisc = dev->qdisc; + dev->qdisc = &noop_qdisc; -qdisc_reset (qdisc); +qdisc_reset (qdisc); -spin_unlock_bh (&dev->queue_lock); +spin_unlock_bh (&dev->queue_lock); -dev_watchdog_down (dev); +dev_watchdog_down (dev); + } + dev->flags &= ~IFF_RUNNING ;
Quoted 7 lines
net/sched/sch_generic.c line 577
Quoted 2 lines
net/sched/sch_generic.c +0
-0while (test_bit (__LINK_STATE_SCHED, &dev->state))yield ();Doing the wait when
IFF_RUNNINGis off isn't necessary though. IfIFF_RUNNINGisn't set, then either the device has never been activated or we've already carried out those waits the last time we were indev_deactivate.
I do like the patch, no question but IFF_RUNNING is still abused by drivers and some subsystems. So I'm not sure how reliable the above code will be without those cases fixed. I submitted a patchset once to fix some of them, not sure about the status. Also, what about those drivers that do not support or do not use netif_carrier_(on|off)?
Quoted 4 lines · Thomas Graf
I do like the patch, no question but
IFF_RUNNINGis still abused by drivers and some subsystems. So I'm not sure how reliable the above code will be without those cases fixed. I submitted a
Ouch, you're right. They either need to be fixed or we'll just have to go back to the simpler version that may call dev_activate/dev_deactivate repeatedly.
Quoted 3 lines
patchset once to fix some of them, not sure about the status. Also, what about those drivers that do not support or do not use
netif_carrier_(on|off)?
They simply won't benefit from this patch. This is OK since they're no worse off than where they were before. It might also prove to be an incentive for someone to finally fix them :)
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtQuoted 4 lines · Herbert Xu
Doing the wait when
IFF_RUNNINGis off isn't necessary though. IfIFF_RUNNINGisn't set, then either the device has never been activated or we've already carried out those waits the last time we were indev_deactivate.
Right. But it still depends on what the synchronization is meant to protect us from. It isn't clear to me, whether it's
o packets in flight
o qdisc references
o device references
so yes, I tried to play it safe.
Anyway, given the comments from Thomas, I've pulled out the IFF_RUNNING stuff. We can add this later, when the other uses have been sorted out.
Quoted 7 lines
I understand your preference for defensive programming. However, in cases like this it's better to do the obvious thing because:
1) We don't cover up bugs that may come back to bite us elsewhere.
2) We don't give people misconceptions. If they're unfamiliar with the system they may infer things from this code that aren't necessarily the case.
I totally agree with this.
-Tommy
net/core/link_watch.c +7 -0
@@ -16,6 +16,7 @@#include <linux/netdevice.h> #include <linux/if.h> #include <net/sock.h> +#include <net/pkt_sched.h> #include <linux/rtnetlink.h> #include <linux/jiffies.h> #include <linux/spinlock.h>
@@ -74,6 +75,12 @@clear_bit (__LINK_STATE_LINKWATCH_PENDING, &dev->state);if (dev->flags &IFF_UP ) { +if (netif_carrier_ok (dev)) { +WARN_ON (dev->qdisc_sleeping == &noop_qdisc); +dev_activate (dev); + }else +dev_deactivate (dev); +netdev_state_change (dev); }
net/sched/sch_generic.c +4 -0
@@ -539,6 +539,10 @@write_unlock_bh (&qdisc_tree_lock); } +if (!netif_carrier_ok (dev)) +/* Delay activation until next carrier-on event */ +return ; +spin_lock_bh (&dev->queue_lock);rcu_assign_pointer (dev->qdisc, dev->qdisc_sleeping);if (dev->qdisc != &noqueue_qdisc) {
Quoted 3 lines · Tommy Christensen
Anyway, given the comments from Thomas, I've pulled out the
IFF_RUNNINGstuff. We can add this later, when the other uses have been sorted out.
Thanks Tommy. This version looks good to me. BTW, you probably should add a Signed-off-by line.
Cheers,
Signature · 4 lines
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txtDavid S. Miller
maintainerQuoted 7 lines · On Wed, 4 May 2005 09:10:23 +1000 Herbert Xu <[email protected]>…
Quoted 3 lines · Tommy Christensen
Anyway, given the comments from Thomas, I've pulled out the
IFF_RUNNINGstuff. We can add this later, when the other uses have been sorted out.Thanks Tommy. This version looks good to me. BTW, you probably should add a Signed-off-by line.
Yes, please provide this and I'll happily apply this patch. Thanks.
Quoted 2 lines · Herbert Xu
Thanks Tommy. This version looks good to me. BTW, you probably should add a Signed-off-by line.
Sigh. I remembered on all but the final patch ...
Thanks for helping out.
net/core/link_watch.c +7 -0
@@ -16,6 +16,7 @@#include <linux/netdevice.h> #include <linux/if.h> #include <net/sock.h> +#include <net/pkt_sched.h> #include <linux/rtnetlink.h> #include <linux/jiffies.h> #include <linux/spinlock.h>
@@ -74,6 +75,12 @@clear_bit (__LINK_STATE_LINKWATCH_PENDING, &dev->state);if (dev->flags &IFF_UP ) { +if (netif_carrier_ok (dev)) { +WARN_ON (dev->qdisc_sleeping == &noop_qdisc); +dev_activate (dev); + }else +dev_deactivate (dev); +netdev_state_change (dev); }
net/sched/sch_generic.c +4 -0
@@ -539,6 +539,10 @@write_unlock_bh (&qdisc_tree_lock); } +if (!netif_carrier_ok (dev)) +/* Delay activation until next carrier-on event */ +return ; +spin_lock_bh (&dev->queue_lock);rcu_assign_pointer (dev->qdisc, dev->qdisc_sleeping);if (dev->qdisc != &noqueue_qdisc) {
Quoted 8 lines · On Wed, 04 May 2005 01:28:24 +0200 Tommy Christensen…
Quoted 2 lines · Herbert Xu
Thanks Tommy. This version looks good to me. BTW, you probably should add a Signed-off-by line.
Sigh. I remembered on all but the final patch ...
Thanks for helping out.Signed-off-by Tommy S. Christensen <[email protected]>
Applied, thanks Tommy.