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