* [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
@ 2010-09-02 20:12 Jarek Poplawski
2010-09-02 20:22 ` David Miller
2010-09-02 20:53 ` Eric Dumazet
0 siblings, 2 replies; 8+ messages in thread
From: Jarek Poplawski @ 2010-09-02 20:12 UTC (permalink / raw)
To: David Miller; +Cc: Eric Dumazet, netdev
Hi,
Almost any tc qdisc activity on 2.6.35 or later triggers this warning
(the patch below):
[ 516.287584] =========================================================
[ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
[ 516.288386] 2.6.35b #7
[ 516.288386] ---------------------------------------------------------
[ 516.288386] swapper/0 just changed the state of lock:
[ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
[ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
[ 516.288386] (est_tree_lock){+.+...}
[ 516.288386]
[ 516.288386] and interrupts could create inverse lock ordering between them.
[ 516.288386]
[ 516.288386]
[ 516.288386] other info that might help us debug this:
[ 516.288386] 2 locks held by swapper/0:
[ 516.288386] #0: (&elist[idx].timer){+.-...}, at: [<c10334b5>] run_timer_softirq+0x11d/0x263
[ 516.288386] #1: (rcu_read_lock){.+.+..}, at: [<c12eac78>] est_timer+0x0/0x1b4
[ 516.288386]
[ 516.288386] the shortest dependencies between 2nd lock and 1st lock:
[ 516.288386] -> (est_tree_lock){+.+...} ops: 5 {
[ 516.288386] HARDIRQ-ON-W at:
[ 516.288386] [<c104a421>] __lock_acquire+0x306/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
[ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
[ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
[ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
[ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386] SOFTIRQ-ON-W at:
[ 516.288386] [<c104a442>] __lock_acquire+0x327/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12ea9d8>] gen_kill_estimator+0x18/0x9b
[ 516.288386] [<c12fea48>] qdisc_destroy+0x34/0x79
[ 516.288386] [<c1300393>] notify_and_destroy+0x35/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c1300686>] tc_get_qdisc+0x125/0x157
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386] INITIAL USE at:
[ 516.288386] [<c104a49a>] __lock_acquire+0x37f/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
[ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
[ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
[ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
[ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386] }
[ 516.288386] ... key at: [<c15a70fc>] est_tree_lock+0x10/0x1c
[ 516.288386] ... acquired at:
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
[ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
[ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
[ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
[ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386]
[ 516.288386] -> (&qdisc_tx_lock){+.-...} ops: 10 {
[ 516.288386] HARDIRQ-ON-W at:
[ 516.288386] [<c104a421>] __lock_acquire+0x306/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c55e1>] _raw_spin_lock_bh+0x2a/0x39
[ 516.288386] [<c12ea8f1>] gnet_stats_start_copy_compat+0x27/0x70
[ 516.288386] [<c12fffc7>] tc_fill_qdisc+0x14d/0x1e2
[ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
[ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386] IN-SOFTIRQ-W at:
[ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
[ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
[ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
[ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
[ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
[ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
[ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
[ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
[ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
[ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
[ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
[ 516.288386] INITIAL USE at:
[ 516.288386] [<c104a49a>] __lock_acquire+0x37f/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c55e1>] _raw_spin_lock_bh+0x2a/0x39
[ 516.288386] [<c12ea8f1>] gnet_stats_start_copy_compat+0x27/0x70
[ 516.288386] [<c12fffc7>] tc_fill_qdisc+0x14d/0x1e2
[ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
[ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
[ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
[ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
[ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
[ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
[ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
[ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
[ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
[ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
[ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
[ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
[ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
[ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
[ 516.288386] }
[ 516.288386] ... key at: [<c1b8318c>] qdisc_tx_lock+0x0/0x8
[ 516.288386] ... acquired at:
[ 516.288386] [<c1049bf3>] check_usage_forwards+0x5d/0x68
[ 516.288386] [<c1049575>] mark_lock+0x102/0x1e0
[ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
[ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
[ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
[ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
[ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
[ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
[ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
[ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
[ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
[ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
[ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
[ 516.288386]
[ 516.288386]
[ 516.288386] stack backtrace:
[ 516.288386] Pid: 0, comm: swapper Not tainted 2.6.35b #7
[ 516.288386] Call Trace:
[ 516.288386] [<c13c3a01>] ? printk+0xf/0x16
[ 516.288386] [<c1049b8b>] print_irq_inversion_bug+0xef/0xfa
[ 516.288386] [<c1049bf3>] check_usage_forwards+0x5d/0x68
[ 516.288386] [<c1049575>] mark_lock+0x102/0x1e0
[ 516.288386] [<c1049b96>] ? check_usage_forwards+0x0/0x68
[ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
[ 516.288386] [<c104a49a>] ? __lock_acquire+0x37f/0xbd6
[ 516.288386] [<c104ac2b>] ? __lock_acquire+0xb10/0xbd6
[ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
[ 516.288386] [<c12eacda>] ? est_timer+0x62/0x1b4
[ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
[ 516.288386] [<c12eacda>] ? est_timer+0x62/0x1b4
[ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
[ 516.288386] [<c10334b5>] ? run_timer_softirq+0x11d/0x263
[ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
[ 516.288386] [<c12eac78>] ? est_timer+0x0/0x1b4
[ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
[ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
[ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
[ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
[ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
[ 516.288386] [<c104007b>] ? create_new_namespaces+0x37/0x127
[ 516.288386] [<c1007c0b>] ? default_idle+0x46/0x65
[ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
[ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
[ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
[ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
----------->
This patch fixes a lockdep warning:
[ 516.287584] =========================================================
[ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
[ 516.288386] 2.6.35b #7
[ 516.288386] ---------------------------------------------------------
[ 516.288386] swapper/0 just changed the state of lock:
[ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
[ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
[ 516.288386] (est_tree_lock){+.+...}
[ 516.288386]
[ 516.288386] and interrupts could create inverse lock ordering between them.
...
So, est_tree_lock needs BH protection because it's taken by
qdisc_tx_lock, which is used both in BH and process contexts.
(Full warning with this patch at netdev, 02 Sep 2010.)
Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
---
diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
index 9fbe7f7..6743146 100644
--- a/net/core/gen_estimator.c
+++ b/net/core/gen_estimator.c
@@ -232,7 +232,7 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
est->last_packets = bstats->packets;
est->avpps = rate_est->pps<<10;
- spin_lock(&est_tree_lock);
+ spin_lock_bh(&est_tree_lock);
if (!elist[idx].timer.function) {
INIT_LIST_HEAD(&elist[idx].list);
setup_timer(&elist[idx].timer, est_timer, idx);
@@ -243,7 +243,7 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
list_add_rcu(&est->list, &elist[idx].list);
gen_add_node(est);
- spin_unlock(&est_tree_lock);
+ spin_unlock_bh(&est_tree_lock);
return 0;
}
@@ -270,7 +270,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
{
struct gen_estimator *e;
- spin_lock(&est_tree_lock);
+ spin_lock_bh(&est_tree_lock);
while ((e = gen_find_node(bstats, rate_est))) {
rb_erase(&e->node, &est_root);
@@ -281,7 +281,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
list_del_rcu(&e->list);
call_rcu(&e->e_rcu, __gen_kill_estimator);
}
- spin_unlock(&est_tree_lock);
+ spin_unlock_bh(&est_tree_lock);
}
EXPORT_SYMBOL(gen_kill_estimator);
@@ -320,9 +320,9 @@ bool gen_estimator_active(const struct gnet_stats_basic_packed *bstats,
ASSERT_RTNL();
- spin_lock(&est_tree_lock);
+ spin_lock_bh(&est_tree_lock);
res = gen_find_node(bstats, rate_est) != NULL;
- spin_unlock(&est_tree_lock);
+ spin_unlock_bh(&est_tree_lock);
return res;
}
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:12 [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator Jarek Poplawski
@ 2010-09-02 20:22 ` David Miller
2010-09-02 20:32 ` Eric Dumazet
2010-09-02 20:53 ` Eric Dumazet
1 sibling, 1 reply; 8+ messages in thread
From: David Miller @ 2010-09-02 20:22 UTC (permalink / raw)
To: jarkao2; +Cc: eric.dumazet, netdev
From: Jarek Poplawski <jarkao2@gmail.com>
Date: Thu, 2 Sep 2010 22:12:58 +0200
> This patch fixes a lockdep warning:
>
> [ 516.287584] =========================================================
> [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> [ 516.288386] 2.6.35b #7
> [ 516.288386] ---------------------------------------------------------
> [ 516.288386] swapper/0 just changed the state of lock:
> [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> [ 516.288386] (est_tree_lock){+.+...}
> [ 516.288386]
> [ 516.288386] and interrupts could create inverse lock ordering between them.
> ...
>
> So, est_tree_lock needs BH protection because it's taken by
> qdisc_tx_lock, which is used both in BH and process contexts.
> (Full warning with this patch at netdev, 02 Sep 2010.)
>
> Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Applied, thanks Jarek.
Eric, please make an effort to run with lockdep enabled when making
locking changes, just for fun :-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:22 ` David Miller
@ 2010-09-02 20:32 ` Eric Dumazet
2010-09-02 20:55 ` Jarek Poplawski
0 siblings, 1 reply; 8+ messages in thread
From: Eric Dumazet @ 2010-09-02 20:32 UTC (permalink / raw)
To: David Miller; +Cc: jarkao2, netdev
Le jeudi 02 septembre 2010 à 13:22 -0700, David Miller a écrit :
> From: Jarek Poplawski <jarkao2@gmail.com>
> Date: Thu, 2 Sep 2010 22:12:58 +0200
>
> > This patch fixes a lockdep warning:
> >
> > [ 516.287584] =========================================================
> > [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> > [ 516.288386] 2.6.35b #7
> > [ 516.288386] ---------------------------------------------------------
> > [ 516.288386] swapper/0 just changed the state of lock:
> > [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> > [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> > [ 516.288386] (est_tree_lock){+.+...}
> > [ 516.288386]
> > [ 516.288386] and interrupts could create inverse lock ordering between them.
> > ...
> >
> > So, est_tree_lock needs BH protection because it's taken by
> > qdisc_tx_lock, which is used both in BH and process contexts.
> > (Full warning with this patch at netdev, 02 Sep 2010.)
> >
> > Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
> >
> > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
>
> Applied, thanks Jarek.
>
> Eric, please make an effort to run with lockdep enabled when making
> locking changes, just for fun :-)
Hmm... I still trying to understand the problem.
I dont understand Jarek analysis yet.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:12 [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator Jarek Poplawski
2010-09-02 20:22 ` David Miller
@ 2010-09-02 20:53 ` Eric Dumazet
2010-09-02 20:59 ` David Miller
2010-09-02 21:22 ` Jarek Poplawski
1 sibling, 2 replies; 8+ messages in thread
From: Eric Dumazet @ 2010-09-02 20:53 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
Le jeudi 02 septembre 2010 à 22:12 +0200, Jarek Poplawski a écrit :
> Hi,
>
> Almost any tc qdisc activity on 2.6.35 or later triggers this warning
> (the patch below):
>
> [ 516.287584] =========================================================
> [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> [ 516.288386] 2.6.35b #7
> [ 516.288386] ---------------------------------------------------------
> [ 516.288386] swapper/0 just changed the state of lock:
> [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> [ 516.288386] (est_tree_lock){+.+...}
> [ 516.288386]
> [ 516.288386] and interrupts could create inverse lock ordering between them.
> [ 516.288386]
> [ 516.288386]
> [ 516.288386] other info that might help us debug this:
> [ 516.288386] 2 locks held by swapper/0:
> [ 516.288386] #0: (&elist[idx].timer){+.-...}, at: [<c10334b5>] run_timer_softirq+0x11d/0x263
> [ 516.288386] #1: (rcu_read_lock){.+.+..}, at: [<c12eac78>] est_timer+0x0/0x1b4
> [ 516.288386]
> [ 516.288386] the shortest dependencies between 2nd lock and 1st lock:
> [ 516.288386] -> (est_tree_lock){+.+...} ops: 5 {
> [ 516.288386] HARDIRQ-ON-W at:
> [ 516.288386] [<c104a421>] __lock_acquire+0x306/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
> [ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
> [ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
> [ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
> [ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386] SOFTIRQ-ON-W at:
> [ 516.288386] [<c104a442>] __lock_acquire+0x327/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12ea9d8>] gen_kill_estimator+0x18/0x9b
> [ 516.288386] [<c12fea48>] qdisc_destroy+0x34/0x79
> [ 516.288386] [<c1300393>] notify_and_destroy+0x35/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c1300686>] tc_get_qdisc+0x125/0x157
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386] INITIAL USE at:
> [ 516.288386] [<c104a49a>] __lock_acquire+0x37f/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
> [ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
> [ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
> [ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
> [ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386] }
> [ 516.288386] ... key at: [<c15a70fc>] est_tree_lock+0x10/0x1c
> [ 516.288386] ... acquired at:
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12ea985>] gen_estimator_active+0x39/0x74
> [ 516.288386] [<c12ea82a>] gnet_stats_copy_rate_est+0x16/0x4d
> [ 516.288386] [<c1300000>] tc_fill_qdisc+0x186/0x1e2
> [ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
> [ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386]
> [ 516.288386] -> (&qdisc_tx_lock){+.-...} ops: 10 {
> [ 516.288386] HARDIRQ-ON-W at:
> [ 516.288386] [<c104a421>] __lock_acquire+0x306/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c55e1>] _raw_spin_lock_bh+0x2a/0x39
> [ 516.288386] [<c12ea8f1>] gnet_stats_start_copy_compat+0x27/0x70
> [ 516.288386] [<c12fffc7>] tc_fill_qdisc+0x14d/0x1e2
> [ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
> [ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386] IN-SOFTIRQ-W at:
> [ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
> [ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
> [ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
> [ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
> [ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
> [ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
> [ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
> [ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
> [ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
> [ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
> [ 516.288386] INITIAL USE at:
> [ 516.288386] [<c104a49a>] __lock_acquire+0x37f/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c55e1>] _raw_spin_lock_bh+0x2a/0x39
> [ 516.288386] [<c12ea8f1>] gnet_stats_start_copy_compat+0x27/0x70
> [ 516.288386] [<c12fffc7>] tc_fill_qdisc+0x14d/0x1e2
> [ 516.288386] [<c13001b2>] qdisc_notify+0x94/0xcf
> [ 516.288386] [<c1300385>] notify_and_destroy+0x27/0x3d
> [ 516.288386] [<c1300490>] qdisc_graft+0xf5/0x1c6
> [ 516.288386] [<c130110a>] tc_modify_qdisc+0x369/0x3b8
> [ 516.288386] [<c12f8d09>] rtnetlink_rcv_msg+0x197/0x1b1
> [ 516.288386] [<c1305cc0>] netlink_rcv_skb+0x30/0x75
> [ 516.288386] [<c12f8b6a>] rtnetlink_rcv+0x1e/0x26
> [ 516.288386] [<c1305b01>] netlink_unicast+0xc4/0x11a
> [ 516.288386] [<c13061a2>] netlink_sendmsg+0x23a/0x252
> [ 516.288386] [<c12e09fd>] __sock_sendmsg+0x4d/0x56
> [ 516.288386] [<c12e0e42>] sock_sendmsg+0x95/0xab
> [ 516.288386] [<c12e1024>] sys_sendmsg+0x13f/0x18f
> [ 516.288386] [<c12e2676>] sys_socketcall+0x155/0x1a3
> [ 516.288386] [<c13c5e25>] syscall_call+0x7/0xb
> [ 516.288386] }
> [ 516.288386] ... key at: [<c1b8318c>] qdisc_tx_lock+0x0/0x8
> [ 516.288386] ... acquired at:
> [ 516.288386] [<c1049bf3>] check_usage_forwards+0x5d/0x68
> [ 516.288386] [<c1049575>] mark_lock+0x102/0x1e0
> [ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
> [ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
> [ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
> [ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
> [ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
> [ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
> [ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
> [ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
> [ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
> [ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
> [ 516.288386]
> [ 516.288386]
> [ 516.288386] stack backtrace:
> [ 516.288386] Pid: 0, comm: swapper Not tainted 2.6.35b #7
> [ 516.288386] Call Trace:
> [ 516.288386] [<c13c3a01>] ? printk+0xf/0x16
> [ 516.288386] [<c1049b8b>] print_irq_inversion_bug+0xef/0xfa
> [ 516.288386] [<c1049bf3>] check_usage_forwards+0x5d/0x68
> [ 516.288386] [<c1049575>] mark_lock+0x102/0x1e0
> [ 516.288386] [<c1049b96>] ? check_usage_forwards+0x0/0x68
> [ 516.288386] [<c104a3c3>] __lock_acquire+0x2a8/0xbd6
> [ 516.288386] [<c104a49a>] ? __lock_acquire+0x37f/0xbd6
> [ 516.288386] [<c104ac2b>] ? __lock_acquire+0xb10/0xbd6
> [ 516.288386] [<c104ad9d>] lock_acquire+0xac/0xc8
> [ 516.288386] [<c12eacda>] ? est_timer+0x62/0x1b4
> [ 516.288386] [<c13c53ce>] _raw_spin_lock+0x25/0x34
> [ 516.288386] [<c12eacda>] ? est_timer+0x62/0x1b4
> [ 516.288386] [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] [<c10334b5>] ? run_timer_softirq+0x11d/0x263
> [ 516.288386] [<c1033536>] run_timer_softirq+0x19e/0x263
> [ 516.288386] [<c12eac78>] ? est_timer+0x0/0x1b4
> [ 516.288386] [<c102d692>] __do_softirq+0xab/0x169
> [ 516.288386] [<c102d77a>] do_softirq+0x2a/0x42
> [ 516.288386] [<c102d8d6>] irq_exit+0x36/0x42
> [ 516.288386] [<c1014705>] smp_apic_timer_interrupt+0x66/0x71
> [ 516.288386] [<c13c5fdf>] apic_timer_interrupt+0x2f/0x34
> [ 516.288386] [<c104007b>] ? create_new_namespaces+0x37/0x127
> [ 516.288386] [<c1007c0b>] ? default_idle+0x46/0x65
> [ 516.288386] [<c1001c13>] cpu_idle+0x31/0x5a
> [ 516.288386] [<c13bc316>] rest_init+0xda/0xdf
> [ 516.288386] [<c15ba7c3>] start_kernel+0x2d4/0x2d9
> [ 516.288386] [<c15ba09f>] i386_start_kernel+0x9f/0xa7
>
> ----------->
>
> This patch fixes a lockdep warning:
>
> [ 516.287584] =========================================================
> [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> [ 516.288386] 2.6.35b #7
> [ 516.288386] ---------------------------------------------------------
> [ 516.288386] swapper/0 just changed the state of lock:
> [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> [ 516.288386] (est_tree_lock){+.+...}
> [ 516.288386]
> [ 516.288386] and interrupts could create inverse lock ordering between them.
> ...
>
> So, est_tree_lock needs BH protection because it's taken by
> qdisc_tx_lock, which is used both in BH and process contexts.
> (Full warning with this patch at netdev, 02 Sep 2010.)
>
> Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> ---
>
> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> index 9fbe7f7..6743146 100644
> --- a/net/core/gen_estimator.c
> +++ b/net/core/gen_estimator.c
> @@ -232,7 +232,7 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
> est->last_packets = bstats->packets;
> est->avpps = rate_est->pps<<10;
>
> - spin_lock(&est_tree_lock);
> + spin_lock_bh(&est_tree_lock);
> if (!elist[idx].timer.function) {
> INIT_LIST_HEAD(&elist[idx].list);
> setup_timer(&elist[idx].timer, est_timer, idx);
> @@ -243,7 +243,7 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
>
> list_add_rcu(&est->list, &elist[idx].list);
> gen_add_node(est);
> - spin_unlock(&est_tree_lock);
> + spin_unlock_bh(&est_tree_lock);
>
> return 0;
> }
> @@ -270,7 +270,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
> {
> struct gen_estimator *e;
>
> - spin_lock(&est_tree_lock);
> + spin_lock_bh(&est_tree_lock);
> while ((e = gen_find_node(bstats, rate_est))) {
> rb_erase(&e->node, &est_root);
>
If you lock bh for est_tree_lock(), we dont need _bh for est_lock here
> @@ -281,7 +281,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
> list_del_rcu(&e->list);
> call_rcu(&e->e_rcu, __gen_kill_estimator);
> }
> - spin_unlock(&est_tree_lock);
> + spin_unlock_bh(&est_tree_lock);
> }
> EXPORT_SYMBOL(gen_kill_estimator);
>
> @@ -320,9 +320,9 @@ bool gen_estimator_active(const struct gnet_stats_basic_packed *bstats,
>
> ASSERT_RTNL();
>
> - spin_lock(&est_tree_lock);
> + spin_lock_bh(&est_tree_lock);
> res = gen_find_node(bstats, rate_est) != NULL;
> - spin_unlock(&est_tree_lock);
> + spin_unlock_bh(&est_tree_lock);
>
> return res;
> }
I must be very tired, I dont understand this patch.
Maybe after a night, I'll understand ?
est_tree_lock is only taken by process context, and I dont see what can
be the problem. Are you sure its not a lockdep false positive, or that
the real bug is elsewhere ?
Sure, we can block BH everywhere, it will reduce bugs and lockdep
alarms, but I would like to understand before, why its needed.
If you believe est_tree_lock can be taken by a softirq handler, please
tell me ;)
Thanks
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:32 ` Eric Dumazet
@ 2010-09-02 20:55 ` Jarek Poplawski
2010-09-03 5:26 ` Eric Dumazet
0 siblings, 1 reply; 8+ messages in thread
From: Jarek Poplawski @ 2010-09-02 20:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Thu, Sep 02, 2010 at 10:32:01PM +0200, Eric Dumazet wrote:
> Le jeudi 02 septembre 2010 ?? 13:22 -0700, David Miller a écrit :
> > From: Jarek Poplawski <jarkao2@gmail.com>
> > Date: Thu, 2 Sep 2010 22:12:58 +0200
> >
> > > This patch fixes a lockdep warning:
> > >
> > > [ 516.287584] =========================================================
> > > [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> > > [ 516.288386] 2.6.35b #7
> > > [ 516.288386] ---------------------------------------------------------
> > > [ 516.288386] swapper/0 just changed the state of lock:
> > > [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> > > [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> > > [ 516.288386] (est_tree_lock){+.+...}
> > > [ 516.288386]
> > > [ 516.288386] and interrupts could create inverse lock ordering between them.
> > > ...
> > >
> > > So, est_tree_lock needs BH protection because it's taken by
> > > qdisc_tx_lock, which is used both in BH and process contexts.
> > > (Full warning with this patch at netdev, 02 Sep 2010.)
> > >
> > > Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
> > >
> > > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> >
> > Applied, thanks Jarek.
> >
> > Eric, please make an effort to run with lockdep enabled when making
> > locking changes, just for fun :-)
>
> Hmm... I still trying to understand the problem.
>
> I dont understand Jarek analysis yet.
>
cpu1: cpu2:
(process) (process)
qdisc_tx_lock est_tree_lock
(waiting for est_tree_lock) (bh)
(waiting for qdisc_tx_lock)
Jarek P.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:53 ` Eric Dumazet
@ 2010-09-02 20:59 ` David Miller
2010-09-02 21:22 ` Jarek Poplawski
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2010-09-02 20:59 UTC (permalink / raw)
To: eric.dumazet; +Cc: jarkao2, netdev
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Sep 2010 22:53:33 +0200
> Maybe after a night, I'll understand ?
>
> est_tree_lock is only taken by process context, and I dont see what can
> be the problem. Are you sure its not a lockdep false positive, or that
> the real bug is elsewhere ?
>
> Sure, we can block BH everywhere, it will reduce bugs and lockdep
> alarms, but I would like to understand before, why its needed.
>
> If you believe est_tree_lock can be taken by a softirq handler, please
> tell me ;)
Hint: est->stats_lock is actually a pointer to a qdisc->lock
Therefore we must comply with the rules of qdisc locking if we wish
to take est->stats_lock in conjunction with est_tree_lock.
The strange mention of qdisc_tx_lock changing state inside of
est_timer() in the lockdep warning should have been the clue :-)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:53 ` Eric Dumazet
2010-09-02 20:59 ` David Miller
@ 2010-09-02 21:22 ` Jarek Poplawski
1 sibling, 0 replies; 8+ messages in thread
From: Jarek Poplawski @ 2010-09-02 21:22 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
On Thu, Sep 02, 2010 at 10:53:33PM +0200, Eric Dumazet wrote:
> Le jeudi 02 septembre 2010 ?? 22:12 +0200, Jarek Poplawski a écrit :
> > @@ -270,7 +270,7 @@ void gen_kill_estimator(struct gnet_stats_basic_packed *bstats,
> > {
> > struct gen_estimator *e;
> >
> > - spin_lock(&est_tree_lock);
> > + spin_lock_bh(&est_tree_lock);
> > while ((e = gen_find_node(bstats, rate_est))) {
> > rb_erase(&e->node, &est_root);
> >
>
> If you lock bh for est_tree_lock(), we dont need _bh for est_lock here
Right. But since the patch was merged and it's not crucial, I guess
you could send a separate patch?
Thanks,
Jarek P.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator
2010-09-02 20:55 ` Jarek Poplawski
@ 2010-09-03 5:26 ` Eric Dumazet
0 siblings, 0 replies; 8+ messages in thread
From: Eric Dumazet @ 2010-09-03 5:26 UTC (permalink / raw)
To: Jarek Poplawski; +Cc: David Miller, netdev
Le jeudi 02 septembre 2010 à 22:55 +0200, Jarek Poplawski a écrit :
> On Thu, Sep 02, 2010 at 10:32:01PM +0200, Eric Dumazet wrote:
> > Le jeudi 02 septembre 2010 ?? 13:22 -0700, David Miller a écrit :
> > > From: Jarek Poplawski <jarkao2@gmail.com>
> > > Date: Thu, 2 Sep 2010 22:12:58 +0200
> > >
> > > > This patch fixes a lockdep warning:
> > > >
> > > > [ 516.287584] =========================================================
> > > > [ 516.288386] [ INFO: possible irq lock inversion dependency detected ]
> > > > [ 516.288386] 2.6.35b #7
> > > > [ 516.288386] ---------------------------------------------------------
> > > > [ 516.288386] swapper/0 just changed the state of lock:
> > > > [ 516.288386] (&qdisc_tx_lock){+.-...}, at: [<c12eacda>] est_timer+0x62/0x1b4
> > > > [ 516.288386] but this lock took another, SOFTIRQ-unsafe lock in the past:
> > > > [ 516.288386] (est_tree_lock){+.+...}
> > > > [ 516.288386]
> > > > [ 516.288386] and interrupts could create inverse lock ordering between them.
> > > > ...
> > > >
> > > > So, est_tree_lock needs BH protection because it's taken by
> > > > qdisc_tx_lock, which is used both in BH and process contexts.
> > > > (Full warning with this patch at netdev, 02 Sep 2010.)
> > > >
> > > > Fixes commit: ae638c47dc040b8def16d05dc6acdd527628f231
> > > >
> > > > Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> > >
> > > Applied, thanks Jarek.
> > >
> > > Eric, please make an effort to run with lockdep enabled when making
> > > locking changes, just for fun :-)
> >
> > Hmm... I still trying to understand the problem.
> >
> > I dont understand Jarek analysis yet.
> >
>
> cpu1: cpu2:
> (process) (process)
> qdisc_tx_lock est_tree_lock
> (waiting for est_tree_lock) (bh)
> (waiting for qdisc_tx_lock)
>
Cool. Reading commit ae638c47dc040b, I thought only RTNL was possibly
hold in these paths.
Thanks Jarek !
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-09-03 5:26 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-09-02 20:12 [PATCH] pkt_sched: Fix lockdep warning on est_tree_lock in gen_estimator Jarek Poplawski
2010-09-02 20:22 ` David Miller
2010-09-02 20:32 ` Eric Dumazet
2010-09-02 20:55 ` Jarek Poplawski
2010-09-03 5:26 ` Eric Dumazet
2010-09-02 20:53 ` Eric Dumazet
2010-09-02 20:59 ` David Miller
2010-09-02 21:22 ` Jarek Poplawski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).