On 9/29/26 9:25 AM, Denis V. Lunev wrote: > From: Denis V. Lunev <[email protected]> > > A production compute node carrying a few thousand datapath flows hit a > soft lockup inside a single netlink flow dump and panicked. > > ovs_flow_cmd_dump() calls ovs_flow_stats_get() for every flow it > emits, and that releases stats->lock with spin_unlock_bh() once per > CPU that has touched the flow. Every release is a local_bh_enable(), > and each one runs the pending softirq backlog in the dumping thread's > own context. > > The skb bounds the flows one callback emits, but not the softirq work > it absorbs. On a CPU that carries the box's packet load the backlog > refills as fast as it drains, so the dumping thread becomes that CPU's > softirq engine. It never sleeps and it has no reschedule point, so > under voluntary preemption nothing can take the CPU away from it: > neither the ksoftirqd the kernel woke to take the work over, nor the > stopper thread the softlockup detector dispatches to refresh its > timestamp. > > Hold BH off across the whole callback instead, the way > ctnetlink_dump_table() does, so the nested spin_unlock_bh() stop > draining softirqs. The loop already runs under rcu_read_lock() and > cannot sleep. What it gives up is preemption under CONFIG_PREEMPT, > since a BH-off region is not preemptible outside PREEMPT_RT. The > region stays short: the skb caps the flows one callback emits, and > empty buckets cost no skb space but are each visited once per dump, as > the cursor only moves forward. The table grows on insert and shrinks > only on flush, so the walk is bounded by the largest flow count the > datapath has held. The softirq backlog the callback used to absorb has > no bound at all. > > Fixes: 63e7959c4b9b ("openvswitch: Per NUMA node flow stats.") > Cc: [email protected] > Signed-off-by: Denis V. Lunev <[email protected]> > --- > v3: > - add the net prefix, Fixes tag and Cc stable > - explain why the BH-off walk stays bounded: the cursor visits each > empty bucket once per dump and the table shrinks only on flush > - drop "here" from the comment, add blank lines around the > local_bh_disable()/local_bh_enable() pair > v2: https://lore.kernel.org/netdev/[email protected]/ > - leave ovs_vport_cmd_dump() alone: nsid_lock has not been BH-safe > since commit aed4969f2bdf ("net: net->nsid_lock does not need BH > safety"), so the vport dump never drained softirqs > - disable BH before the table dereference and say in a comment that > the region is not there for safety > - drop the ovs_flow_stats_get() history, note the empty-bucket walk > and the lost CONFIG_PREEMPT preemption in the message > - move the Cc list out of the commit message > v1: https://lore.kernel.org/netdev/[email protected]/ > > net/openvswitch/datapath.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 631a03136fa1..4fc5d0bebd85 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -1532,6 +1532,12 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, > struct netlink_callback *cb) > return -ENODEV; > } > > + /* > + * Not needed for safety. Stops every spin_unlock_bh() in > + * ovs_flow_stats_get() from running the softirq backlog. > + */ > + local_bh_disable(); > + > ti = rcu_dereference(dp->table.ti); > for (;;) { > struct sw_flow *flow; > @@ -1552,6 +1558,8 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, > struct netlink_callback *cb) > cb->args[0] = bucket; > cb->args[1] = obj; > } > + > + local_bh_enable(); > rcu_read_unlock(); > return skb->len; > }
Reviewed-by: Ilya Maximets <[email protected]> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
