Re: [PATCH net-next v2 03/14] net: bridge: mcast: avoid sleeping on bridge-down
From: Ido Schimmel <idosch@nvidia.com>
Date: 2026-02-08 16:02:00
Also in:
bridge, lkml
On Fri, Feb 06, 2026 at 03:52:09AM +0100, Linus Lüssing wrote:
We later want to use the multicast lock when setting the bridge
interface up or down, to be able to atomically both check all conditions
to toggle the multicast active state and to subsequently toggle it.
While most variables we check / contexts we check from are serialized
(toggled variables through netlink/sysfs) the timer_pending() check is
not and might run in parallel.
However so far we are not allowed to spinlock __br_multicast_stop() as
its call to timer_delete_sync() might sleep. Therefore replacing the
sleeping variant with the non-sleeping one. It is sufficient to only
wait for any timer callback to finish when we are freeing the multicast
context.
Using the timer_shutdown() instead of the timer_delete() variant also
allows us to detect that we are stopping from within the according timer
callbacks, to retain the promise of the previous timer_delete_sync()
calls that no multicast state is changed after these
timer_{delete,shutdown}*() calls. And more importantly that we are not
inadvertently rearming timers.Can you clarify what you mean by "allows us to detect that we are stopping from within the according timer callbacks"?
quoted hunk ↗ jump to hunk
This new check also makes the netif_running() check redundant/obsolete in these contexts. Signed-off-by: Linus Lüssing <redacted> --- net/bridge/br_device.c | 4 ++ net/bridge/br_multicast.c | 108 ++++++++++++++++++++++++++------------ net/bridge/br_private.h | 5 ++ net/bridge/br_vlan.c | 5 ++ 4 files changed, 87 insertions(+), 35 deletions(-)diff --git a/net/bridge/br_device.c b/net/bridge/br_device.c index a818fdc22da9..d9d1227d5708 100644 --- a/net/bridge/br_device.c +++ b/net/bridge/br_device.c@@ -168,7 +168,9 @@ static int br_dev_open(struct net_device *dev) netdev_update_features(dev); netif_start_queue(dev); br_stp_enable_bridge(br); + spin_lock_bh(&br->multicast_lock); br_multicast_open(br);
Maybe move the spin_lock_bh() / spin_unlock_bh() to br_multicast_open() and have it call br_multicast_open_locked() that will also be invoked from br_multicast_toggle()?
quoted hunk ↗ jump to hunk
+ spin_unlock_bh(&br->multicast_lock); if (br_opt_get(br, BROPT_MULTICAST_ENABLED)) br_multicast_join_snoopers(br);@@ -191,7 +193,9 @@ static int br_dev_stop(struct net_device *dev) struct net_bridge *br = netdev_priv(dev); br_stp_disable_bridge(br); + spin_lock_bh(&br->multicast_lock); br_multicast_stop(br);
And like br_multicast_open(), move the locking into br_multicast_stop()?
quoted hunk ↗ jump to hunk
+ spin_unlock_bh(&br->multicast_lock); if (br_opt_get(br, BROPT_MULTICAST_ENABLED)) br_multicast_leave_snoopers(br);diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index dccae08b4f4c..f5a368dd20a3 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c@@ -1665,6 +1665,14 @@ static void br_multicast_router_expired(struct net_bridge_mcast_port *pmctx, spin_unlock(&br->multicast_lock); } +static bool br_multicast_stopping(struct net_bridge *br,
Nit: br_multicast_is_stopping() ?
+ struct timer_list *timer)
+{
+ lockdep_assert_held_once(&br->multicast_lock);
+
+ return !timer->function;
+}