On Wed, Sep 24, 2025 at 8:08 AM Lucas Vargas Dias via dev
<[email protected]> wrote:
>
> Change the logic to save sbflow uuid and just update if
> the lflow is reused. Otherwise, it's removed.
> Also, reduce sbflow searching with uuidset instead of
> searching through all lflow table.
> Add lflow states:
> LFLOW_STALE - Lflow is not relevant
> LFLOW_TO_SYNC - Lflow needs to be synced with SB DB
> LFLOW_SYNCED - Lflow is synced with SB SB
>
> It generates the following results in a scenario with:
> - LSPs: 56548
> - LRPs: 27304
> - LRs: 7922
> - LSs: 28602
>
> Without the commit:
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lr-add lr9-1
> Time spent on processing nb_cfg 275438:
> ovn-northd delay before processing: 16069ms
> ovn-northd completion: 32828ms
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb ls-add bar9-1
> Time spent on processing nb_cfg 275439:
> ovn-northd delay before processing: 15019ms
> ovn-northd completion: 33207ms
>
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lrp-add lr9-1
> rp9-1-bar 00:01:ff:22:00:01 192.168.10.1/24 2801:80:3eaf:4401::1/64
> Time spent on processing nb_cfg 275440:
> ovn-northd delay before processing: 14784ms
> ovn-northd completion: 33577ms
>
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lsp-add bar9-1
> bar-rp9-1 -- set Logical_Switch_Port bar-rp9-1 type=router
> options:router-port=rp9-1-bar -- lsp-set-addresses bar-rp9-1 router
> Time spent on processing nb_cfg 275441:
> ovn-northd delay before processing: 14598ms
> ovn-northd completion: 31942ms
>
> With the commit:
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lr-add lr9-1
> Time spent on processing nb_cfg 275401:
> ovn-northd delay before processing: 12602ms
> ovn-northd completion: 26103ms
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb ls-add bar9-1
> Time spent on processing nb_cfg 275402:
> ovn-northd delay before processing: 12639ms
> ovn-northd completion: 26759ms
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lrp-add lr9-1
> rp9-1-bar 00:01:ff:22:00:01 192.168.10.1/24 2801:80:3eaf:4401::1/64
> Time spent on processing nb_cfg 275403:
> ovn-northd delay before processing: 11874ms
> ovn-northd completion: 29733ms
> ovn-nbctl --no-leader-only --print-wait-time --wait=sb lsp-add bar9-1
> bar-rp9-1 -- set Logical_Switch_Port bar-rp9-1 type=router
> options:router-port=rp9-1-bar -- lsp-set-addresses bar-rp9-1 router
> Time spent on processing nb_cfg 275404:
> ovn-northd delay before processing: 4058ms
> ovn-northd completion: 17323ms
>
> Signed-off-by: Lucas Vargas Dias <[email protected]>
Thanks for the improvements.
I've one comment below.
Numan
> ---
> northd/en-lflow.c | 2 +-
> northd/lflow-mgr.c | 108 +++++++++++++++++++++++++--------------------
> northd/lflow-mgr.h | 10 ++++-
> northd/northd.c | 8 ++--
> 4 files changed, 74 insertions(+), 54 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 50570b611..13c5e3119 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -122,7 +122,7 @@ en_lflow_run(struct engine_node *node, void *data)
> stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
> struct lflow_data *lflow_data = data;
> - lflow_table_clear(lflow_data->lflow_table);
> + lflow_table_clear(lflow_data->lflow_table, false);
> lflow_reset_northd_refs(&lflow_input);
> lflow_ref_clear(lflow_input.igmp_lflow_ref);
>
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index 6a66a9718..95986f9c3 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -26,6 +26,7 @@
> #include "lflow-mgr.h"
> #include "lib/ovn-parallel-hmap.h"
> #include "lib/ovn-util.h"
> +#include "lib/uuidset.h"
>
> VLOG_DEFINE_THIS_MODULE(lflow_mgr);
>
> @@ -37,7 +38,8 @@ static void ovn_lflow_init(struct ovn_lflow *, struct
> ovn_datapath *od,
> uint16_t priority, char *match,
> char *actions, char *io_port,
> char *ctrl_meter, char *stage_hint,
> - const char *where, const char *flow_desc);
> + const char *where, const char *flow_desc,
> + struct uuid sbuuid);
> static struct ovn_lflow *ovn_lflow_find(const struct hmap *lflows,
> enum ovn_stage stage,
> uint16_t priority, const char *match,
> @@ -147,6 +149,13 @@ static struct ovs_mutex
> lflow_hash_locks[LFLOW_HASH_LOCK_MASK + 1];
> */
> extern struct ovs_mutex fake_hash_mutex;
>
> +
> +enum ovn_lflow_state {
> + LFLOW_STALE,
> + LFLOW_TO_SYNC,
> + LFLOW_SYNCED,
> +};
> +
> /* Represents a logical ovn flow (lflow).
> *
> * A logical flow with match 'M' and actions 'A' - L(M, A) is created
> @@ -181,14 +190,7 @@ struct ovn_lflow {
> struct hmap dp_refcnts_map; /* Maintains the number of times this
> ovn_lflow
> * is referenced by a given datapath.
> * Contains 'struct dp_refcnt' in the map. */
> -};
> -
> -/* Logical flow table. */
> -struct lflow_table {
> - struct hmap entries; /* hmap of lflows. */
> - struct hmap ls_dp_groups; /* hmap of logical switch dp groups. */
> - struct hmap lr_dp_groups; /* hmap of logical router dp groups. */
> - ssize_t max_seen_lflow_size;
> + enum ovn_lflow_state sync_state;
> };
>
> struct lflow_table *
> @@ -210,10 +212,15 @@ lflow_table_init(struct lflow_table *lflow_table)
> }
>
> void
> -lflow_table_clear(struct lflow_table *lflow_table)
> +lflow_table_clear(struct lflow_table *lflow_table, bool destroy_all)
> {
> struct ovn_lflow *lflow;
> HMAP_FOR_EACH_SAFE (lflow, hmap_node, &lflow_table->entries) {
> + ovs_assert(lflow->sync_state == LFLOW_SYNCED);
Lets say a logical port change (or any change) was handled incrementally and
a new logical flow was added to the lflow_table. Its state when added
would be LFLOW_TO_SYNC.
And for some reason the function lflow_ref_sync_lflows__() returns
false [1] . The state of this new flow
will not be LFLOW_SYNCED. The engine will fall back to recompute and
we would assert here.
I think this scenario should be addressed. Please see the comments in [1].
[1] - https://github.com/ovn-org/ovn/blob/main/northd/lflow-mgr.c#L1135
Other than that the patch LGTM.
Thanks
Numan
> + if (!destroy_all) {
> + lflow->sync_state = LFLOW_STALE;
> + continue;
> + }
> ovn_lflow_destroy(lflow_table, lflow);
> }
>
> @@ -224,7 +231,7 @@ lflow_table_clear(struct lflow_table *lflow_table)
> void
> lflow_table_destroy(struct lflow_table *lflow_table)
> {
> - lflow_table_clear(lflow_table);
> + lflow_table_clear(lflow_table, true);
> hmap_destroy(&lflow_table->entries);
> ovn_dp_groups_destroy(&lflow_table->ls_dp_groups);
> ovn_dp_groups_destroy(&lflow_table->lr_dp_groups);
> @@ -257,16 +264,42 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table,
> const struct sbrec_logical_flow_table *sb_flow_table,
> const struct sbrec_logical_dp_group_table
> *dpgrp_table)
> {
> + struct uuidset sb_uuid_set = UUIDSET_INITIALIZER(&sb_uuid_set);
> struct hmap lflows_temp = HMAP_INITIALIZER(&lflows_temp);
> struct hmap *lflows = &lflow_table->entries;
> struct ovn_lflow *lflow;
> + const struct sbrec_logical_flow *sbflow;
>
> fast_hmap_size_for(&lflows_temp,
> lflow_table->max_seen_lflow_size);
>
> + HMAP_FOR_EACH_SAFE (lflow, hmap_node, lflows) {
> + if (lflow->sync_state == LFLOW_STALE) {
> + ovn_lflow_destroy(lflow_table, lflow);
> + continue;
> + }
> + sbflow = NULL;
> + if (!uuid_is_zero(&lflow->sb_uuid)) {
> + sbflow = sbrec_logical_flow_table_get_for_uuid(sb_flow_table,
> + &lflow->sb_uuid);
> + }
> + sync_lflow_to_sb(lflow, ovnsb_txn, lflow_table, ls_datapaths,
> + lr_datapaths, ovn_internal_version_changed,
> + sbflow, dpgrp_table);
> + uuidset_insert(&sb_uuid_set, &lflow->sb_uuid);
> + hmap_remove(lflows, &lflow->hmap_node);
> + hmap_insert(&lflows_temp, &lflow->hmap_node,
> + hmap_node_hash(&lflow->hmap_node));
> + }
> /* Push changes to the Logical_Flow table to database. */
> - const struct sbrec_logical_flow *sbflow;
> SBREC_LOGICAL_FLOW_TABLE_FOR_EACH_SAFE (sbflow, sb_flow_table) {
> + struct uuidset_node *node = uuidset_find(&sb_uuid_set,
> + &sbflow->header_.uuid);
> + if (!node) {
> + sbrec_logical_flow_delete(sbflow);
> + continue;
> + }
> + uuidset_delete(&sb_uuid_set, node);
> struct sbrec_logical_dp_group *dp_group = sbflow->logical_dp_group;
> struct ovn_datapath *logical_datapath_od = NULL;
> size_t i;
> @@ -297,38 +330,8 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table,
> sbrec_logical_flow_delete(sbflow);
> continue;
> }
> -
> - enum ovn_pipeline pipeline
> - = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT;
> -
> - lflow = ovn_lflow_find(
> - lflows,
> - ovn_stage_build(ovn_datapath_get_type(logical_datapath_od),
> - pipeline, sbflow->table_id),
> - sbflow->priority, sbflow->match, sbflow->actions,
> - sbflow->controller_meter, sbflow->hash);
> - if (lflow) {
> - sync_lflow_to_sb(lflow, ovnsb_txn, lflow_table, ls_datapaths,
> - lr_datapaths, ovn_internal_version_changed,
> - sbflow, dpgrp_table);
> -
> - hmap_remove(lflows, &lflow->hmap_node);
> - hmap_insert(&lflows_temp, &lflow->hmap_node,
> - hmap_node_hash(&lflow->hmap_node));
> - } else {
> - sbrec_logical_flow_delete(sbflow);
> - }
> - }
> -
> - HMAP_FOR_EACH_SAFE (lflow, hmap_node, lflows) {
> - sync_lflow_to_sb(lflow, ovnsb_txn, lflow_table, ls_datapaths,
> - lr_datapaths, ovn_internal_version_changed,
> - NULL, dpgrp_table);
> -
> - hmap_remove(lflows, &lflow->hmap_node);
> - hmap_insert(&lflows_temp, &lflow->hmap_node,
> - hmap_node_hash(&lflow->hmap_node));
> }
> + uuidset_destroy(&sb_uuid_set);
> hmap_swap(lflows, &lflows_temp);
> hmap_destroy(&lflows_temp);
> }
> @@ -847,7 +850,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
> size_t dp_bitmap_len, enum ovn_stage stage, uint16_t priority,
> char *match, char *actions, char *io_port, char *ctrl_meter,
> char *stage_hint, const char *where,
> - const char *flow_desc)
> + const char *flow_desc, struct uuid sbuuid)
> {
> lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
> lflow->od = od;
> @@ -861,7 +864,8 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
> lflow->flow_desc = flow_desc;
> lflow->dpg = NULL;
> lflow->where = where;
> - lflow->sb_uuid = UUID_ZERO;
> + lflow->sb_uuid = sbuuid;
> + lflow->sync_state = LFLOW_TO_SYNC;
> hmap_init(&lflow->dp_refcnts_map);
> ovs_list_init(&lflow->referenced_by);
> }
> @@ -957,13 +961,18 @@ do_ovn_lflow_add(struct lflow_table *lflow_table,
> size_t dp_bitmap_len,
> {
> struct ovn_lflow *old_lflow;
> struct ovn_lflow *lflow;
> + struct uuid sbuuid = UUID_ZERO;
>
> ovs_assert(dp_bitmap_len);
>
> old_lflow = ovn_lflow_find(&lflow_table->entries, stage,
> priority, match, actions, ctrl_meter, hash);
> if (old_lflow) {
> - return old_lflow;
> + if (old_lflow->sync_state != LFLOW_STALE) {
> + return old_lflow;
> + }
> + sbuuid = old_lflow->sb_uuid;
> + ovn_lflow_destroy(lflow_table, old_lflow);
> }
>
> lflow = xzalloc(sizeof *lflow);
> @@ -975,14 +984,16 @@ do_ovn_lflow_add(struct lflow_table *lflow_table,
> size_t dp_bitmap_len,
> io_port ? xstrdup(io_port) : NULL,
> nullable_xstrdup(ctrl_meter),
> ovn_lflow_hint(stage_hint), where,
> - flow_desc);
> + flow_desc, sbuuid);
>
> if (parallelization_state != STATE_USE_PARALLELIZATION) {
> hmap_insert(&lflow_table->entries, &lflow->hmap_node, hash);
> } else {
> hmap_insert_fast(&lflow_table->entries, &lflow->hmap_node,
> hash);
> - thread_lflow_counter++;
> + if (uuid_is_zero(&lflow->sb_uuid)) {
> + thread_lflow_counter++;
> + }
> }
>
> return lflow;
> @@ -1169,6 +1180,7 @@ sync_lflow_to_sb(struct ovn_lflow *lflow,
> ovn_dp_group_release(dp_groups, pre_sync_dpg);
> }
>
> + lflow->sync_state = LFLOW_SYNCED;
> return true;
> }
>
> diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
> index 1521270d6..91708c8a9 100644
> --- a/northd/lflow-mgr.h
> +++ b/northd/lflow-mgr.h
> @@ -26,10 +26,16 @@ struct ovn_datapath;
> struct ovsdb_idl_row;
>
> /* lflow map which stores the logical flows. */
> -struct lflow_table;
> +struct lflow_table {
> + struct hmap entries; /* hmap of lflows. */
> + struct hmap ls_dp_groups; /* hmap of logical switch dp groups. */
> + struct hmap lr_dp_groups; /* hmap of logical router dp groups. */
> + ssize_t max_seen_lflow_size;
> +};
> +
> struct lflow_table *lflow_table_alloc(void);
> void lflow_table_init(struct lflow_table *);
> -void lflow_table_clear(struct lflow_table *);
> +void lflow_table_clear(struct lflow_table *, bool);
> void lflow_table_destroy(struct lflow_table *);
> void lflow_table_expand(struct lflow_table *);
> void lflow_table_set_size(struct lflow_table *, size_t);
> diff --git a/northd/northd.c b/northd/northd.c
> index 8b5413ef3..6dbf22e18 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -18012,9 +18012,9 @@ noop_callback(struct worker_pool *pool OVS_UNUSED,
> static void
> fix_flow_table_size(struct lflow_table *lflow_table,
> struct lswitch_flow_build_info *lsiv,
> - size_t n_lsiv)
> + size_t n_lsiv, size_t start)
> {
> - size_t total = 0;
> + size_t total = start;
> for (size_t i = 0; i < n_lsiv; i++) {
> total += lsiv[i].thread_lflow_counter;
> }
> @@ -18089,8 +18089,10 @@ build_lswitch_and_lrouter_flows(
> }
>
> /* Run thread pool. */
> + size_t current_lflow_table_size = hmap_count(&lflows->entries);
> run_pool_callback(build_lflows_pool, NULL, NULL, noop_callback);
> - fix_flow_table_size(lflows, lsiv, build_lflows_pool->size);
> + fix_flow_table_size(lflows, lsiv, build_lflows_pool->size,
> + current_lflow_table_size);
>
> for (index = 0; index < build_lflows_pool->size; index++) {
> ds_destroy(&lsiv[index].match);
> --
> 2.34.1
>
>
> --
>
>
>
>
> _'Esta mensagem é direcionada apenas para os endereços constantes no
> cabeçalho inicial. Se você não está listado nos endereços constantes no
> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa
> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão
> imediatamente anuladas e proibidas'._
>
>
> * **'Apesar do Magazine Luiza tomar
> todas as precauções razoáveis para assegurar que nenhum vírus esteja
> presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por
> quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.*
>
>
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
