Re: [ovs-dev] [PATCH net-next 11/18] openvswitch: Use nested-BH locking for ovs_actions.

On 2025-03-13 14:23:16 [+0100], Ilya Maximets wrote:

quoted

originate from within the recursion.

It's true that ovs_packet_cmd_execute() can not be re-intered, while
ovs_dp_process_packet() can be re-entered if the packet leaves OVS and
then comes back from another port.  It's still better to handle all the
locking within datapath.c and not lock for RT in actions.c and for non-RT
in datapath.c.

Okay.

quoted

quoted

quoted

quoted

Also, the name of the struct ovs_action doesn't make a lot of sense,
I'd suggest to call it pcpu_storage or something like that instead.
I.e. have a more generic name as the fields inside are not directly
related to each other.

Understood. ovs_pcpu_storage maybe?

It's OK, I guess, but see also a point about locking inside datapath.c
instead and probably not needing to change anything in actions.c.

If you say that adding a lock to ovs_dp_process_packet() and another to
ovs_packet_cmd_execute() then I can certainly update. However based on
what I wrote above, I am not sure.

I think, it's better if we keep all the locks in datapath.c and let
actions.c assume that all the operations are always safe as it was
originally intended.

If you say so. Then I move the logic to the two callers to datapath.c
then. But I would need the same recursive lock-detection as I currently
have in ovs_dp_process_packet(). That means we would have the lock
datapath.c and the data structure it protects in actions.c.

Cc: Aaron and Eelco, in case they have some thoughts on this as well.

While at it, I would keep "openvswitch: Merge three per-CPU structures
into one." since it looks like a nice clean up.

Best regards, Ilya Maximets.

Sebastian