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
