On 1/25/24 16:40, Numan Siddique wrote: > On Thu, Jan 25, 2024 at 4:21 AM Dumitru Ceara <dce...@redhat.com> wrote: >> >> On 1/25/24 06:44, Han Zhou wrote: >>> On Wed, Jan 24, 2024 at 8:39 PM Numan Siddique <num...@ovn.org> wrote: >>>> >>>> On Wed, Jan 24, 2024 at 10:53 PM Han Zhou <hz...@ovn.org> wrote: >>>>> >>>>> 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. >>>>> >>>> >>>> I'll update the next version with the proper documentation. >>>> >>>> @Han Regarding your comment that we may have the requirement in the >>>> future to add a logical flow to >>>> 2 or more lflow_ref's, I doubt if we would have such a requirement >>>> or a scenario. >>>> >>>> Because calling lflow_table_add_lflow(,..., lflow_ref1, lflow_ref2) >>>> is same as calling lflow_table_add_lflow twice >>>> >>>> i.e >>>> lflow_table_add_lflow(,..., lflow_ref1) >>>> lflow_table_add_lflow(,..., lflow_ref2) >>>> >>>> In the second call, since the ovn_lflow already exists in the lflow >>>> table, it will be just referenced in the lflow_ref2. >>>> It would be a problem for sure if these lflow_refs (ref1 and ref2) >>>> belong to different objects. >>>> >>> >>> The scenario you mentioned here is more like duplicated lflows generated >>> when processing different objects, which has different lflow_refs, which is >>> not the scenario I was talking about. >>> I am thinking about the case when a lflow is generated, there are multiple >>> inputs. For example (extremely simplified just to express the idea), when >>> generating lflows for a LB object, we also consult other objects such as >>> datapath. Now we implemented I-P for LB, so for the LB object, for each >>> lflow generated we will call lflow_table_add_lflow(..., lb->lflow_ref) to >>> maintain the reference from the LB object to the lflow. However, if in the >>> future we want to do I-P for datapaths, which means when a datapath is >>> deleted or updated we need to quickly find the lflows that is linked to the >>> datapath, we need to maintain the reference from the datapath to the same >>> lflow that is generated by the same call lflow_table_add_lflow(..., >>> lb->lflow_ref, datapath->lflow_ref). Or, do you suggest even in this case >>> we just call the function twice, with the second call's only purpose being >>> to add the reference to the secondary input? Hmm, it would actually work as >>> if we are trying to add a redundant lflow, although wasting some cycles for >>> the ovn_lflow_find(). Of course the locking issue would arise in this case. >>> Anyway, I don't have a realistic example to prove this is a requirement of >>> I-P for solving real production scale/performance issues. In the example I >>> provided, datapath I-P seems to be a non-goal as far as I can tell for now. >>> So I am not too worried. >>> >>>> However I do see a scenario where lflow_table_add_lflow() could be >>>> called with a different lflow_ref object. >>>> >>>> Few scenarios I could think of >>>> >>>> 1. While generating logical flows for a logical switch port (of type >>>> router) 'P1', there is a possibility (most likely a mistake from >>>> a contributor) may call something like >>>> >>>> lflow_table_add_lflow(...., P1->peer->lflow_ref) >>>> >>>> Even in this case, we don't generate logical router port flows in >>>> parallel with logical switch ports and >>>> hence P1->peer->lflow_ref may not be modified by multiple threads. >>>> But it's a possibility. And I think Dumitru >>>> is perhaps concerned about such scenarios. >>>> >>> In this case it does seem to be a problem, because it is possible that >>> thread1 is processing P1 while thread2 is processing P1->peer, and now both >>> threads can access P1->peer->lflow_ref. We should make it clear that when >>> processing an object, we only add lflow_ref for that object. >>> >>>> 2. While generating load balancer flows we may also generate logical >>>> flows for the routers and store them in the od->lflow_ref (If we >>>> happen to support I-P for router ports) >>>> i.e HMAP_FOR_EACH_PARALLEL (lb, lb_maps) { >>>> generate_lflows_for_lb(lb, lb->lflow_ref) >>>> FOR_EACH_ROUTER_OF_LB(lr, lb) { >>>> generate_lb_lflows_for_router(lr, lr->lflow_ref) >>>> } >>>> } >>>> >>>> In this case, it is possible that a logical router 'lr' may be >>>> associated with multiple lbs and this may corrupt the lr->lflow_ref. >>>> >>>> >>>> We have 2 choices (after this patch series is accepted :)) >>>> a. Don't do anything. The documentation added by this patch series >>>> (which I'll update in the next version) is good enough. >>>> and whenever such scenarios arise, the contributor will add >>>> the thread_safe code or make sure that he/she will not use the >>>> lflow_ref of other objects while >>>> generating lflows. In the scenario 2 above, the contributor >>>> should not generate lflows for the routers in the lb_maps loop. >>>> Instead should generate >>>> them in the lr_datapaths loop or lr_stateful loop. >>>> >>>> b. As a follow up patch, make the lflow_table_add_lflow() >>>> thread_safe for lflow_ref so that we are covered for the future >>>> scenarios. >>>> The downside of this is that we have to maintain hash locks for >>>> each lflow_ref and reconsile the lflow_ref hmap after all the threads >>>> finish the lflow generation. >>>> It does incur some cost (both memory and cpu wise). But it >>>> will be only during recomputes. Which should not be too frequent. >>>> >>> >>> I'd prefer solution (a) with proper documentation to emphasize the >>> rule/assumption. >>> >> >> Would it make sense to also try to detect whether a lflow_ref is used >> from multiple threads? E.g., store the id of the last thread that >> accessed it and assert that it's the same with the thread currently >> accessing the lflow ref? >> >> Just to make sure we crash explicitly in case such bugs are >> inadvertently introduced? >> > > I think it would be hard to make a decision here because I-P is > handled in a single thread. > How do we distinguish between a full recompute with parallel threads > vs I-P single threading > in lflow_ref.c. >
We could distinguish between recompute and incremental updates but that would probably add a connection between lflow-mgr and the I-P engine. That's not so nice indeed. Let's add a clear comment warning people about the right way to use this. Regards, Dumitru > Numan > >> Regards, >> Dumitru >> >>> Thanks, >>> Han >>> >>>> >>>> Let me know what you think. I'm fine with both, but personally prefer >>>> (1) as I don't see such scenarios in the near future. >>>> >>>> >>>> Thanks >>>> Numan >>>> >>>> >>>> >>>> >>>>> 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 >>> >> >> _______________________________________________ >> 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