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