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 <[email protected]>
> > ---
> > 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
