On Wed, Jan 24, 2024 at 4:23 AM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 1/24/24 06:01, Han Zhou wrote:
> > 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?
>
> It happens that it's safe in this current patch set because indeed we
> always process individual ports, datapaths, lbs, etc, in the same
> thread. However, this code (lflow_table_add_lflow()) is generic and
> there's nothing (not even a comment) that would warn developers in the
> future about the potential race if the lflow_ref is shared.
>
> I spoke to Numan offline a bit about this and I think the current plan
> is to leave it as is and add proper locking as a follow up (or in v7).
> But I think we still need a clear comment here warning users about this.
> Maybe we should add a comment where the lflow_ref structure is defined
too.
>
> What do you think?
I totally agree with you about adding comments to explain the thread safety
considerations, and make it clear that the lflow_ref should always be
associated with the object that is being processed by the thread.
With regard to any follow up change for proper locking, I am not sure what
scenario is your concern. I think if we always make sure the lflow_ref
passed in is the one owned by the object then the current locking is
sufficient. And this is the natural case.
However, I did think about a situation where there might be a potential
problem in the future when we need to maintain references for more than one
input for the same lflow. For the "main" input, which is the object that
the thread is iterating and generating lflow for, will have its lflow_ref
passed in this function, but we might also need to maintain the reference
of the lflow for a secondary input (or even third). In that case it is not
just the locking issue, but firstly we need to have a proper way to pass in
the secondary lflow_ref, which is what I had mentioned in the review
comments for v3:
https://mail.openvswitch.org/pipermail/ovs-dev/2023-December/410269.html.
(the last paragraph). Regardless of that, it is not a problem for now, and
I hope there is no need to add references for more inputs for the I-P that
matters for production use cases.
Thanks,
Han
>
> Regards,
> Dumitru
>
> >
> > 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
