Messages in this thread |  | | From | Adrián Moreno <> | | Date | Wed, 13 May 2026 03:49:08 -0700 | | Subject | Re: [PATCH net-next v3 2/2] net: openvswitch: decouple flow_table from ovs_mutex |
| |
On Thu, May 07, 2026 at 01:31:24PM +0200, Paolo Abeni wrote: > 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.
I'll give it a try.
> > > @@ -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.
Good point! I'll fix it.
> > @@ -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.
OK. I'll consolidate it in an independent follow-up patch.
> > > +/* 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 >
|  |