Re: [PATCH net-next v3 2/2] net: openvswitch: decouple flow_table from ovs_mutex

[Paolo Abeni <pabeni@redhat.com>]

On 5/5/26 10:42 AM, Adrian Moreno wrote:
> In order to protect flow operations from RTNL contention, this patch
> decouples flow_table modifications from ovs_mutex by means of the
> following:
> 
> 1 - Create a new mutex inside the flow_table that protects it from
> concurrent modifications.
> Putting the mutex inside flow_table makes it easier to consume for
> functions inside flow_table.c that do not currently take pointers to the
> datapath.
> Some function signatures need to be changed to accept flow_table so that
> lockdep checks can be performed.
> 
> 2 - Create a reference count to temporarily extend rcu protection from
> the datapath to the flow_table.
> One reference is held by the datapath, the other is temporarily
> increased during flow modifications.
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  net/openvswitch/datapath.c   | 230 ++++++++++++++++++++++-------------
>  net/openvswitch/flow.c       |  13 +-
>  net/openvswitch/flow.h       |   9 +-
>  net/openvswitch/flow_table.c | 173 ++++++++++++++++----------
>  net/openvswitch/flow_table.h |  53 +++++++-
>  5 files changed, 318 insertions(+), 160 deletions(-)

This is still considerably big. I'm wondering if introducing the
lockdep_ovs_tbl_is_held/rcu_dereference_ovs_tbl annotations with a
separate earlier patch would make it more palatable? Just a very wild
guess; if the result is ugly (or you have string feeling that would be)
please ignore.

> @@ -1112,7 +1132,8 @@ static int ovs_flow_cmd_new(struct sk_buff *skb, struct genl_info *info)
>  						       ufid_flags);
>  			BUG_ON(error < 0);
>  		}
> -		ovs_unlock();
> +		mutex_unlock(&table->lock);
> +		ovs_flow_tbl_put(table);

Minot nit: you can consolidate 2 ovs_flow_tbl_put() calls after the
if/than/else statement.

@@ -524,9 +540,22 @@ void ovs_flow_tbl_destroy_rcu(struct rcu_head *rcu)
>  	call_rcu(&mc->rcu, mask_cache_rcu_cb);
>  	call_rcu(&ma->rcu, mask_array_rcu_cb);
>  	table_instance_destroy(ti, ufid_ti);
> +	mutex_destroy(&table->lock);
>  	kfree(table);
>  }
>  
> +void ovs_flow_tbl_put(struct flow_table *table)
> +{
> +	if (refcount_dec_and_test(&table->refcnt)) {
> +		mutex_lock(&table->lock);
> +		table_instance_flow_flush(table,
> +					  ovs_tbl_dereference(table->ti, table),
> +					  ovs_tbl_dereference(table->ufid_ti, table));
> +		mutex_unlock(&table->lock);

As mentioned in the previous patch you can follow-up moving here the

	call_rcu(&mc->rcu, mask_cache_rcu_cb);
 	call_rcu(&ma->rcu, mask_array_rcu_cb);

currently in ovs_flow_tbl_destroy_rcu.

> +/* Must be called with flow_table->lock held. */
>  int ovs_flow_tbl_flush(struct flow_table *flow_table)
>  {
>  	struct table_instance *old_ti, *new_ti;
>  	struct table_instance *old_ufid_ti, *new_ufid_ti;
>  
> +	ASSERT_OVS_TBL(flow_table);

Minor nit: adding the assert and the comment is redundant. I think the
assert alone would be better. There are other similar later occurrences.

/P