On 7/6/23 12:13, Han Zhou wrote: > On Thu, Jun 29, 2023 at 10:10 PM Numan Siddique <[email protected]> wrote: >> >> On Fri, Jun 30, 2023 at 7:00 AM Han Zhou <[email protected]> wrote: >>> >>> On Thu, Jun 29, 2023 at 9:19 AM Dumitru Ceara <[email protected]> wrote: >>>> >>>> On 6/27/23 10:23, Numan Siddique wrote: >>>>> On Mon, Jun 26, 2023 at 10:34 PM Han Zhou <[email protected]> wrote: >>>>>> >>>>>> On Mon, Jun 26, 2023 at 7:25 AM Numan Siddique <[email protected]> > wrote: >>>>>>> >>>>>>> On Sun, Jun 18, 2023 at 11:48 AM Han Zhou <[email protected]> wrote: >>>>>>>> >>>>>>>> For incremental processing, it is important to maintain > relationship >>>>>>>> between the inputs and the logical flows generated. This patch >>> creates >>>>>>>> the links between ovn_port and logical flows. The same data > structure >>>>>>>> may be expanded to maintain links between logical flows and other >>> types >>>>>>>> of inputs. >>>>>>>> >>>>>>>> This patch also refactors the temp_lflow_list operations to >>>>>>>> collected_lflows with helper functions to start and end > collecting. >>> It >>>>>>>> still uses global variables just to avoid updating all the >>> lflow_add_... >>>>>>>> related code all over the northd.c file. >>>>>>>> >>>>>>>> Signed-off-by: Han Zhou <[email protected]> >>>>>>> >>>>>>> Hi Han, >>>>>>> >>>>>>> Please see a few comments below. I did review all the 3 patches > in >>> the >>>>>> series. >>>>>>> They LGTM overall. I'd like to do some more testing before > providing >>> my >>>>>> Acks. >>>>>>> >>>>>> >>>>>> Thanks for your review! >>>>>> >>>>>>> >>>>>>>> --- >>>>>>>> northd/northd.c | 271 >>> +++++++++++++++++++++++++++++++----------------- >>>>>>>> 1 file changed, 178 insertions(+), 93 deletions(-) >>>>>>>> >>>>>>>> diff --git a/northd/northd.c b/northd/northd.c >>>>>>>> index 98f528f93cfc..aa0f853ce2db 100644 >>>>>>>> --- a/northd/northd.c >>>>>>>> +++ b/northd/northd.c >>>>>>>> @@ -1457,6 +1457,19 @@ struct ovn_port_routable_addresses { >>>>>>>> size_t n_addrs; >>>>>>>> }; >>>>>>>> >>>>>>>> +/* A node that maintains link between an object (such as an >>> ovn_port) >>>>>> and >>>>>>>> + * a lflow. */ >>>>>>>> +struct lflow_ref_node { >>>>>>>> + /* This list follows different lflows referenced by the same >>>>>> object. List >>>>>>>> + * head is, for example, ovn_port->lflows. */ >>>>>>>> + struct ovs_list lflow_list_node; >>>>>>>> + /* This list follows different objects that reference the > same >>>>>> lflow. List >>>>>>>> + * head is ovn_lflow->referenced_by. */ >>>>>>>> + struct ovs_list ref_list_node; >>>>>>>> + /* The lflow. */ >>>>>>>> + struct ovn_lflow *lflow; >>>>>>>> +}; >>>>>>>> + >>>>>>>> /* A logical switch port or logical router port. >>>>>>>> * >>>>>>>> * In steady state, an ovn_port points to a northbound >>>>>> Logical_Switch_Port >>>>>>>> @@ -1548,6 +1561,28 @@ struct ovn_port { >>>>>>>> >>>>>>>> /* Temporarily used for traversing a list (or hmap) of > ports. */ >>>>>>>> bool visited; >>>>>>>> + >>>>>>>> + /* List of struct lflow_ref_node that points to the lflows >>>>>> generated by >>>>>>>> + * this ovn_port. >>>>>>>> + * >>>>>>>> + * This data is initialized and destroyed by the en_northd > node, >>>>>> but >>>>>>>> + * populated and used only by the en_lflow node. Ideally > this >>> data >>>>>> should >>>>>>>> + * be maintained as part of en_lflow's data (struct >>> lflow_data): a >>>>>> hash >>>>>>>> + * index from ovn_port key to lflows. However, it would be > less >>>>>> efficient >>>>>>>> + * and more complex: >>>>>>>> + * >>>>>>>> + * 1. It would require an extra search (using the index) to > find >>>>>> the >>>>>>>> + * lflows. >>>>>>>> + * >>>>>>>> + * 2. Building the index needs to be thread-safe, using > either a >>>>>> global >>>>>>>> + * lock which is obviously less efficient, or hash-based > lock >>>>>> array which >>>>>>>> + * is more complex. >>>>>>>> + * >>>>>>>> + * Adding the list here is more straightforward. The > drawback is >>>>>> that we >>>>>>>> + * need to keep in mind that this data belongs to en_lflow > node, >>>>>> so never >>>>>>>> + * access it from any other nodes. >>>>>>>> + */ >>>>>>>> + struct ovs_list lflows; >>>>>>>> }; >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> static bool lsp_can_be_inc_processed(const struct >>>>>> nbrec_logical_switch_port *); >>>>>>>> @@ -1635,6 +1670,8 @@ ovn_port_create(struct hmap *ports, const > char >>>>>> *key, >>>>>>>> ovn_port_set_nb(op, nbsp, nbrp); >>>>>>>> op->l3dgw_port = op->cr_port = NULL; >>>>>>>> hmap_insert(ports, &op->key_node, hash_string(op->key, 0)); >>>>>>>> + >>>>>>>> + ovs_list_init(&op->lflows); >>>>>>>> return op; >>>>>>>> } >>>>>>>> >>>>>>>> @@ -1665,6 +1702,13 @@ ovn_port_destroy_orphan(struct ovn_port > *port) >>>>>>>> destroy_lport_addresses(&port->proxy_arp_addrs); >>>>>>>> free(port->json_key); >>>>>>>> free(port->key); >>>>>>>> + >>>>>>>> + struct lflow_ref_node *l; >>>>>>>> + LIST_FOR_EACH_SAFE (l, lflow_list_node, &port->lflows) { >>>>>>>> + ovs_list_remove(&l->lflow_list_node); >>>>>>>> + ovs_list_remove(&l->ref_list_node); >>>>>>>> + free(l); >>>>>>>> + } >>>>>>>> free(port); >>>>>>>> } >>>>>>>> >>>>>>>> @@ -4893,6 +4937,7 @@ static struct ovn_port * >>>>>>>> ls_port_create(struct ovsdb_idl_txn *ovnsb_txn, struct hmap >>> *ls_ports, >>>>>>>> const char *key, const struct >>> nbrec_logical_switch_port >>>>>> *nbsp, >>>>>>>> struct ovn_datapath *od, const struct >>>>>> sbrec_port_binding *sb, >>>>>>>> + struct ovs_list *lflows, >>>>>>>> const struct sbrec_mirror_table > *sbrec_mirror_table, >>>>>>>> const struct sbrec_chassis_table >>> *sbrec_chassis_table, >>>>>>>> struct ovsdb_idl_index *sbrec_chassis_by_name, >>>>>>>> @@ -4903,6 +4948,9 @@ ls_port_create(struct ovsdb_idl_txn > *ovnsb_txn, >>>>>> struct hmap *ls_ports, >>>>>>>> parse_lsp_addrs(op); >>>>>>>> op->od = od; >>>>>>>> hmap_insert(&od->ports, &op->dp_node, >>>>>> hmap_node_hash(&op->key_node)); >>>>>>>> + if (lflows) { >>>>>>>> + ovs_list_splice(&op->lflows, lflows->next, lflows); >>>>>>>> + } >>>>>>>> >>>>>>>> /* Assign explicitly requested tunnel ids first. */ >>>>>>>> if (!ovn_port_assign_requested_tnl_id(sbrec_chassis_table, > op)) >>> { >>>>>>>> @@ -5082,7 +5130,7 @@ northd_handle_ls_changes(struct > ovsdb_idl_txn >>>>>> *ovnsb_idl_txn, >>>>>>>> goto fail; >>>>>>>> } >>>>>>>> op = ls_port_create(ovnsb_idl_txn, > &nd->ls_ports, >>>>>>>> - new_nbsp->name, new_nbsp, > od, >>> NULL, >>>>>>>> + new_nbsp->name, new_nbsp, > od, >>>>>> NULL, NULL, >>>>>>>> ni->sbrec_mirror_table, >>>>>>>> ni->sbrec_chassis_table, >>>>>>>> ni->sbrec_chassis_by_name, >>>>>>>> @@ -5114,13 +5162,16 @@ northd_handle_ls_changes(struct > ovsdb_idl_txn >>>>>> *ovnsb_idl_txn, >>>>>>>> op->visited = true; >>>>>>>> continue; >>>>>>>> } >>>>>>>> + struct ovs_list lflows = >>> OVS_LIST_INITIALIZER(&lflows); >>>>>>>> + ovs_list_splice(&lflows, op->lflows.next, >>> &op->lflows); >>>>>>>> ovn_port_destroy(&nd->ls_ports, op); >>>>>>>> op = ls_port_create(ovnsb_idl_txn, > &nd->ls_ports, >>>>>>>> - new_nbsp->name, new_nbsp, > od, >>> sb, >>>>>>>> + new_nbsp->name, new_nbsp, > od, >>> sb, >>>>>> &lflows, >>>>>>>> ni->sbrec_mirror_table, >>>>>>>> ni->sbrec_chassis_table, >>>>>>>> ni->sbrec_chassis_by_name, >>>>>>>> > ni->sbrec_chassis_by_hostname); >>>>>>>> + ovs_assert(ovs_list_is_empty(&lflows)); >>>>>>>> if (!op) { >>>>>>>> goto fail; >>>>>>>> } >>>>>>>> @@ -5577,7 +5628,8 @@ ovn_igmp_group_destroy(struct hmap >>> *igmp_groups, >>>>>>>> >>>>>>>> struct ovn_lflow { >>>>>>>> struct hmap_node hmap_node; >>>>>>>> - struct ovs_list list_node; >>>>>>>> + struct ovs_list list_node; /* For temporary list of > lflows. >>>>>> Don't remove >>>>>>>> + at destroy. */ >>>>>>>> >>>>>>>> struct ovn_datapath *od; /* 'logical_datapath' in SB > schema. >>>>>> */ >>>>>>>> unsigned long *dpg_bitmap; /* Bitmap of all datapaths by > their >>>>>> 'index'.*/ >>>>>>>> @@ -5591,6 +5643,8 @@ struct ovn_lflow { >>>>>>>> size_t n_ods; /* Number of datapaths > referenced >>> by >>>>>> 'od' and >>>>>>>> * 'dpg_bitmap'. */ >>>>>>>> struct ovn_dp_group *dpg; /* Link to unique Sb datapath >>> group. >>>>>> */ >>>>>>>> + >>>>>>>> + struct ovs_list referenced_by; /* List of struct >>> lflow_ref_node. >>>>>> */ >>>>>>>> const char *where; >>>>>>>> >>>>>>>> struct uuid sb_uuid; /* SB DB row uuid, > specified by >>>>>> northd. */ >>>>>>>> @@ -5640,6 +5694,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, > struct >>>>>> ovn_datapath *od, >>>>>>>> char *stage_hint, const char *where) >>>>>>>> { >>>>>>>> ovs_list_init(&lflow->list_node); >>>>>>>> + ovs_list_init(&lflow->referenced_by); >>>>>>>> lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len); >>>>>>>> lflow->od = od; >>>>>>>> lflow->stage = stage; >>>>>>>> @@ -5767,20 +5822,30 @@ ovn_dp_group_add_with_reference(struct >>>>>> ovn_lflow *lflow_ref, >>>>>>>> } >>>>>>>> } >>>>>>>> >>>>>>>> +/* This global variable collects the lflows generated by >>>>>> do_ovn_lflow_add(). >>>>>>>> + * start_collecting_lflows() will enable the lflow collection > and >>> the >>>>>> calls to >>>>>>>> + * do_ovn_lflow_add (or the macros ovn_lflow_add_...) will add >>>>>> generated lflows >>>>>>>> + * to the list. end_collecting_lflows() will disable it. */ >>>>>>>> +static thread_local struct ovs_list collected_lflows; >>>>>>>> +static thread_local bool collecting_lflows = false; >>>>>>>> + >>>>>>>> +static void >>>>>>>> +start_collecting_lflows(void) >>>>>>>> +{ >>>>>>>> + ovs_assert(!collecting_lflows); >>>>>>>> + ovs_list_init(&collected_lflows); >>>>>>>> + collecting_lflows = true; >>>>>>>> +} >>>>>>>> + >>>>>>>> +static void >>>>>>>> +end_collecting_lflows(void) >>>>>>>> +{ >>>>>>>> + ovs_assert(collecting_lflows); >>>>>>>> + collecting_lflows = false; >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> I think we can avoid these functions and the thread local variable >>>>>>> "collected_lflows". >>>>>>> >>>>>>> I'd suggest the below >>>>>>> >>>>>>> ---------------------------- >>>>>>> >>>>>>> static void >>>>>>> do_ovn_lflow_add(struct hmap *lflow_map, const struct ovn_datapath >>> *od, >>>>>>> const unsigned long *dp_bitmap, size_t > dp_bitmap_len, >>>>>>> uint32_t hash, enum ovn_stage stage, uint16_t >>> priority, >>>>>>> const char *match, const char *actions, const > char >>>>>> *io_port, >>>>>>> struct ovs_list *lflow_ref_list, >>>>>>> const struct ovsdb_idl_row *stage_hint, >>>>>>> const char *where, const char *ctrl_meter) >>>>>>> OVS_REQUIRES(fake_hash_mutex) >>>>>>> { >>>>>>> ... >>>>>>> ... >>>>>>> /* At the end. */ >>>>>>> if (lflow_ref_list) { >>>>>>> struct lflow_ref_node *lfrn = xmalloc(sizeof *lfrn); >>>>>>> lfrn->lflow = lflow; >>>>>>> ovs_list_insert(lflow_ref_list, &lfrn->lflow_list_node); >>>>>>> ovs_list_insert(&lflow->referenced_by, > &lfrn->ref_list_node); >>>>>>> } >>>>>>> } >>>>>>> >>>>>>> >>>>>>> #define ovn_lflow_add_with_lport_and_hint(LFLOW_MAP, OD, STAGE, >>> PRIORITY, >>>>>> \ >>>>>>> MATCH, ACTIONS, >>> IN_OUT_PORT, \ >>>>>>> LFLOW_REF_LIST, > STAGE_HINT) >>> \ >>>>>>> ovn_lflow_add_at(LFLOW_MAP, OD, NULL, 0, STAGE, PRIORITY, > MATCH, >>>>>> ACTIONS, \ >>>>>>> IN_OUT_PORT, LFLOW_REF_LIST, NULL, > STAGE_HINT, \ >>>>>>> OVS_SOURCE_LOCATOR) >>>>>>> >>>>>>> And pass &op->lflows in ovn_lflow_add_with_lport_and_hint() >>>>>>> >>>>>>> ----------------------------- >>>>>>> >>>>>>> What do you think ? Definitely this would result in a bit more > work >>>>>>> as it would require a lot of (tedious) changes. >>>>>>> I think this is a better approach. >>>>>>> >>>>>> Firstly, I think it is not good to use "lflow_ref_list" directly > in the >>>>>> parameter, because there can be more than one object contributing > to >>> the >>>>>> lflow generation. When we implement I-P for more inputs, a single >>> lflow may >>>>>> be referenced by multiple objects. We can't pass multiple >>> lflow_ref_list to >>>>>> the function either, because the number of such lists is unknown. > For >>>>>> example, a lflow may be generated as a result of a LSP, a DP and a > LB >>>>>> backend. If we want to implement I-P for LSP, DP and LB backend, we >>> need to >>>>>> track the reference for all of them. So the current idea is just to >>> collect >>>>>> a list of lflows generated by a higher level function, such as the >>>>>> build_lswitch_and_lrouter_iterate_by_lsp. When implementing I-P for >>> more >>>>>> than one object of the same lflow, this needs to be more > fine-grained. >>>> >>>> I'm still reviewing this patch but my first impression was that we can >>>> probably try to use 'struct objdep_mgr' (defined in lib/objdep.h) to >>>> model all these (potentially many-to-many) relationships. We do > similar >>>> things in ovn-controller. >>>> >>> My first impression was similar, but later I figured out it is different >>> (and more complex than ovn-controller in general) and we will need to > make >>> changes to objdep_mgr. However, until more input I-P is implemented I am >>> not sure if I will be able to get a general enough abstraction in >>> objdep_mgr that can handle all the use cases. On the other hand, the >>> references between LSP and related lflows are quite simple, so I went > ahead >>> and implemented the lists (embeded into ovn_port and lflow without extra >>> hash tables), with minimum effort, to make it work at least for this >>> scenario and should also work for similar scenarios. I think we can >>> refactor the reference implementation any time but it may be better to > get >>> more I-P implemented, during the process we can always revisit and see > if >>> objdep_mgr can be extended or another common dependency/reference > library >>> for ovn-northd can be created, etc. What do you think? >> >> As part of my refactor and northd I-P for load balancers and data >> paths, I'll try to see >> if objdep_mgr can be used. For now with just lport I-P, I suppose it's > fine. >> >> Acked-by: Numan Siddique <[email protected]> >> > Haven't heard back from Dumitru for a while, so I assume you are fine with > this (but please share any time if you have more comments). > Thank you both, Numan and Dumitru. I applied this to main. >
Sorry, Han, I'm quite slow in reviewing. I'm OK with you applying the patches, I wouldn't want to block development. If I find anything later on I'll post follow up fixes or reports. I still think it's best to reuse objdep_mgr and I think Numan is working on changing to that in his series so we're fine. Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
