On 9/15/26 2:24 PM, 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 how many flows one callback emits, and a large but > sparse table adds only a walk over empty buckets, so no dumper carries > a budget of its own. Neither bounds the softirq work the callback > 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. That > region is bounded by the skb and the table size, where the softirq > backlog it used to absorb is not.
Sashiko argues that the table size is not really bounded, which is fair, so it may be good to try and reword the argument a bit. Maybe point again to the fact that these buckets are empty and the walk should be fast enough. Not very important, only asking because there is a couple of things to change below anyway. > > Signed-off-by: Denis V. Lunev <[email protected]> > --- > v2: > - 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, add the net prefix You say here that the net prefix was added, but it wasn't. It should be '[PATCH net v2]'. But also, if you're targeting the net tree then, as also noted by the sashiko, you need a Fixes tag (63e7959c4b9b would work, I suppose) and the Cc for stable in the commit message tag section. Also, please, add links to previous versions of the patch here in the changelog section. Especially if you're renaming the patch between versions. > > net/openvswitch/datapath.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c > index 631a03136fa1..a80bac81c043 100644 > --- a/net/openvswitch/datapath.c > +++ b/net/openvswitch/datapath.c > @@ -1532,6 +1532,11 @@ 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 here. The word 'here' reads strange, I'd suggest removing it. > + */ > + local_bh_disable(); Since the comment only applies to the local_bh_disable(), I'd suggest having an empty line here. > ti = rcu_dereference(dp->table.ti); > for (;;) { > struct sw_flow *flow; > @@ -1552,6 +1557,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, > struct netlink_callback *cb) > cb->args[0] = bucket; > cb->args[1] = obj; > } And an empty line here for symmetry. > + local_bh_enable(); > rcu_read_unlock(); > return skb->len; > } _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
