On 1/30/24 22:21, num...@ovn.org wrote: > From: Numan Siddique <num...@ovn.org> > > ovn_lflow_add() and other related functions/macros are now moved > into a separate module - lflow-mgr.c. This module maintains a > table 'struct lflow_table' for the logical flows. lflow table > maintains a hmap to store the logical flows. > > It also maintains the logical switch and router dp groups. > > Previous commits which added lflow incremental processing for > the VIF logical ports, stored the references to > the logical ports' lflows using 'struct lflow_ref_list'. This > struct is renamed to 'struct lflow_ref' and is part of lflow-mgr.c. > It is modified a bit to store the resource to lflow references. > > Example usage of 'struct lflow_ref'. > > 'struct ovn_port' maintains 2 instances of lflow_ref. i,e > > struct ovn_port { > ... > ... > struct lflow_ref *lflow_ref; > struct lflow_ref *stateful_lflow_ref; > }; > > All the logical flows generated by > build_lswitch_and_lrouter_iterate_by_lsp() uses the ovn_port->lflow_ref. > > All the logical flows generated by build_lsp_lflows_for_lbnats() > uses the ovn_port->stateful_lflow_ref. > > When handling the ovn_port changes incrementally, the lflows referenced > in 'struct ovn_port' are cleared and regenerated and synced to the > SB logical flows. > > eg. > > lflow_ref_clear_lflows(op->lflow_ref); > build_lswitch_and_lrouter_iterate_by_lsp(op, ...); > lflow_ref_sync_lflows_to_sb(op->lflow_ref, ...); > > This patch does few more changes: > - Logical flows are now hashed without the logical > datapaths. If a logical flow is referenced by just one > datapath, we don't rehash it. > > - The synthetic 'hash' column of sbrec_logical_flow now > doesn't use the logical datapath. This means that > when ovn-northd is updated/upgraded and has this commit, > all the logical flows with 'logical_datapath' column > set will get deleted and re-added causing some disruptions. > > - With the commit [1] which added I-P support for logical > port changes, multiple logical flows with same match 'M' > and actions 'A' are generated and stored without the > dp groups, which was not the case prior to > that patch. > One example to generate these lflows is: > ovn-nbctl lsp-set-addresses sw0p1 "MAC1 IP1" > ovn-nbctl lsp-set-addresses sw1p1 "MAC1 IP1" > ovn-nbctl lsp-set-addresses sw2p1 "MAC1 IP1" > > Now with this patch we go back to the earlier way. i.e > one logical flow with logical_dp_groups set. > > - With this patch any updates to a logical port which > doesn't result in new logical flows will not result in > deletion and addition of same logical flows. > Eg. > ovn-nbctl set logical_switch_port sw0p1 external_ids:foo=bar > will be a no-op to the SB logical flow table. > > [1] - 8bbd678("northd: Incremental processing of VIF additions in 'lflow' > node.") > > Signed-off-by: Numan Siddique <num...@ovn.org> > ---
[...] > + > +/* Logical flow sync using 'struct lflow_ref' > + * ========================================== > + * The 'struct lflow_ref' represents a collection of (or references to) > + * logical flows (struct ovn_lflow) which belong to a logical entity 'E'. > + * This entity 'E' is external to lflow manager (see northd.h and northd.c) > + * Eg. logical datapath (struct ovn_datapath), logical switch and router > ports > + * (struct ovn_port), load balancer (struct lb_datapath) etc. > + * > + * General guidelines on using 'struct lflow_ref'. > + * - For an entity 'E', create an instance of lflow_ref > + * E->lflow_ref = lflow_ref_create(); > + * > + * - For each logical flow L(M, A) generated for the entity 'E' > + * pass E->lflow_ref when adding L(M, A) to the lflow table. > + * Eg. lflow_table_add_lflow(lflow_table, od_of_E, M, A, .., > E->lflow_ref); > + * > + * If lflows L1, L2 and L3 are generated for 'E', then > + * E->lflow_ref stores these in its hmap. > + * i.e E->lflow_ref->lflow_ref_nodes = hmap[LRN(L1, E1), LRN(L2, E1), > + * LRN(L3, E1)] > + * > + * LRN is an instance of 'struct lflow_ref_node'. > + * 'struct lflow_ref_node' is used to store a logical lflow L(M, A) as a > + * reference in the lflow_ref. It is possible that an lflow L(M,A) can be > + * referenced by one or more lflow_ref's. For each reference, an instance of > + * this struct 'lflow_ref_node' is created. > + * > + * For example, if entity E1 generates lflows L1, L2 and L3 > + * and entity E2 generates lflows L1, L3, and L4 then > + * an instance of this struct is created for each entity. > + * For example LRN(L1, E1). > + * > + * Each logical flow's L also maintains a list of its references in the > + * ovn_lflow->referenced_by list. > + * > + * > + * > + * L1 L2 L3 L4 > + * | | (list) | | > + * (lflow_ref) v v v v > + * ---------------------------------------------------------------------- > + * | E1 (hmap) => LRN(L1,E1) => LRN(L2, E1) => LRN(L3, E1) | | > + * | | | | | > + * | v v v | > + * | E2 (hmap) => LRN(L1,E2) ================> LRN(L3, E2) => LRN(L4, E2) | > + * ---------------------------------------------------------------------- > + * > + * > + * Life cycle of 'struct lflow_ref_node' > + * ===================================== > + * For a given logical flow L1 and entity E1's lflow_ref, > + * 1. LRN(L1, E1) is created in lflow_table_add_lflow() and its 'linked' > flag > + * is set to true. > + * 2. LRN(L1, E1) is stored in the hmap - E1->lflow_ref->lflow_ref_nodes. > + * 3. LRN(L1, E1) is also stored in the linked list L1->referenced_by. > + * 4. LRN(L1, E1)->linked is set to false when the client calls > + * lflow_ref_unlink_lflows(E1->lflow_ref). > + * 5. LRN(L1, E1)->linked is set to true again when the client calls > + * lflow_table_add_lflow(L1, ..., E1->lflow_ref) and LRN(L1, E1) > + * is already present. > + * 6. LRN(L1, E1) is destroyed if LRN(L1, E1)->linked is false > + * when the client calls lflow_ref_sync_lflows(). > + * 7. LRN(L1, E1) is also destroyed in lflow_ref_clear(E1->lflow_ref). > + * > + * > + * Incremental lflow generation for a logical entity > + * ================================================= > + * Lets take the above example again. > + * > + * > + * L1 L2 L3 L4 > + * | | (list) | | > + * (lflow_ref) v v v v > + * ---------------------------------------------------------------------- > + * | E1 (hmap) => LRN(L1,E1) => LRN(L2, E1) => LRN(L3, E1) | | > + * | | | | | > + * | v v v | > + * | E2 (hmap) => LRN(L1,E2) ================> LRN(L3, E2) => LRN(L4, E2) | > + * ---------------------------------------------------------------------- > + * > + * > + * L1 is referenced by E1 and E2 > + * L2 is referenced by just E1 > + * L3 is referenced by E1 and E2 > + * L4 is referenced by just E2 > + * > + * L1->dpg_bitmap = [E1->od->index, E2->od->index] > + * L2->dpg_bitmap = [E1->od->index] > + * L3->dpg_bitmap = [E1->od->index, E2->od->index] > + * L4->dpg_bitmap = [E2->od->index] > + * > + * > + * When 'E' gets updated, > + * 1. the client should first call > + * lflow_ref_unlink_lflows(E1->lflow_ref); > + * > + * This function sets the 'linked' flag to false and clears the dp > bitmap > + * of linked lflows. > + * > + * LRN(L1,E1)->linked = false; > + * LRN(L2,E1)->linked = false; > + * LRN(L3,E1)->linked = false; > + * > + * bitmap status of all lflows in the lflows table > + * ----------------------------------------------- > + * L1->dpg_bitmap = [E2->od->index] > + * L2->dpg_bitmap = [] > + * L3->dpg_bitmap = [E2->od->index] > + * L4->dpg_bitmap = [E2->od->index] > + * > + * 2. In step (2), client should generate the logical flows again for > 'E1'. > + * Lets say it calls: > + * lflow_table_add_lflow(lflow_table, L3, E1->lflow_ref) > + * lflow_table_add_lflow(lflow_table, L5, E1->lflow_ref) > + * > + * So, E1 generates the flows L3 and L5 and discards L1 and L2. > + * > + * Below is the state of LRNs of E1 > + * LRN(L1,E1)->linked = false; > + * LRN(L2,E1)->linked = false; > + * LRN(L3,E1)->linked = true; > + * LRN(L5,E1)->linked = true; > + * > + * bitmap status of all lflows in the lflow table after end of step (2) > + * -------------------------------------------------------------------- > + * L1->dpg_bitmap = [E2->od->index] > + * L2->dpg_bitmap = [] > + * L3->dpg_bitmap = [E1->od->index, E2->od->index] > + * L4->dpg_bitmap = [E2->od->index] > + * L5->dpg_bitmap = [E1->od->index] > + * > + * 3. In step (3), client should sync the E1's lflows by calling > + * lflow_ref_sync_lflows(E1->lflow_ref,....); > + * > + * Below is how the logical flows in SB DB gets updated: > + * lflow L1: > + * SB:L1->logical_dp_group = NULL; > + * SB:L1->logical_datapath = E2->od; > + * > + * lflow L2: L2 is deleted since no datapath is using it. > + * > + * lflow L3: No changes > + * > + * lflow L5: New row is created for this. > + * > + * After step (3) > + * > + * L1 L5 L3 L4 > + * | | (list) | | > + * (lflow_ref) v v v v > + * ---------------------------------------------------------------------- > + * | E1 (hmap) ===============> LRN(L2, E1) => LRN(L3, E1) | | > + * | | | | | > + * | v v v | > + * | E2 (hmap) => LRN(L1,E2) ================> LRN(L3, E2) => LRN(L4, E2) | > + * ---------------------------------------------------------------------- > + * > + * Thread safety in lflow_ref > + * ========================== > + * The function lflow_table_add_lflow() is not thread safe for lflow_ref. > + * Client should ensure that same instance of lflow_ref's are not used > + * by multiple threads when calling lflow_table_add_lflow(). > + * > + * One way to ensure thread safety is to maintain array of hash locks > + * in each lflow_ref just like how we have static variable lflow_hash_locks > + * of type ovs_mutex. This would mean that client has to reconsile the s/reconsile/reconcile > + * lflow_ref hmap lflow_ref_nodes (by calling hmap_expand()) after the > + * lflow generation is complete. (See lflow_table_expand()). > + * > + * Presently the client of lflow manager (northd.c) doesn't call > + * lflow_table_add_lflow() in multiple threads for the same lflow_ref. > + * But it may change in the future and we may need to add the thread > + * safety support. > + * > + * Until then care should be taken by the contributors to avoid this > + * scenario. > + */ Thanks for documenting this! > > +extern int parallelization_state; > +enum { > + STATE_NULL, /* parallelization is off */ > + STATE_INIT_HASH_SIZES, /* parallelization is on; hashes sizing needed > */ > + STATE_USE_PARALLELIZATION /* parallelization is on */ > +}; > + > +extern thread_local size_t thread_lflow_counter; > + I'm not a fan of this TBH. We define and initialize this counter in northd.c but we increment it here. I think this is a sign of the fact that we didn't split out the lflow managment properly. I guess the logic that runs lflow build in parallel (currently in northd.c) should be more abstract and should be moved to lflow-mgr.c. I think that's a large change too though so not really possible to do in this release cycle but can we please add a TODO item for it? With these small comments addressed: Acked-by: Dumitru Ceara <dce...@redhat.com> Thanks, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev