[PATCH 1/1] openvswitch: disable BH once per netlink dump instead of per item

[PATCH 1/1] openvswitch: disable BH once per netlink dump instead of per item

[Denis V. Lunev]

From: Denis V. Lunev <den@openvz.org>

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.
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() 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.

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.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Aaron Conole <aconole@redhat.com>
CC: Eelco Chaudron <echaudro@redhat.com>
CC: Ilya Maximets <i.maximets@ovn.org>
CC: "David S. Miller" <davem@davemloft.net>
CC: Eric Dumazet <edumazet@google.com>
CC: Jakub Kicinski <kuba@kernel.org>
CC: Paolo Abeni <pabeni@redhat.com>
CC: Simon Horman <horms@kernel.org>
---
 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();
 	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;
-- 
2.53.0