On Wed, Mar 19, 2025 at 11:12:24AM -0700, Breno Leitao wrote:
> 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>
The IPIs are not fun, but in the interest of getting *some* solution
moving forward... ;-)
Reviewed-by: Paul E. McKenney <paul...@kernel.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);
>
>
