On 9/14/26 10:27 PM, Ilya Maximets wrote:
> 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,
Somehow sashiko didn't send an email, but it points out that this is
not exactly true, as while the reply is limited by the skb size, the
work may not be in case the table is large but empty. This should
not be a big problem, but it's better to mention in the commit message.
>> 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 ?
It was a ovs_flow_stats_get(), I see, but then "exactly this" is even
more strange claim as you're locking around the whole dump and not per
flow.
>
>> 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,
Sashiko also points out:
> Before this change the loop held only rcu_read_lock() and was preemptible
> under CONFIG_PREEMPT; with local_bh_disable() the whole callback is
> softirq-off and, on !PREEMPT_RT, non-preemptible, and there is no reschedule
> point possible inside it.
So, the "forbids nothing that was allowed before" claim is not fully correct.
>> 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]>
This CC list doesn't need to be in the commit message, it can go under
the cut line.
Also, please, include the tree you're targeting in the subject prefix.
Best regards, Ilya Maximets.
>> ---
>> 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
