On 9/9/26 7:19 PM, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
>
> ovs_flow_cmd_dump() and ovs_vport_cmd_dump() release a BH-disabling
> spinlock once per item they emit: stats->lock in ovs_flow_stats_get()
> for every CPU that has touched the flow, and nsid_lock in
> peernet2id_alloc() for every vport that lives in a foreign netns.
This doesn't seem to be the case since last year:
aed4969f2bdf ("net: net->nsid_lock does not need BH safety")
> 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 much work a dump callback does, which is why no
> dumper carries a budget of its own, but it does not bound 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. The watchdog then panics a node that has enough flows and
> ports to keep the dump running.
>
> ovs_flow_stats_get()
ovs_flow_cmd_dump ?
> used to do exactly this. One local_bh_disable()
> around the whole per-CPU walk was added by commit 4f647e0a3c37
> ("openvswitch: fix a possible deadlock and lockdep warning") to close
> an ABBA deadlock between two CPUs reading each other's stats.
> Commit 63e7959c4b9b ("openvswitch: Per NUMA node flow stats.") dropped
> that region the same day while reworking the stats layout, and replaced
> it with a spin_lock_bh() per item. The deadlock stayed fixed; the
> single region did not come back.
"used to do exactly this" is not a valid argument as it never actually
did this in reality. Patches you mentioned were developed more or less
together with all parties aware of each other's work and applied within
a short window.
>
> Hold BH off across the whole callback instead, the way
> ctnetlink_dump_table() does. Both loops already run under
> rcu_read_lock() and cannot sleep, so this forbids nothing that was
> allowed before, and the nested spin_unlock_bh() in the callees stop
> draining softirqs.
This sounds fine to me, though as mentioned above, the vport dump
doesn't need the change, AFAIU.
>
> Signed-off-by: Denis V. Lunev <[email protected]>
> CC: Aaron Conole <[email protected]>
> CC: Eelco Chaudron <[email protected]>
> CC: Ilya Maximets <[email protected]>
> CC: "David S. Miller" <[email protected]>
> CC: Eric Dumazet <[email protected]>
> CC: Jakub Kicinski <[email protected]>
> CC: Paolo Abeni <[email protected]>
> CC: Simon Horman <[email protected]>
> ---
> net/openvswitch/datapath.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 631a03136fa1..ae2c9924aeb0 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1533,6 +1533,7 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> }
>
> ti = rcu_dereference(dp->table.ti);
> + local_bh_disable();
I'd put this before the dereference to avoid breaking the logical
block. It may also be good to add a small comment on why it is
done here, as it is not needed for safety.
> for (;;) {
> struct sw_flow *flow;
> u32 bucket, obj;
> @@ -1552,6 +1553,7 @@ 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;
> }
> @@ -2565,6 +2567,7 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> rcu_read_unlock();
> return -ENODEV;
> }
> + local_bh_disable();
> for (i = bucket; i < DP_VPORT_HASH_BUCKETS; i++) {
> struct vport *vport;
>
> @@ -2585,6 +2588,7 @@ static int ovs_vport_cmd_dump(struct sk_buff *skb,
> struct netlink_callback *cb)
> skip = 0;
> }
> out:
> + local_bh_enable();
> rcu_read_unlock();
>
> cb->args[0] = i;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
