| Message ID | 73065927a49619fcd60e5b765c929f899a66cd1a.1701853200.git.dcaratti@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/sched: fix false lockdep warning on qdisc root lock | expand |
On Wed, Dec 6, 2023 at 10:04 AM Davide Caratti <dcaratti@redhat.com> wrote: > > Xiumei and Cristoph reported the following lockdep splat, it complains of > the qdisc root being taken twice: > > ============================================ > WARNING: possible recursive locking detected > 6.7.0-rc3+ #598 Not tainted > -------------------------------------------- > swapper/2/0 is trying to acquire lock: > ffff888177190110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > > but task is already holding lock: > ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > > other info that might help us debug this: > Possible unsafe locking scenario: > > CPU0 > ---- > lock(&sch->q.lock); > lock(&sch->q.lock); > > *** DEADLOCK *** > > May be due to missing lock nesting notation > > 5 locks held by swapper/2/0: > #0: ffff888135a09d98 ((&in_dev->mr_ifc_timer)){+.-.}-{0:0}, at: call_timer_fn+0x11a/0x510 > #1: ffffffffaaee5260 (rcu_read_lock){....}-{1:2}, at: ip_finish_output2+0x2c0/0x1ed0 > #2: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 > #3: ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > #4: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 > > Can you add a Fixes: tag ? Also, what is the interaction with htb_set_lockdep_class_child(), have you tried to use HTB after your patch ? Could htb_set_lockdep_class_child() be removed ? > CC: Xiumei Mu <xmu@redhat.com> > Reported-by: Cristoph Paasch <cpaasch@apple.com> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451 > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > include/net/sch_generic.h | 1 + > net/sched/sch_generic.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > index dcb9160e6467..a395ca76066c 100644 > --- a/include/net/sch_generic.h > +++ b/include/net/sch_generic.h > @@ -126,6 +126,7 @@ struct Qdisc { > > struct rcu_head rcu; > netdevice_tracker dev_tracker; > + struct lock_class_key root_lock_key; > /* private data */ > long privdata[] ____cacheline_aligned; > }; > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > index 8dd0e5925342..da3e1ea42852 100644 > --- a/net/sched/sch_generic.c > +++ b/net/sched/sch_generic.c > @@ -944,7 +944,9 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, > __skb_queue_head_init(&sch->gso_skb); > __skb_queue_head_init(&sch->skb_bad_txq); > gnet_stats_basic_sync_init(&sch->bstats); > + lockdep_register_key(&sch->root_lock_key); > spin_lock_init(&sch->q.lock); > + lockdep_set_class(&sch->q.lock, &sch->root_lock_key); > > if (ops->static_flags & TCQ_F_CPUSTATS) { > sch->cpu_bstats = > @@ -1064,6 +1066,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc) > if (ops->destroy) > ops->destroy(qdisc); > > + lockdep_unregister_key(&qdisc->root_lock_key); > module_put(ops->owner); > netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker); > > -- > 2.41.0 >
On Wed, Dec 6, 2023 at 11:16 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Dec 6, 2023 at 10:04 AM Davide Caratti <dcaratti@redhat.com> wrote: > > > > Xiumei and Cristoph reported the following lockdep splat, it complains of > > the qdisc root being taken twice: > > > > ============================================ > > WARNING: possible recursive locking detected > > 6.7.0-rc3+ #598 Not tainted > > -------------------------------------------- > > swapper/2/0 is trying to acquire lock: > > ffff888177190110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > > > > but task is already holding lock: > > ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > > > > other info that might help us debug this: > > Possible unsafe locking scenario: > > > > CPU0 > > ---- > > lock(&sch->q.lock); > > lock(&sch->q.lock); > > > > *** DEADLOCK *** > > > > May be due to missing lock nesting notation > > > > 5 locks held by swapper/2/0: > > #0: ffff888135a09d98 ((&in_dev->mr_ifc_timer)){+.-.}-{0:0}, at: call_timer_fn+0x11a/0x510 > > #1: ffffffffaaee5260 (rcu_read_lock){....}-{1:2}, at: ip_finish_output2+0x2c0/0x1ed0 > > #2: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 > > #3: ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 > > #4: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 > > > > > > Can you add a Fixes: tag ? > > Also, what is the interaction with htb_set_lockdep_class_child(), have > you tried to use HTB after your patch ? > > Could htb_set_lockdep_class_child() be removed ? > > > > CC: Xiumei Mu <xmu@redhat.com> > > Reported-by: Cristoph Paasch <cpaasch@apple.com> > > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451 > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > --- > > include/net/sch_generic.h | 1 + > > net/sched/sch_generic.c | 3 +++ > > 2 files changed, 4 insertions(+) > > > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > > index dcb9160e6467..a395ca76066c 100644 > > --- a/include/net/sch_generic.h > > +++ b/include/net/sch_generic.h > > @@ -126,6 +126,7 @@ struct Qdisc { > > > > struct rcu_head rcu; > > netdevice_tracker dev_tracker; > > + struct lock_class_key root_lock_key; > > /* private data */ > > long privdata[] ____cacheline_aligned; > > }; > > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c > > index 8dd0e5925342..da3e1ea42852 100644 > > --- a/net/sched/sch_generic.c > > +++ b/net/sched/sch_generic.c > > @@ -944,7 +944,9 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, > > __skb_queue_head_init(&sch->gso_skb); > > __skb_queue_head_init(&sch->skb_bad_txq); > > gnet_stats_basic_sync_init(&sch->bstats); > > + lockdep_register_key(&sch->root_lock_key); > > spin_lock_init(&sch->q.lock); > > + lockdep_set_class(&sch->q.lock, &sch->root_lock_key); > > > > if (ops->static_flags & TCQ_F_CPUSTATS) { > > sch->cpu_bstats = > > @@ -1064,6 +1066,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc) > > if (ops->destroy) > > ops->destroy(qdisc); > > > > + lockdep_unregister_key(&qdisc->root_lock_key); lockdep_unregister_key() has a synchronize_rcu() call. This would slow down qdisc dismantle too much. I think we need to find another solution to this problem.
diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index dcb9160e6467..a395ca76066c 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -126,6 +126,7 @@ struct Qdisc { struct rcu_head rcu; netdevice_tracker dev_tracker; + struct lock_class_key root_lock_key; /* private data */ long privdata[] ____cacheline_aligned; }; diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 8dd0e5925342..da3e1ea42852 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -944,7 +944,9 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue, __skb_queue_head_init(&sch->gso_skb); __skb_queue_head_init(&sch->skb_bad_txq); gnet_stats_basic_sync_init(&sch->bstats); + lockdep_register_key(&sch->root_lock_key); spin_lock_init(&sch->q.lock); + lockdep_set_class(&sch->q.lock, &sch->root_lock_key); if (ops->static_flags & TCQ_F_CPUSTATS) { sch->cpu_bstats = @@ -1064,6 +1066,7 @@ static void __qdisc_destroy(struct Qdisc *qdisc) if (ops->destroy) ops->destroy(qdisc); + lockdep_unregister_key(&qdisc->root_lock_key); module_put(ops->owner); netdev_put(qdisc_dev(qdisc), &qdisc->dev_tracker);
Xiumei and Cristoph reported the following lockdep splat, it complains of the qdisc root being taken twice: ============================================ WARNING: possible recursive locking detected 6.7.0-rc3+ #598 Not tainted -------------------------------------------- swapper/2/0 is trying to acquire lock: ffff888177190110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 but task is already holding lock: ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 other info that might help us debug this: Possible unsafe locking scenario: CPU0 ---- lock(&sch->q.lock); lock(&sch->q.lock); *** DEADLOCK *** May be due to missing lock nesting notation 5 locks held by swapper/2/0: #0: ffff888135a09d98 ((&in_dev->mr_ifc_timer)){+.-.}-{0:0}, at: call_timer_fn+0x11a/0x510 #1: ffffffffaaee5260 (rcu_read_lock){....}-{1:2}, at: ip_finish_output2+0x2c0/0x1ed0 #2: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 #3: ffff88811995a110 (&sch->q.lock){+.-.}-{2:2}, at: __dev_queue_xmit+0x1560/0x2e70 #4: ffffffffaaee5200 (rcu_read_lock_bh){....}-{1:2}, at: __dev_queue_xmit+0x209/0x2e70 stack backtrace: CPU: 2 PID: 0 Comm: swapper/2 Not tainted 6.7.0-rc3+ #598 Hardware name: Red Hat KVM, BIOS 1.13.0-2.module+el8.3.0+7353+9de0a3cc 04/01/2014 Call Trace: <IRQ> dump_stack_lvl+0x4a/0x80 __lock_acquire+0xfdd/0x3150 lock_acquire+0x1ca/0x540 _raw_spin_lock+0x34/0x80 __dev_queue_xmit+0x1560/0x2e70 tcf_mirred_act+0x82e/0x1260 [act_mirred] tcf_action_exec+0x161/0x480 tcf_classify+0x689/0x1170 prio_enqueue+0x316/0x660 [sch_prio] dev_qdisc_enqueue+0x46/0x220 __dev_queue_xmit+0x1615/0x2e70 ip_finish_output2+0x1218/0x1ed0 __ip_finish_output+0x8b3/0x1350 ip_output+0x163/0x4e0 igmp_ifc_timer_expire+0x44b/0x930 call_timer_fn+0x1a2/0x510 run_timer_softirq+0x54d/0x11a0 __do_softirq+0x1b3/0x88f irq_exit_rcu+0x18f/0x1e0 sysvec_apic_timer_interrupt+0x6f/0x90 </IRQ> This happens when TC does a mirred egress redirect from the root qdisc of device A to the root qdisc of device B. As long as these two locks aren't protecting the same qdisc, they can be acquired in chain: add a per-qdisc lockdep class to silence false warnings. CC: Xiumei Mu <xmu@redhat.com> Reported-by: Cristoph Paasch <cpaasch@apple.com> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/451 Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- include/net/sch_generic.h | 1 + net/sched/sch_generic.c | 3 +++ 2 files changed, 4 insertions(+)