On Fri, Jan 19, 2024 at 2:50 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 1/11/24 16:31, num...@ovn.org wrote:
> > +
> > +void
> > +lflow_table_add_lflow(struct lflow_table *lflow_table,
> > + const struct ovn_datapath *od,
> > + const unsigned long *dp_bitmap, size_t
dp_bitmap_len,
> > + enum ovn_stage stage, uint16_t priority,
> > + const char *match, const char *actions,
> > + const char *io_port, const char *ctrl_meter,
> > + const struct ovsdb_idl_row *stage_hint,
> > + const char *where,
> > + struct lflow_ref *lflow_ref)
> > + OVS_EXCLUDED(fake_hash_mutex)
> > +{
> > + struct ovs_mutex *hash_lock;
> > + uint32_t hash;
> > +
> > + ovs_assert(!od ||
> > + ovn_stage_to_datapath_type(stage) ==
ovn_datapath_get_type(od));
> > +
> > + hash = ovn_logical_flow_hash(ovn_stage_get_table(stage),
> > + ovn_stage_get_pipeline(stage),
> > + priority, match,
> > + actions);
> > +
> > + hash_lock = lflow_hash_lock(&lflow_table->entries, hash);
> > + struct ovn_lflow *lflow =
> > + do_ovn_lflow_add(lflow_table, od, dp_bitmap,
> > + dp_bitmap_len, hash, stage,
> > + priority, match, actions,
> > + io_port, ctrl_meter, stage_hint, where);
> > +
> > + if (lflow_ref) {
> > + /* lflow referencing is only supported if 'od' is not NULL. */
> > + ovs_assert(od);
> > +
> > + struct lflow_ref_node *lrn =
> > + lflow_ref_node_find(&lflow_ref->lflow_ref_nodes, lflow,
hash);
> > + if (!lrn) {
> > + lrn = xzalloc(sizeof *lrn);
> > + lrn->lflow = lflow;
> > + lrn->dp_index = od->index;
> > + ovs_list_insert(&lflow_ref->lflows_ref_list,
> > + &lrn->lflow_list_node);
> > + inc_dp_refcnt(&lflow->dp_refcnts_map, lrn->dp_index);
> > + ovs_list_insert(&lflow->referenced_by,
&lrn->ref_list_node);
> > +
> > + hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node,
hash);
> > + }
> > +
> > + lrn->linked = true;
> > + }
> > +
> > + lflow_hash_unlock(hash_lock);
> > +
> > +}
> > +
>
> This part is not thread safe.
>
> If two threads try to add logical flows that have different hashes and
> lflow_ref is not NULL we're going to have a race condition when
> inserting to the &lflow_ref->lflow_ref_nodes hash map because the two
> threads will take different locks.
>
I think it is safe because a lflow_ref is always associated with an object,
e.g. port, datapath, lb, etc., and lflow generation for a single such
object is never executed in parallel, which is how the parallel lflow build
is designed.
Does it make sense?
Thanks,
Han
> That might corrupt the map.
>
> I guess, if we don't want to cause more performance degradation we
> should maintain as many lflow_ref instances as we do hash_locks, i.e.,
> LFLOW_HASH_LOCK_MASK + 1. Will that even be possible?
>
> Wdyt?
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
