Messages in this thread |  | | Date | Thu, 7 May 2026 13:31:24 +0200 | | Subject | Re: [PATCH net-next v3 2/2] net: openvswitch: decouple flow_table from ovs_mutex | | From | Paolo Abeni <> |
| |
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
|  |