Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
From: Herbert Xu <[EMAIL PROTECTED]> Date: Wed, 29 Nov 2006 23:07:09 +1100 > On Wed, Nov 29, 2006 at 12:42:24PM +0100, Peter Zijlstra wrote: > > > > However I'm not quite sure yet how to teach lockdep about this. The > > proposed patch will shut it up though. > > As a rule I think we should never make semantic changes to shut up > lockdep. Especially ones which are costly, as this proposed change is in that it disables software interrupts in a place where that is completely unnecessary. Let's not even consider this patch :) - 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/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
On Wed, Nov 29, 2006 at 12:42:24PM +0100, Peter Zijlstra wrote: > > However I'm not quite sure yet how to teach lockdep about this. The > proposed patch will shut it up though. As a rule I think we should never make semantic changes to shut up lockdep. Cheers, -- 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 - 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/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
On Wed, 2006-11-29 at 18:49 +1100, Herbert Xu wrote: > Peter Zijlstra <[EMAIL PROTECTED]> wrote: > > > > = > > [ INFO: possible irq lock inversion dependency detected ] > > 2.6.19-rc6 #4 > > - > > nc/1854 just changed the state of lock: > > (af_callback_keys + sk->sk_family#2){-.-?}, at: [] > > sock_def_error_report+0x1f/0x90 > > but this lock was taken by another, soft-irq-safe lock in the past: > > (slock-AF_INET){-+..} > > > > and interrupts could create inverse lock ordering between them. > > I think this is bogus. The slock is not a standard lock. When we > hold it in process context we don't actually hold the spin lock part > of it. However, it does prevent the softirq path from running in > critical sections which also prevents any attempt to grab the > callback lock from softirq context. > > If you still think there is a problem, please show an actual scenario > where it dead locks. process context does lock_sock(sk) which is basically a sleeping lock and sets an owner field when acquired. BH context does bh_lock_sock(sk); which spins on the spinlock protecting the owner field; and checks for an owner under this lock. When an owner is found it will stick the skb on a queue for later processing. This scheme does indeed seem to avoid the reported deadlock scenario - although I didn't audit all code paths. However I'm not quite sure yet how to teach lockdep about this. The proposed patch will shut it up though. - 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/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
On 29-11-2006 08:49, Herbert Xu wrote: > Peter Zijlstra <[EMAIL PROTECTED]> wrote: >> = >> [ INFO: possible irq lock inversion dependency detected ] >> 2.6.19-rc6 #4 >> - >> nc/1854 just changed the state of lock: >> (af_callback_keys + sk->sk_family#2){-.-?}, at: [] >> sock_def_error_report+0x1f/0x90 >> but this lock was taken by another, soft-irq-safe lock in the past: >> (slock-AF_INET){-+..} >> >> and interrupts could create inverse lock ordering between them. > > I think this is bogus. The slock is not a standard lock. When we > hold it in process context we don't actually hold the spin lock part > of it. However, it does prevent the softirq path from running in > critical sections which also prevents any attempt to grab the > callback lock from softirq context. > > If you still think there is a problem, please show an actual scenario > where it dead locks. It would be nice to have a look at other parts of stack backtraces probably with softirq part, which took that lock: sk->sk_family#2){-.-?} Jarek P. - 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/
Re: [PATCH] lockdep: fix sk->sk_callback_lock locking
Peter Zijlstra <[EMAIL PROTECTED]> wrote: > > = > [ INFO: possible irq lock inversion dependency detected ] > 2.6.19-rc6 #4 > - > nc/1854 just changed the state of lock: > (af_callback_keys + sk->sk_family#2){-.-?}, at: [] > sock_def_error_report+0x1f/0x90 > but this lock was taken by another, soft-irq-safe lock in the past: > (slock-AF_INET){-+..} > > and interrupts could create inverse lock ordering between them. I think this is bogus. The slock is not a standard lock. When we hold it in process context we don't actually hold the spin lock part of it. However, it does prevent the softirq path from running in critical sections which also prevents any attempt to grab the callback lock from softirq context. If you still think there is a problem, please show an actual scenario where it dead locks. Thanks, -- 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 - 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/
[PATCH] lockdep: fix sk->sk_callback_lock locking
= [ INFO: possible irq lock inversion dependency detected ] 2.6.19-rc6 #4 - nc/1854 just changed the state of lock: (af_callback_keys + sk->sk_family#2){-.-?}, at: [] sock_def_error_report+0x1f/0x90 but this lock was taken by another, soft-irq-safe lock in the past: (slock-AF_INET){-+..} and interrupts could create inverse lock ordering between them. stack backtrace: [] show_trace_log_lvl+0x26/0x40 [] show_trace+0x1b/0x20 [] dump_stack+0x24/0x30 [] print_irq_inversion_bug+0x10c/0x130 [] check_usage_backwards+0x42/0x50 [] mark_lock+0x312/0x620 [] __lock_acquire+0x4c5/0xcb0 [] lock_acquire+0x69/0x90 [] _read_lock+0x39/0x50 [] sock_def_wakeup+0x1c/0x60 [] inet_shutdown+0x93/0xf0 [] sys_shutdown+0x32/0x60 [] sys_socketcall+0x1d4/0x260 [] syscall_call+0x7/0xb === stack backtrace: [] show_trace_log_lvl+0x26/0x40 [] show_trace+0x1b/0x20 [] dump_stack+0x24/0x30 [] print_irq_inversion_bug+0x10c/0x130 [] check_usage_backwards+0x42/0x50 [] mark_lock+0x312/0x620 [] __lock_acquire+0x4c5/0xcb0 [] lock_acquire+0x69/0x90 [] _read_lock+0x39/0x50 [] sock_def_error_report+0x1f/0x90 [] tcp_disconnect+0x318/0x490 [] inet_stream_connect+0x220/0x260 [] sys_connect+0x6b/0x90 [] sys_socketcall+0x9f/0x260 [] syscall_call+0x7/0xb === sk->sk_callback_lock is usually only read locked from softirq context however it seems lockdep found two spots that are reachable from process context, thus creating the possibility of a deadlock. For now fix these two call sites with manual disabling of softirqs; if more of these sites are found we might consider changing the read_lock() to read_lock_bh(). Signed-off-by: Peter Zijlstra <[EMAIL PROTECTED]> CC: Martin Josefsson <[EMAIL PROTECTED]> --- net/ipv4/af_inet.c |2 ++ net/ipv4/tcp.c |2 ++ 2 files changed, 4 insertions(+) Index: linux-2.6-netdev/net/ipv4/af_inet.c === --- linux-2.6-netdev.orig/net/ipv4/af_inet.c2006-11-27 14:41:51.0 +0100 +++ linux-2.6-netdev/net/ipv4/af_inet.c 2006-11-28 07:06:23.0 +0100 @@ -731,7 +731,9 @@ int inet_shutdown(struct socket *sock, i } /* Wake up anyone sleeping in poll. */ + local_bh_disable(); sk->sk_state_change(sk); + local_bh_enable(); release_sock(sk); return err; } Index: linux-2.6-netdev/net/ipv4/tcp.c === --- linux-2.6-netdev.orig/net/ipv4/tcp.c2006-11-28 07:06:16.0 +0100 +++ linux-2.6-netdev/net/ipv4/tcp.c 2006-11-28 07:06:20.0 +0100 @@ -1765,7 +1765,9 @@ int tcp_disconnect(struct sock *sk, int BUG_TRAP(!inet->num || icsk->icsk_bind_hash); + local_bh_disable(); sk->sk_error_report(sk); + local_bh_enable(); return err; } - 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/