diff options
author | Herbert Xu <herbert@gondor.apana.org.au> | 2006-09-18 03:22:30 -0400 |
---|---|---|
committer | David S. Miller <davem@davemloft.net> | 2006-09-18 03:22:30 -0400 |
commit | d7811e623dd4be3e3bdba2d6330f7f15541ee45f (patch) | |
tree | f43bafe2e8d7e43b2cc3f4cb7dcda4c5e8815630 | |
parent | cbe21d8fefca605b90e34f0f019505e599712f9b (diff) |
[NET]: Drop tx lock in dev_watchdog_up
Fix lockdep warning with GRE, iptables and Speedtouch ADSL, PPP over ATM.
On Sat, Sep 02, 2006 at 08:39:28PM +0000, Krzysztof Halasa wrote:
>
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> -------------------------------------------------------
> swapper/0 is trying to acquire lock:
> (&dev->queue_lock){-+..}, at: [<c02c8c46>] dev_queue_xmit+0x56/0x290
>
> but task is already holding lock:
> (&dev->_xmit_lock){-+..}, at: [<c02c8e14>] dev_queue_xmit+0x224/0x290
>
> which lock already depends on the new lock.
This turns out to be a genuine bug. The queue lock and xmit lock are
intentionally taken out of order. Two things are supposed to prevent
dead-locks from occuring:
1) When we hold the queue_lock we're supposed to only do try_lock on the
tx_lock.
2) We always drop the queue_lock after taking the tx_lock and before doing
anything else.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&dev->_xmit_lock){-+..}:
> [<c012e7b6>] lock_acquire+0x76/0xa0
> [<c0336241>] _spin_lock_bh+0x31/0x40
> [<c02d25a9>] dev_activate+0x69/0x120
This path obviously breaks assumption 1) and therefore can lead to ABBA
dead-locks.
I've looked at the history and there seems to be no reason for the lock
to be held at all in dev_watchdog_up. The lock appeared in day one and
even there it was unnecessary. In fact, people added __dev_watchdog_up
precisely in order to get around the tx lock there.
The function dev_watchdog_up is already serialised by rtnl_lock since
its only caller dev_activate is always called under it.
So here is a simple patch to remove the tx lock from dev_watchdog_up.
In 2.6.19 we can eliminate the unnecessary __dev_watchdog_up and
replace it with dev_watchdog_up.
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
-rw-r--r-- | net/sched/sch_generic.c | 2 |
1 files changed, 0 insertions, 2 deletions
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 0834c2ee9174..6f9151899795 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c | |||
@@ -238,9 +238,7 @@ void __netdev_watchdog_up(struct net_device *dev) | |||
238 | 238 | ||
239 | static void dev_watchdog_up(struct net_device *dev) | 239 | static void dev_watchdog_up(struct net_device *dev) |
240 | { | 240 | { |
241 | netif_tx_lock_bh(dev); | ||
242 | __netdev_watchdog_up(dev); | 241 | __netdev_watchdog_up(dev); |
243 | netif_tx_unlock_bh(dev); | ||
244 | } | 242 | } |
245 | 243 | ||
246 | static void dev_watchdog_down(struct net_device *dev) | 244 | static void dev_watchdog_down(struct net_device *dev) |