On Wed, Mar 19, 2025 at 09:05:07AM -0700, Paul E. McKenney wrote:
> > I think we should redesign lockdep_unregister_key() to work on a separately
> > allocated piece of memory,
> > then use kfree_rcu() in it.
> >
> > Ie not embed a "struct lock_class_key" in the struct Qdisc, but a pointer to
> >
> > struct ... {
> > struct lock_class_key;
> > struct rcu_head rcu;
> > }
>
> Works for me!
I've tested a different approach, using synchronize_rcu_expedited()
instead of synchronize_rcu(), given how critical this function is
called, and the command performance improves dramatically.
This approach has some IPI penalties, but, it might be quicker to review
and get merged, mitigating the network issue.
Does it sound a bad approach?
Date: Wed Mar 19 10:23:56 2025 -0700
lockdep: Speed up lockdep_unregister_key() with expedited RCU
synchronization
lockdep_unregister_key() is called from critical code paths, including
sections where rtnl_lock() is held. When replacing a qdisc in a network
device, network egress traffic is disabled while __qdisc_destroy() is
called for every queue. This function calls lockdep_unregister_key(),
which was blocked waiting for synchronize_rcu() to complete.
For example, a simple tc command to replace a qdisc could take 13
seconds:
# time /usr/sbin/tc qdisc replace dev eth0 root handle 0x1234: mq
real 0m13.195s
user 0m0.001s
sys 0m2.746s
During this time, network egress is completely frozen while waiting for
RCU synchronization.
Use synchronize_rcu_expedite() instead to minimize the impact on
critical operations like network connectivity changes.
Signed-off-by: Breno Leitao <lei...@debian.org>
diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index 4470680f02269..96b87f1853f4f 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -6595,8 +6595,10 @@ void lockdep_unregister_key(struct lock_class_key *key)
if (need_callback)
call_rcu(&delayed_free.rcu_head, free_zapped_rcu);
- /* Wait until is_dynamic_key() has finished accessing k->hash_entry. */
- synchronize_rcu();
+ /* Wait until is_dynamic_key() has finished accessing k->hash_entry.
+ * This needs to be quick, since it is called in critical sections
+ */
+ synchronize_rcu_expedite();
}
EXPORT_SYMBOL_GPL(lockdep_unregister_key);
