On 6/2/23 06:11, Han Zhou wrote:
> This patch introduces a change handler for 'northd' input within the
> 'lflow' node. It specifically handles cases when VIFs are created, which
> is an easier start for lflow incremental processing. Support for
> update/delete will be added later.
> 
> Below are the performance test results simulating an ovn-k8s topology of 500
> nodes x 50 lsp per node:
> 
> Before:
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>     ovn-northd completion:          773ms
> 
> After:
> ovn-nbctl --wait=hv --print-wait-time lsp-add ls_1_0001 lsp_1_0001_01 -- 
> lsp-set-addresses lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101" -- 
> lsp-set-port-security lsp_1_0001_01 "ff:f1:bb:00:01:01 1.0.1.101"
>     ovn-northd completion:          30ms
> 
> It is more than 95% reduction (or 20x faster).
> 
> Signed-off-by: Han Zhou <[email protected]>
> Reviewed-by: Ales Musil <[email protected]>
> ---
>  northd/en-lflow.c        |  82 ++++++++-----
>  northd/en-lflow.h        |   1 +
>  northd/inc-proc-northd.c |   2 +-
>  northd/northd.c          | 245 +++++++++++++++++++++++++++++++++------
>  northd/northd.h          |   6 +-
>  tests/ovn-northd.at      |   4 +-
>  6 files changed, 277 insertions(+), 63 deletions(-)
> 
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index 081ec7c353ed..28ab1c67fb8f 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -30,43 +30,49 @@
>  
>  VLOG_DEFINE_THIS_MODULE(en_lflow);
>  
> -void en_lflow_run(struct engine_node *node, void *data)
> +static void
> +lflow_get_input_data(struct engine_node *node,
> +                     struct lflow_input *lflow_input)
>  {
> -    const struct engine_context *eng_ctx = engine_get_context();
> -
> -    struct lflow_input lflow_input;
> -
>      struct northd_data *northd_data = engine_get_input_data("northd", node);
> -
> -    struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections);
> -
> -    lflow_input.nbrec_bfd_table =
> +    lflow_input->nbrec_bfd_table =
>          EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> -    lflow_input.sbrec_bfd_table =
> +    lflow_input->sbrec_bfd_table =
>          EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> -    lflow_input.sbrec_logical_flow_table =
> +    lflow_input->sbrec_logical_flow_table =
>          EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
> -    lflow_input.sbrec_multicast_group_table =
> +    lflow_input->sbrec_multicast_group_table =
>          EN_OVSDB_GET(engine_get_input("SB_multicast_group", node));
> -    lflow_input.sbrec_igmp_group_table =
> +    lflow_input->sbrec_igmp_group_table =
>          EN_OVSDB_GET(engine_get_input("SB_igmp_group", node));
>  
> -    lflow_input.sbrec_mcast_group_by_name_dp =
> +    lflow_input->sbrec_mcast_group_by_name_dp =
>             engine_ovsdb_node_get_index(
>                            engine_get_input("SB_multicast_group", node),
>                           "sbrec_mcast_group_by_name");
>  
> -    lflow_input.ls_datapaths = &northd_data->ls_datapaths;
> -    lflow_input.lr_datapaths = &northd_data->lr_datapaths;
> -    lflow_input.ls_ports = &northd_data->ls_ports;
> -    lflow_input.lr_ports = &northd_data->lr_ports;
> -    lflow_input.port_groups = &northd_data->port_groups;
> -    lflow_input.meter_groups = &northd_data->meter_groups;
> -    lflow_input.lbs = &northd_data->lbs;
> -    lflow_input.bfd_connections = &bfd_connections;
> -    lflow_input.features = &northd_data->features;
> -    lflow_input.ovn_internal_version_changed =
> +    lflow_input->ls_datapaths = &northd_data->ls_datapaths;
> +    lflow_input->lr_datapaths = &northd_data->lr_datapaths;
> +    lflow_input->ls_ports = &northd_data->ls_ports;
> +    lflow_input->lr_ports = &northd_data->lr_ports;
> +    lflow_input->port_groups = &northd_data->port_groups;
> +    lflow_input->meter_groups = &northd_data->meter_groups;
> +    lflow_input->lbs = &northd_data->lbs;
> +    lflow_input->features = &northd_data->features;
> +    lflow_input->ovn_internal_version_changed =
>                        northd_data->ovn_internal_version_changed;
> +    lflow_input->bfd_connections = NULL;
> +}
> +
> +void en_lflow_run(struct engine_node *node, void *data)
> +{
> +    const struct engine_context *eng_ctx = engine_get_context();
> +
> +    struct lflow_input lflow_input;
> +    lflow_get_input_data(node, &lflow_input);
> +
> +    struct hmap bfd_connections = HMAP_INITIALIZER(&bfd_connections);
> +    lflow_input.bfd_connections = &bfd_connections;
>  
>      struct lflow_data *lflow_data = data;
>      lflow_data_destroy(lflow_data);
> @@ -76,8 +82,8 @@ void en_lflow_run(struct engine_node *node, void *data)
>      build_bfd_table(eng_ctx->ovnsb_idl_txn,
>                      lflow_input.nbrec_bfd_table,
>                      lflow_input.sbrec_bfd_table,
> -                    &bfd_connections,
> -                    &northd_data->lr_ports);
> +                    lflow_input.lr_ports,
> +                    &bfd_connections);
>      build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input, &lflow_data->lflows);
>      bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
>                              &bfd_connections);
> @@ -87,6 +93,30 @@ void en_lflow_run(struct engine_node *node, void *data)
>      engine_set_node_state(node, EN_UPDATED);
>  }
>  
> +bool
> +lflow_northd_handler(struct engine_node *node,
> +                     void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    if (!northd_data->change_tracked) {
> +        return false;
> +    }
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    struct lflow_data *lflow_data = data;
> +
> +    struct lflow_input lflow_input;
> +    lflow_get_input_data(node, &lflow_input);
> +
> +    if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn,
> +                                        &northd_data->tracked_ls_changes,
> +                                        &lflow_input, &lflow_data->lflows)) {
> +        return false;
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +    return true;
> +}
> +
>  void *en_lflow_init(struct engine_node *node OVS_UNUSED,
>                       struct engine_arg *arg OVS_UNUSED)
>  {
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index 0e4d522ff3fe..5e3fbc25e3e0 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -12,5 +12,6 @@
>  void en_lflow_run(struct engine_node *node, void *data);
>  void *en_lflow_init(struct engine_node *node, struct engine_arg *arg);
>  void en_lflow_cleanup(void *data);
> +bool lflow_northd_handler(struct engine_node *, void *data);
>  
>  #endif /* EN_LFLOW_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index f992a9ec8420..f6ceb8280624 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -182,7 +182,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>      engine_add_input(&en_lflow, &en_sb_logical_flow, NULL);
>      engine_add_input(&en_lflow, &en_sb_multicast_group, NULL);
>      engine_add_input(&en_lflow, &en_sb_igmp_group, NULL);
> -    engine_add_input(&en_lflow, &en_northd, NULL);
> +    engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
>  
>      engine_add_input(&en_sync_to_sb_addr_set, &en_nb_address_set,
>                       sync_to_sb_addr_set_nb_address_set_handler);
> diff --git a/northd/northd.c b/northd/northd.c
> index 196653ad3d0e..f8deed53bd8c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -5472,6 +5472,7 @@ ovn_igmp_group_destroy(struct hmap *igmp_groups,
>  
>  struct ovn_lflow {
>      struct hmap_node hmap_node;
> +    struct ovs_list list_node;
>  
>      struct ovn_datapath *od;     /* 'logical_datapath' in SB schema.  */
>      unsigned long *dpg_bitmap;   /* Bitmap of all datapaths by their 
> 'index'.*/
> @@ -5531,6 +5532,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct 
> ovn_datapath *od,
>                 char *match, char *actions, char *io_port, char *ctrl_meter,
>                 char *stage_hint, const char *where)
>  {
> +    ovs_list_init(&lflow->list_node);
>      lflow->dpg_bitmap = bitmap_allocate(dp_bitmap_len);
>      lflow->od = od;
>      lflow->stage = stage;
> @@ -5659,7 +5661,18 @@ ovn_dp_group_add_with_reference(struct ovn_lflow 
> *lflow_ref,
>  
>  /* Adds a row with the specified contents to the Logical_Flow table.
>   * Version to use when hash bucket locking is NOT required.
> + *
> + * Note: This function can add generated lflows to the global variable
> + * temp_lflow_list as its output, controlled by the global variable
> + * add_lflow_to_temp_list. The caller of the ovn_lflow_add_... marcros can 
> get
> + * a list of lflows generated by setting add_lflow_to_temp_list to true. The
> + * caller is responsible for initializing the temp_lflow_list, and also
> + * reset the add_lflow_to_temp_list to false when it is no longer needed.
> + * XXX: this mechanism is temporary and will be replaced when we add hash 
> index
> + * to lflow_data and refactor related functions.
>   */
> +static bool add_lflow_to_temp_list = false;
> +static struct ovs_list temp_lflow_list;
>  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,
> @@ -5676,12 +5689,17 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct 
> ovn_datapath *od,
>      size_t bitmap_len = od ? ods_size(od->datapaths) : dp_bitmap_len;
>      ovs_assert(bitmap_len);
>  
> -    old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> -                               actions, ctrl_meter, hash);
> -    if (old_lflow) {
> -        ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
> -                                        bitmap_len);
> -        return;
> +    if (add_lflow_to_temp_list) {
> +        ovs_assert(od);
> +        ovs_assert(!dp_bitmap);
> +    } else {
> +        old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> +                                   actions, ctrl_meter, hash);
> +        if (old_lflow) {
> +            ovn_dp_group_add_with_reference(old_lflow, od, dp_bitmap,
> +                                            bitmap_len);
> +            return;
> +        }
>      }
>  
>      lflow = xmalloc(sizeof *lflow);
> @@ -5702,6 +5720,10 @@ do_ovn_lflow_add(struct hmap *lflow_map, const struct 
> ovn_datapath *od,
>          hmap_insert_fast(lflow_map, &lflow->hmap_node, hash);
>          thread_lflow_counter++;
>      }
> +
> +    if (add_lflow_to_temp_list) {
> +        ovs_list_insert(&temp_lflow_list, &lflow->list_node);
> +    }
>  }
>  
>  /* Adds a row with the specified contents to the Logical_Flow table. */
> @@ -9947,7 +9969,7 @@ void
>  build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                  const struct nbrec_bfd_table *nbrec_bfd_table,
>                  const struct sbrec_bfd_table *sbrec_bfd_table,
> -                struct hmap *bfd_connections, struct hmap *lr_ports)
> +                const struct hmap *lr_ports, struct hmap *bfd_connections)
>  {
>      struct hmap sb_only = HMAP_INITIALIZER(&sb_only);
>      const struct sbrec_bfd *sb_bt;
> @@ -15299,32 +15321,28 @@ build_lswitch_and_lrouter_iterate_by_lr(struct 
> ovn_datapath *od,
>   */
>  static void
>  build_lswitch_and_lrouter_iterate_by_lsp(struct ovn_port *op,
> -                                         struct lswitch_flow_build_info *lsi)
> +                                         const struct hmap *ls_ports,
> +                                         const struct hmap *lr_ports,
> +                                         const struct shash *meter_groups,
> +                                         struct ds *match,
> +                                         struct ds *actions,
> +                                         struct hmap *lflows)
>  {
>      ovs_assert(op->nbsp);
>  
>      /* Build Logical Switch Flows. */
> -    build_lswitch_port_sec_op(op, lsi->lflows, &lsi->actions, &lsi->match);
> -    build_lswitch_learn_fdb_op(op, lsi->lflows, &lsi->actions,
> -                               &lsi->match);
> -    build_lswitch_arp_nd_responder_skip_local(op, lsi->lflows,
> -                                              &lsi->match);
> -    build_lswitch_arp_nd_responder_known_ips(op, lsi->lflows,
> -                                             lsi->ls_ports,
> -                                             lsi->meter_groups,
> -                                             &lsi->actions,
> -                                             &lsi->match);
> -    build_lswitch_dhcp_options_and_response(op, lsi->lflows,
> -                                            lsi->meter_groups);
> -    build_lswitch_external_port(op, lsi->lflows);
> -    build_lswitch_ip_unicast_lookup(op, lsi->lflows, &lsi->actions,
> -                                    &lsi->match);
> +    build_lswitch_port_sec_op(op, lflows, actions, match);
> +    build_lswitch_learn_fdb_op(op, lflows, actions, match);
> +    build_lswitch_arp_nd_responder_skip_local(op, lflows, match);
> +    build_lswitch_arp_nd_responder_known_ips(op, lflows, ls_ports,
> +                                             meter_groups, actions, match);
> +    build_lswitch_dhcp_options_and_response(op, lflows, meter_groups);
> +    build_lswitch_external_port(op, lflows);
> +    build_lswitch_ip_unicast_lookup(op, lflows, actions, match);
>  
>      /* Build Logical Router Flows. */
> -    build_ip_routing_flows_for_router_type_lsp(op, lsi->lr_ports,
> -                                               lsi->lflows);
> -    build_arp_resolve_flows_for_lsp(op, lsi->lflows, lsi->lr_ports,
> -                                    &lsi->match, &lsi->actions);
> +    build_ip_routing_flows_for_router_type_lsp(op, lr_ports, lflows);
> +    build_arp_resolve_flows_for_lsp(op, lflows, lr_ports, match, actions);
>  }
>  
>  /* Helper function to combine all lflow generation which is iterated by 
> logical
> @@ -15410,7 +15428,12 @@ build_lflows_thread(void *arg)
>                      if (stop_parallel_processing()) {
>                          return NULL;
>                      }
> -                    build_lswitch_and_lrouter_iterate_by_lsp(op, lsi);
> +                    build_lswitch_and_lrouter_iterate_by_lsp(op, 
> lsi->ls_ports,
> +                                                             lsi->lr_ports,
> +                                                             
> lsi->meter_groups,
> +                                                             &lsi->match,
> +                                                             &lsi->actions,
> +                                                             lsi->lflows);
>                  }
>              }
>              for (bnum = control->id;
> @@ -15595,7 +15618,11 @@ build_lswitch_and_lrouter_flows(const struct 
> ovn_datapaths *ls_datapaths,
>          stopwatch_stop(LFLOWS_DATAPATHS_STOPWATCH_NAME, time_msec());
>          stopwatch_start(LFLOWS_PORTS_STOPWATCH_NAME, time_msec());
>          HMAP_FOR_EACH (op, key_node, ls_ports) {
> -            build_lswitch_and_lrouter_iterate_by_lsp(op, &lsi);
> +            build_lswitch_and_lrouter_iterate_by_lsp(op, lsi.ls_ports,
> +                                                     lsi.lr_ports,
> +                                                     lsi.meter_groups,
> +                                                     &lsi.match, 
> &lsi.actions,
> +                                                     lsi.lflows);
>          }
>          HMAP_FOR_EACH (op, key_node, lr_ports) {
>              build_lswitch_and_lrouter_iterate_by_lrp(op, &lsi);
> @@ -15677,6 +15704,20 @@ build_mcast_groups(const struct 
> sbrec_igmp_group_table *sbrec_igmp_group_table,
>                     struct hmap *mcast_groups,
>                     struct hmap *igmp_groups);
>  
> +static struct sbrec_multicast_group *
> +create_sb_multicast_group(struct ovsdb_idl_txn *ovnsb_txn,
> +                          const struct sbrec_datapath_binding *dp,
> +                          const char *name,
> +                          int64_t tunnel_key)
> +{
> +    struct sbrec_multicast_group *sbmc =
> +        sbrec_multicast_group_insert(ovnsb_txn);
> +    sbrec_multicast_group_set_datapath(sbmc, dp);
> +    sbrec_multicast_group_set_name(sbmc, name);
> +    sbrec_multicast_group_set_tunnel_key(sbmc, tunnel_key);
> +    return sbmc;
> +}
> +
>  /* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB 
> database,
>   * constructing their contents based on the OVN_NB database. */
>  void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> @@ -16002,10 +16043,8 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
>              ovn_multicast_destroy(&mcast_groups, mc);
>              continue;
>          }
> -        sbmc = sbrec_multicast_group_insert(ovnsb_txn);
> -        sbrec_multicast_group_set_datapath(sbmc, mc->datapath->sb);
> -        sbrec_multicast_group_set_name(sbmc, mc->group->name);
> -        sbrec_multicast_group_set_tunnel_key(sbmc, mc->group->key);
> +        sbmc = create_sb_multicast_group(ovnsb_txn, mc->datapath->sb,
> +                                         mc->group->name, mc->group->key);
>          ovn_multicast_update_sbrec(mc, sbmc);
>          ovn_multicast_destroy(&mcast_groups, mc);
>      }
> @@ -16020,6 +16059,146 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
>      hmap_destroy(&mcast_groups);
>  }
>  
> +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
> +                                    struct tracked_ls_changes *ls_changes,
> +                                    struct lflow_input *lflow_input,
> +                                    struct hmap *lflows)
> +{
> +    struct ls_change *ls_change;
> +    LIST_FOR_EACH (ls_change, list_node, &ls_changes->updated) {
> +        ovs_list_init(&temp_lflow_list);
> +        add_lflow_to_temp_list = true;
> +        if (!ovs_list_is_empty(&ls_change->updated_ports) ||
> +            !ovs_list_is_empty(&ls_change->deleted_ports)) {
> +            /* XXX: implement lflow index so that we can handle updated and
> +             * deleted LSPs incrementally. */
> +            ovs_list_init(&temp_lflow_list);
> +            add_lflow_to_temp_list = false;
> +            return false;
> +        }
> +
> +        const struct sbrec_multicast_group *sbmc_flood =
> +            mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> +                               MC_FLOOD, ls_change->od->sb);
> +        const struct sbrec_multicast_group *sbmc_flood_l2 =
> +            mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> +                               MC_FLOOD_L2, ls_change->od->sb);
> +        const struct sbrec_multicast_group *sbmc_unknown =
> +            mcast_group_lookup(lflow_input->sbrec_mcast_group_by_name_dp,
> +                               MC_UNKNOWN, ls_change->od->sb);
> +
> +        struct ovn_port *op;
> +        LIST_FOR_EACH (op, list, &ls_change->added_ports) {
> +            struct ds match = DS_EMPTY_INITIALIZER;
> +            struct ds actions = DS_EMPTY_INITIALIZER;
> +            build_lswitch_and_lrouter_iterate_by_lsp(op, 
> lflow_input->ls_ports,
> +                                                     lflow_input->lr_ports,
> +                                                     
> lflow_input->meter_groups,
> +                                                     &match, &actions,
> +                                                     lflows);
> +            ds_destroy(&match);
> +            ds_destroy(&actions);
> +            if (!sbmc_flood) {
> +                sbmc_flood = create_sb_multicast_group(ovnsb_txn,
> +                    ls_change->od->sb, MC_FLOOD, OVN_MCAST_FLOOD_TUNNEL_KEY);
> +            }
> +            sbrec_multicast_group_update_ports_addvalue(sbmc_flood, op->sb);
> +
> +            if (!sbmc_flood_l2) {
> +                sbmc_flood_l2 = create_sb_multicast_group(ovnsb_txn,
> +                    ls_change->od->sb, MC_FLOOD_L2,
> +                    OVN_MCAST_FLOOD_L2_TUNNEL_KEY);
> +            }
> +            sbrec_multicast_group_update_ports_addvalue(sbmc_flood_l2, 
> op->sb);
> +
> +            if (op->has_unknown) {
> +                if (!sbmc_unknown) {
> +                    sbmc_unknown = create_sb_multicast_group(ovnsb_txn,
> +                        ls_change->od->sb, MC_UNKNOWN,
> +                        OVN_MCAST_UNKNOWN_TUNNEL_KEY);
> +                }
> +                sbrec_multicast_group_update_ports_addvalue(sbmc_unknown,
> +                                                            op->sb);
> +            }
> +        }
> +        /* Sync the newly added flows to SB. */
> +        struct ovn_lflow *lflow;
> +        LIST_FOR_EACH (lflow, list_node, &temp_lflow_list) {
> +            size_t n_datapaths;
> +            struct ovn_datapath **datapaths_array;
> +            if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) {
> +                n_datapaths = ods_size(lflow_input->ls_datapaths);
> +                datapaths_array = lflow_input->ls_datapaths->array;
> +            } else {
> +                n_datapaths = ods_size(lflow_input->lr_datapaths);
> +                datapaths_array = lflow_input->lr_datapaths->array;
> +            }
> +            uint32_t n_ods = bitmap_count1(lflow->dpg_bitmap, n_datapaths);
> +            ovs_assert(n_ods == 1);
> +            /* There is only one datapath, so it should be moved out of the
> +             * group to a single 'od'. */
> +            size_t index = bitmap_scan(lflow->dpg_bitmap, true, 0,
> +                                       n_datapaths);
> +
> +            bitmap_set0(lflow->dpg_bitmap, index);
> +            lflow->od = datapaths_array[index];
> +
> +            /* Logical flow should be re-hashed to allow lookups. */
> +            uint32_t hash = hmap_node_hash(&lflow->hmap_node);
> +            /* Remove from lflows. */
> +            hmap_remove(lflows, &lflow->hmap_node);
> +            hash = 
> ovn_logical_flow_hash_datapath(&lflow->od->sb->header_.uuid,
> +                                                  hash);
> +            /* Add back. */
> +            hmap_insert(lflows, &lflow->hmap_node, hash);
> +
> +            /* Sync to SB. */
> +            const struct sbrec_logical_flow *sbflow;
> +            sbflow = sbrec_logical_flow_insert(ovnsb_txn);
> +            const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
> +            uint8_t table = ovn_stage_get_table(lflow->stage);
> +            sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> +            sbrec_logical_flow_set_logical_dp_group(sbflow, NULL);
> +            sbrec_logical_flow_set_pipeline(sbflow, pipeline);
> +            sbrec_logical_flow_set_table_id(sbflow, table);
> +            sbrec_logical_flow_set_priority(sbflow, lflow->priority);
> +            sbrec_logical_flow_set_match(sbflow, lflow->match);
> +            sbrec_logical_flow_set_actions(sbflow, lflow->actions);
> +            if (lflow->io_port) {
> +                struct smap tags = SMAP_INITIALIZER(&tags);
> +                smap_add(&tags, "in_out_port", lflow->io_port);
> +                sbrec_logical_flow_set_tags(sbflow, &tags);
> +                smap_destroy(&tags);
> +            }
> +            sbrec_logical_flow_set_controller_meter(sbflow, 
> lflow->ctrl_meter);
> +            /* Trim the source locator lflow->where, which looks something 
> like
> +             * "ovn/northd/northd.c:1234", down to just the part following 
> the
> +             * last slash, e.g. "northd.c:1234". */
> +            const char *slash = strrchr(lflow->where, '/');
> +#if _WIN32
> +            const char *backslash = strrchr(lflow->where, '\\');
> +            if (!slash || backslash > slash) {
> +                slash = backslash;
> +            }
> +#endif
> +            const char *where = slash ? slash + 1 : lflow->where;
> +
> +            struct smap ids = SMAP_INITIALIZER(&ids);
> +            smap_add(&ids, "stage-name", ovn_stage_to_str(lflow->stage));
> +            smap_add(&ids, "source", where);
> +            if (lflow->stage_hint) {
> +                smap_add(&ids, "stage-hint", lflow->stage_hint);
> +            }
> +            sbrec_logical_flow_set_external_ids(sbflow, &ids);
> +            smap_destroy(&ids);
> +        }

Hi Han, Numan,

I'm quite worried about this kind of duplication.  Most of this code is
the same as the one that iterates the lflow hash map in build_lflows().
With some subtle differences.

In my opinion we need to refactor this.  It will be impossible to
maintain correctly when we have to make changes in one of the two cases.

Again, sorry for bringing this up so late.  But, as we add more I-P,
I think it's best if we try to avoid this kind of duplication.

If you're busy I can try to see if I can refactor this part and maybe
figure out the lflow index TODO:

/* XXX: implement lflow index so that we can handle updated and
 * deleted LSPs incrementally. */

Wdyt?

Thanks,
Dumitru

> +    }
> +    ovs_list_init(&temp_lflow_list);
> +    add_lflow_to_temp_list = false;
> +    return true;
> +
> +}
> +
>  /* Each port group in Port_Group table in OVN_Northbound has a corresponding
>   * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the entries
>   * contains lport uuids, while in OVN_Southbound we store the lport names.
> diff --git a/northd/northd.h b/northd/northd.h
> index 1fd9bed477e7..6cb88fe87915 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -331,11 +331,15 @@ void northd_indices_create(struct northd_data *data,
>  void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
>                    struct lflow_input *input_data,
>                    struct hmap *lflows);
> +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn,
> +                                    struct tracked_ls_changes *,
> +                                    struct lflow_input *, struct hmap 
> *lflows);
>  
>  void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                       const struct nbrec_bfd_table *,
>                       const struct sbrec_bfd_table *,
> -                     struct hmap *bfd_connections, struct hmap *lr_ports);
> +                     const struct hmap *lr_ports,
> +                     struct hmap *bfd_connections);
>  void bfd_cleanup_connections(const struct nbrec_bfd_table *,
>                               struct hmap *bfd_map);
>  void run_update_worker_pool(int n_threads);
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 74ae84530112..635cd2d892e6 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -9506,14 +9506,14 @@ check as northd ovn-appctl -t NORTHD_TYPE 
> inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-1 -- lsp-set-addresses lsp0-1 
> "aa:aa:aa:00:00:01 192.168.0.11"
>  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [3
>  ])
> -AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [6
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [5
>  ])
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats
>  check ovn-nbctl --wait=hv lsp-add ls0 lsp0-2 -- lsp-set-addresses lsp0-2 
> "aa:aa:aa:00:00:02 192.168.0.12"
>  AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd 
> recompute], [0], [3
>  ])
> -AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [6
> +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats lflow 
> recompute], [0], [5
>  ])
>  
>  check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats

_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to