[ovs-dev] northd: Add bfd and bfd_consumer nodes to I-P engine.

Introduce bfd and bfd_consumer nodes to northd I-P engine to track bfd
connections and northd static_route/policy_route changes.

Reported-at: https://issues.redhat.com/browse/FDP-600
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/en-lflow.c        |  19 +--
 northd/en-northd.c       |  92 +++++++++++++
 northd/en-northd.h       |   8 ++
 northd/inc-proc-northd.c |  21 ++-
 northd/northd.c          | 274 ++++++++++++++++++++++++---------------
 northd/northd.h          |  48 ++++++-
 6 files changed, 344 insertions(+), 118 deletions(-)

On Thu, May 2, 2024 at 8:22 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
wrote:
>
> Introduce bfd and bfd_consumer nodes to northd I-P engine to track bfd
> connections and northd static_route/policy_route changes.
>
> Reported-at: https://issues.redhat.com/browse/FDP-600
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>

Hi Lorenzo,

Thanks for the patch. Could you explain in more detail about the motivation
and background information of this patch? I followed the link in
Reported-at but still not clear.
I assume it is for performance reasons, so could you explain what scenarios
would benefit from this?

For the patch, I haven't looked into details yet, but it seems you didn't
add any change handler for the two new nodes. So will there be follow up
patches for the real performance improvement?
But even for the current patch, with this: engine_add_input(&en_bfd,
&en_northd, NULL); does it mean any en_northd change would trigger a lflow
recompute because of the dependency en_northd -> en_bfd -> en_lflow. Would
this be a performance regression? If not, why?

For the noop handler used, please provide a detailed explanation to justify
it: why the node depends on the input but doesn't need to process when the
input changes?

Thanks,
Han

> ---
>  northd/en-lflow.c        |  19 +--
>  northd/en-northd.c       |  92 +++++++++++++
>  northd/en-northd.h       |   8 ++
>  northd/inc-proc-northd.c |  21 ++-
>  northd/northd.c          | 274 ++++++++++++++++++++++++---------------
>  northd/northd.h          |  48 ++++++-
>  6 files changed, 344 insertions(+), 118 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index c4b927fb8..9efbd5117 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -41,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
>                       struct lflow_input *lflow_input)
>  {
>      struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +    struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> +    struct bfd_consumer_data *bfd_consumer_data =
> +        engine_get_input_data("bfd_consumer", node);
>      struct port_group_data *pg_data =
>          engine_get_input_data("port_group", node);
>      struct sync_meters_data *sync_meters_data =
> @@ -78,7 +81,10 @@ lflow_get_input_data(struct engine_node *node,
>      lflow_input->meter_groups = &sync_meters_data->meter_groups;
>      lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map;
>      lflow_input->svc_monitor_map = &northd_data->svc_monitor_map;
> -    lflow_input->bfd_connections = NULL;
> +    lflow_input->bfd_connections = &bfd_data->bfd_connections;
> +    lflow_input->parsed_routes = &bfd_consumer_data->parsed_routes;
> +    lflow_input->route_tables = &bfd_consumer_data->route_tables;
> +    lflow_input->route_policies = &bfd_consumer_data->route_policies;
>
>      struct ed_type_global_config *global_config =
>          engine_get_input_data("global_config", node);
> @@ -95,25 +101,14 @@ void en_lflow_run(struct engine_node *node, void
*data)
>      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;
> -
>      stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>      struct lflow_data *lflow_data = data;
>      lflow_table_clear(lflow_data->lflow_table);
>      lflow_reset_northd_refs(&lflow_input);
>
> -    build_bfd_table(eng_ctx->ovnsb_idl_txn,
> -                    lflow_input.nbrec_bfd_table,
> -                    lflow_input.sbrec_bfd_table,
> -                    lflow_input.lr_ports,
> -                    &bfd_connections);
>      build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input,
>                   lflow_data->lflow_table);
> -    bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
> -                            &bfd_connections);
> -    hmap_destroy(&bfd_connections);
>      stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
>
>      engine_set_node_state(node, EN_UPDATED);
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index 4479b4aff..2d8c05608 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,66 @@ northd_global_config_handler(struct engine_node
*node, void *data OVS_UNUSED)
>      return true;
>  }
>
> +void
> +en_bfd_consumer_run(struct engine_node *node, void *data)
> +{
> +
> +    struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +    struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> +    const struct nbrec_bfd_table *nbrec_bfd_table =
> +        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> +    struct bfd_consumer_data *bfd_consumer_data = data;
> +
> +    bfd_consumer_destroy(data);
> +    bfd_consumer_init(data);
> +
> +    struct ovn_datapath *od;
> +    HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) {
> +        for (int i = 0; i < od->nbr->n_ports; i++) {
> +            const char *route_table_name =
> +                smap_get(&od->nbr->ports[i]->options, "route_table");
> +            get_route_table_id(&bfd_consumer_data->route_tables,
> +                               route_table_name);
> +        }
> +        for (int i = 0; i < od->nbr->n_static_routes; i++) {
> +                parsed_routes_add(od, &northd_data->lr_ports,
> +                                  &bfd_consumer_data->parsed_routes,
> +                                  &bfd_consumer_data->route_tables,
> +                                  od->nbr->static_routes[i],
> +                                  &bfd_data->bfd_connections);
> +        }
> +        build_route_policies(od, &northd_data->lr_ports,
> +                             &bfd_data->bfd_connections,
> +                             &bfd_consumer_data->route_policies);
> +    }
> +    bfd_cleanup_connections(nbrec_bfd_table, &bfd_data->bfd_connections);
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +}
> +
> +void
> +en_bfd_run(struct engine_node *node, void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd",
node);
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    struct bfd_data *bfd_data = data;
> +    const struct nbrec_bfd_table *nbrec_bfd_table =
> +        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> +    const struct sbrec_bfd_table *sbrec_bfd_table =
> +        EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> +
> +    bfd_destroy(data);
> +    bfd_init(data);
> +    if (build_bfd_table(eng_ctx->ovnsb_idl_txn,
> +                        nbrec_bfd_table, sbrec_bfd_table,
> +                        &northd_data->lr_ports,
> +                        &bfd_data->bfd_connections)) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    } else {
> +        engine_set_node_state(node, EN_UNCHANGED);
> +    }
> +}
> +
>  void
>  *en_northd_init(struct engine_node *node OVS_UNUSED,
>                  struct engine_arg *arg OVS_UNUSED)
> @@ -247,6 +307,26 @@ void
>      return data;
>  }
>
> +void
> +*en_bfd_consumer_init(struct engine_node *node OVS_UNUSED,
> +                      struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct bfd_consumer_data *data = xzalloc(sizeof *data);
> +
> +    bfd_consumer_init(data);
> +    return data;
> +}
> +
> +void
> +*en_bfd_init(struct engine_node *node OVS_UNUSED,
> +             struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct bfd_data *data = xzalloc(sizeof *data);
> +
> +    bfd_init(data);
> +    return data;
> +}
> +
>  void
>  en_northd_cleanup(void *data)
>  {
> @@ -259,3 +339,15 @@ en_northd_clear_tracked_data(void *data_)
>      struct northd_data *data = data_;
>      destroy_northd_data_tracked_changes(data);
>  }
> +
> +void
> +en_bfd_consumer_cleanup(void *data)
> +{
> +    bfd_consumer_destroy(data);
> +}
> +
> +void
> +en_bfd_cleanup(void *data)
> +{
> +    bfd_destroy(data);
> +}
> diff --git a/northd/en-northd.h b/northd/en-northd.h
> index 9b7bda32a..55b9dd83c 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -19,5 +19,13 @@ bool northd_nb_logical_switch_handler(struct
engine_node *, void *data);
>  bool northd_nb_logical_router_handler(struct engine_node *, void *data);
>  bool northd_sb_port_binding_handler(struct engine_node *, void *data);
>  bool northd_lb_data_handler(struct engine_node *, void *data);
> +void *en_bfd_consumer_init(struct engine_node *node OVS_UNUSED,
> +                           struct engine_arg *arg OVS_UNUSED);
> +void en_bfd_consumer_cleanup(void *data);
> +void en_bfd_consumer_run(struct engine_node *node, void *data);
> +void *en_bfd_init(struct engine_node *node OVS_UNUSED,
> +                  struct engine_arg *arg OVS_UNUSED);
> +void en_bfd_cleanup(void *data);
> +void en_bfd_run(struct engine_node *node, void *data);
>
>  #endif /* EN_NORTHD_H */
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index e1073812c..9491c974c 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -61,7 +61,9 @@ static unixctl_cb_func chassis_features_list;
>      NB_NODE(meter, "meter") \
>      NB_NODE(bfd, "bfd") \
>      NB_NODE(static_mac_binding, "static_mac_binding") \
> -    NB_NODE(chassis_template_var, "chassis_template_var")
> +    NB_NODE(chassis_template_var, "chassis_template_var") \
> +    NB_NODE(logical_router_static_route, "logical_router_static_route") \
> +    NB_NODE(logical_router_policy, "logical_router_policy")
>
>      enum nb_engine_node {
>  #define NB_NODE(NAME, NAME_STR) NB_##NAME,
> @@ -155,6 +157,8 @@ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data,
"lb_data");
>  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_nat, "lr_nat");
>  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_stateful, "lr_stateful");
>  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful, "ls_stateful");
> +static ENGINE_NODE(bfd_consumer, "bfd_consumer");
> +static ENGINE_NODE(bfd, "bfd");
>
>  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                            struct ovsdb_idl_loop *sb)
> @@ -237,6 +241,19 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>      engine_add_input(&en_fdb_aging, &en_global_config,
>                       node_global_config_handler);
>
> +    engine_add_input(&en_bfd, &en_nb_bfd, NULL);
> +    engine_add_input(&en_bfd, &en_sb_bfd, NULL);
> +    engine_add_input(&en_bfd, &en_northd, NULL);
> +    engine_add_input(&en_bfd, &en_nb_logical_router_policy, NULL);
> +    engine_add_input(&en_bfd, &en_nb_logical_router_static_route, NULL);
> +
> +    engine_add_input(&en_bfd_consumer, &en_bfd, NULL);
> +    engine_add_input(&en_bfd_consumer, &en_northd, engine_noop_handler);
> +    engine_add_input(&en_bfd_consumer, &en_nb_bfd, NULL);
> +    engine_add_input(&en_bfd_consumer, &en_nb_logical_router_policy,
NULL);
> +    engine_add_input(&en_bfd_consumer,
&en_nb_logical_router_static_route,
> +                     NULL);
> +
>      engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
>      engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
>      engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
> @@ -255,6 +272,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>      engine_add_input(&en_lflow, &en_port_group,
lflow_port_group_handler);
>      engine_add_input(&en_lflow, &en_lr_stateful,
lflow_lr_stateful_handler);
>      engine_add_input(&en_lflow, &en_ls_stateful,
lflow_ls_stateful_handler);
> +    engine_add_input(&en_lflow, &en_bfd_consumer, NULL);
> +    engine_add_input(&en_lflow, &en_bfd, NULL);
>
>      engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL);
>      engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, NULL);
> diff --git a/northd/northd.c b/northd/northd.c
> index 5e12fd1e8..6f9aa3ff9 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9780,47 +9780,53 @@ bfd_cleanup_connections(const struct
nbrec_bfd_table *nbrec_bfd_table,
>              nbrec_bfd_set_status(nb_bt, "admin_down");
>          }
>      }
> -
> -    HMAP_FOR_EACH_POP (bfd_e, hmap_node, bfd_map) {
> -        free(bfd_e);
> -    }
>  }
>
>  #define BFD_DEF_MINTX       1000 /* 1s */
>  #define BFD_DEF_MINRX       1000 /* 1s */
>  #define BFD_DEF_DETECT_MULT 5
>
> -static void
> +static bool
>  build_bfd_update_sb_conf(const struct nbrec_bfd *nb_bt,
>                           const struct sbrec_bfd *sb_bt)
>  {
> +    bool ret = false;
> +
>      if (strcmp(nb_bt->dst_ip, sb_bt->dst_ip)) {
>          sbrec_bfd_set_dst_ip(sb_bt, nb_bt->dst_ip);
> +        ret = true;
>      }
>
>      if (strcmp(nb_bt->logical_port, sb_bt->logical_port)) {
>          sbrec_bfd_set_logical_port(sb_bt, nb_bt->logical_port);
> +        ret = true;
>      }
>
>      if (strcmp(nb_bt->status, sb_bt->status)) {
>          sbrec_bfd_set_status(sb_bt, nb_bt->status);
> +        ret = true;
>      }
>
>      int detect_mult = nb_bt->n_detect_mult ? nb_bt->detect_mult[0]
>                                             : BFD_DEF_DETECT_MULT;
>      if (detect_mult != sb_bt->detect_mult) {
>          sbrec_bfd_set_detect_mult(sb_bt, detect_mult);
> +        ret = true;
>      }
>
>      int min_tx = nb_bt->n_min_tx ? nb_bt->min_tx[0] : BFD_DEF_MINTX;
>      if (min_tx != sb_bt->min_tx) {
>          sbrec_bfd_set_min_tx(sb_bt, min_tx);
> +        ret = true;
>      }
>
>      int min_rx = nb_bt->n_min_rx ? nb_bt->min_rx[0] : BFD_DEF_MINRX;
>      if (min_rx != sb_bt->min_rx) {
>          sbrec_bfd_set_min_rx(sb_bt, min_rx);
> +        ret = true;
>      }
> +
> +    return ret;
>  }
>
>  /* RFC 5881 section 4
> @@ -9846,7 +9852,7 @@ static int bfd_get_unused_port(unsigned long
*bfd_src_ports)
>      return port + BFD_UDP_SRC_PORT_START;
>  }
>
> -void
> +bool
>  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,
> @@ -9856,6 +9862,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>      const struct sbrec_bfd *sb_bt;
>      unsigned long *bfd_src_ports;
>      struct bfd_entry *bfd_e;
> +    bool ret = false;
>      uint32_t hash;
>
>      bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN);
> @@ -9901,6 +9908,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>              int d_mult = nb_bt->n_detect_mult ? nb_bt->detect_mult[0]
>                                                : BFD_DEF_DETECT_MULT;
>              sbrec_bfd_set_detect_mult(sb_bt, d_mult);
> +            ret = true;
>          } else {
>              if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
>                  if (!strcmp(nb_bt->status, "admin_down") ||
> @@ -9909,12 +9917,16 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                  } else {
>                      nbrec_bfd_set_status(nb_bt, bfd_e->sb_bt->status);
>                  }
> +                ret = true;
> +            }
> +            if (build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt)) {
> +                ret = true;
>              }
> -            build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
>              if (op && op->sb && op->sb->chassis &&
>                  strcmp(op->sb->chassis->name,
bfd_e->sb_bt->chassis_name)) {
>                  sbrec_bfd_set_chassis_name(bfd_e->sb_bt,
>                                             op->sb->chassis->name);
> +                ret = true;
>              }
>
>              hmap_remove(&sb_only, &bfd_e->hmap_node);
> @@ -9937,10 +9949,13 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>          }
>          sbrec_bfd_delete(bfd_e->sb_bt);
>          free(bfd_e);
> +        ret = true;
>      }
>      hmap_destroy(&sb_only);
>
>      bitmap_free(bfd_src_ports);
> +
> +    return ret;
>  }
>
>  /* Returns a string of the IP address of the router port 'op' that
> @@ -10032,20 +10047,22 @@ static bool check_bfd_state(
>
>  static void
>  build_routing_policy_flow(struct lflow_table *lflows, struct
ovn_datapath *od,
> -                          const struct hmap *lr_ports,
> -                          const struct nbrec_logical_router_policy *rule,
> -                          const struct hmap *bfd_connections,
> +                          const struct hmap *lr_ports, struct
route_policy *rp,
>                            const struct ovsdb_idl_row *stage_hint,
>                            struct lflow_ref *lflow_ref)
>  {
> +    const struct nbrec_logical_router_policy *rule = rp->rule;
>      struct ds match = DS_EMPTY_INITIALIZER;
>      struct ds actions = DS_EMPTY_INITIALIZER;
>
>      if (!strcmp(rule->action, "reroute")) {
>          ovs_assert(rule->n_nexthops <= 1);
>
> -        char *nexthop =
> -            (rule->n_nexthops == 1 ? rule->nexthops[0] : rule->nexthop);
> +        if (!rp->n_valid_nexthops) {
> +            return;
> +        }
> +
> +        char *nexthop = rp->valid_nexthops[0];
>          struct ovn_port *out_port =
get_outport_for_routing_policy_nexthop(
>               od, lr_ports, rule->priority, nexthop);
>          if (!out_port) {
> @@ -10061,10 +10078,6 @@ build_routing_policy_flow(struct lflow_table
*lflows, struct ovn_datapath *od,
>              return;
>          }
>
> -        if (!check_bfd_state(rule, bfd_connections, out_port, nexthop)) {
> -            return;
> -        }
> -
>          uint32_t pkt_mark = smap_get_uint(&rule->options, "pkt_mark", 0);
>          if (pkt_mark) {
>              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
> @@ -10107,19 +10120,18 @@ static void
>  build_ecmp_routing_policy_flows(struct lflow_table *lflows,
>                                  struct ovn_datapath *od,
>                                  const struct hmap *lr_ports,
> -                                const struct nbrec_logical_router_policy
*rule,
> -                                const struct hmap *bfd_connections,
> +                                struct route_policy *rp,
>                                  uint16_t ecmp_group_id,
>                                  struct lflow_ref *lflow_ref)
>  {
> -    ovs_assert(rule->n_nexthops > 1);
> -
>      bool nexthops_is_ipv4 = true;
> +    const struct nbrec_logical_router_policy *rule = rp->rule;
> +    ovs_assert(rule->n_nexthops > 1);
>
>      /* Check that all the nexthops belong to the same addr family before
>       * adding logical flows. */
> -    for (uint16_t i = 0; i < rule->n_nexthops; i++) {
> -        bool is_ipv4 = strchr(rule->nexthops[i], '.') ? true : false;
> +    for (uint16_t i = 0; i < rp->n_valid_nexthops; i++) {
> +        bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false;
>
>          if (i == 0) {
>              nexthops_is_ipv4 = is_ipv4;
> @@ -10130,7 +10142,7 @@ build_ecmp_routing_policy_flows(struct
lflow_table *lflows,
>              VLOG_WARN_RL(&rl, "nexthop [%s] of the router policy with "
>                           "the match [%s] do not belong to the same
address "
>                           "family as other next hops",
> -                         rule->nexthops[i], rule->match);
> +                         rp->valid_nexthops[i], rule->match);
>              return;
>          }
>      }
> @@ -10138,40 +10150,30 @@ build_ecmp_routing_policy_flows(struct
lflow_table *lflows,
>      struct ds match = DS_EMPTY_INITIALIZER;
>      struct ds actions = DS_EMPTY_INITIALIZER;
>
> -    size_t *valid_nexthops = xcalloc(rule->n_nexthops, sizeof
*valid_nexthops);
> -    size_t n_valid_nexthops = 0;
> -
> -    for (size_t i = 0; i < rule->n_nexthops; i++) {
> +    for (size_t i = 0; i < rp->n_valid_nexthops; i++) {
>          struct ovn_port *out_port =
get_outport_for_routing_policy_nexthop(
> -             od, lr_ports, rule->priority, rule->nexthops[i]);
> +             od, lr_ports, rule->priority, rp->valid_nexthops[i]);
>          if (!out_port) {
>              goto cleanup;
>          }
>
>          const char *lrp_addr_s =
> -            find_lrp_member_ip(out_port, rule->nexthops[i]);
> +            find_lrp_member_ip(out_port, rp->valid_nexthops[i]);
>          if (!lrp_addr_s) {
>              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
1);
>              VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
>                              " priority %"PRId64" nexthop %s",
> -                            rule->priority, rule->nexthops[i]);
> +                            rule->priority, rp->valid_nexthops[i]);
>              goto cleanup;
>          }
>
> -        if (!check_bfd_state(rule, bfd_connections, out_port,
> -                             rule->nexthops[i])) {
> -            continue;
> -        }
> -
> -        valid_nexthops[n_valid_nexthops++] = i + 1;
> -
>          ds_clear(&actions);
>          uint32_t pkt_mark = smap_get_uint(&rule->options, "pkt_mark", 0);
>          if (pkt_mark) {
>              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
>          }
>
> -        bool is_ipv4 = strchr(rule->nexthops[i], '.') ? true : false;
> +        bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false;
>
>          ds_put_format(&actions, "%s = %s; "
>                        "%s = %s; "
> @@ -10180,7 +10182,7 @@ build_ecmp_routing_policy_flows(struct
lflow_table *lflows,
>                        "flags.loopback = 1; "
>                        "next;",
>                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> -                      rule->nexthops[i],
> +                      rp->valid_nexthops[i],
>                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
>                        lrp_addr_s,
>                        out_port->lrp_networks.ea_s,
> @@ -10196,37 +10198,30 @@ build_ecmp_routing_policy_flows(struct
lflow_table *lflows,
>                                  lflow_ref);
>      }
>
> -    if (!n_valid_nexthops) {
> -        goto cleanup;
> -    }
> -
>      ds_clear(&actions);
> -    if (n_valid_nexthops > 1) {
> +    if (rp->n_valid_nexthops > 1) {
>          ds_put_format(&actions, "%s = %"PRIu16
>                        "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id,
>                        REG_ECMP_MEMBER_ID);
>
> -        for (size_t i = 0; i < n_valid_nexthops; i++) {
> +        for (size_t i = 0; i < rp->n_valid_nexthops; i++) {
>              if (i > 0) {
>                  ds_put_cstr(&actions, ", ");
>              }
>
> -            ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]);
> +            ds_put_format(&actions, "%"PRIuSIZE, i + 1);
>          }
>          ds_put_cstr(&actions, ");");
>      } else {
>          ds_put_format(&actions, "%s = %"PRIu16
> -                      "; %s = %"PRIuSIZE"; next;", REG_ECMP_GROUP_ID,
> -                      ecmp_group_id, REG_ECMP_MEMBER_ID,
> -                      valid_nexthops[0]);
> +                      "; %s = 1; next;", REG_ECMP_GROUP_ID,
> +                      ecmp_group_id, REG_ECMP_MEMBER_ID);
>      }
>      ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY,
>                              rule->priority, rule->match,
>                              ds_cstr(&actions), &rule->header_,
>                              lflow_ref);
> -
>  cleanup:
> -    free(valid_nexthops);
>      ds_destroy(&match);
>      ds_destroy(&actions);
>  }
> @@ -10251,7 +10246,7 @@ route_table_add(struct simap *route_tables, const
char *route_table_name)
>      return rtb_id;
>  }
>
> -static uint32_t
> +uint32_t
>  get_route_table_id(struct simap *route_tables, const char
*route_table_name)
>  {
>      if (!route_table_name || !route_table_name[0]) {
> @@ -10292,18 +10287,6 @@ build_route_table_lflow(struct ovn_datapath *od,
struct lflow_table *lflows,
>      ds_destroy(&actions);
>  }
>
> -struct parsed_route {
> -    struct ovs_list list_node;
> -    struct in6_addr prefix;
> -    unsigned int plen;
> -    bool is_src_route;
> -    uint32_t route_table_id;
> -    uint32_t hash;
> -    const struct nbrec_logical_router_static_route *route;
> -    bool ecmp_symmetric_reply;
> -    bool is_discard_route;
> -};
> -
>  static uint32_t
>  route_hash(struct parsed_route *route)
>  {
> @@ -10318,9 +10301,9 @@ find_static_route_outport(struct ovn_datapath
*od, const struct hmap *lr_ports,
>
>  /* Parse and validate the route. Return the parsed route if successful.
>   * Otherwise return NULL. */
> -static struct parsed_route *
> +struct parsed_route *
>  parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> -                  struct ovs_list *routes, struct simap *route_tables,
> +                  struct hmap *routes, struct simap *route_tables,
>                    const struct nbrec_logical_router_static_route *route,
>                    const struct hmap *bfd_connections)
>  {
> @@ -10410,20 +10393,10 @@ parsed_routes_add(struct ovn_datapath *od,
const struct hmap *lr_ports,
>      pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
>                                               "ecmp_symmetric_reply",
false);
>      pr->is_discard_route = is_discard_route;
> -    ovs_list_insert(routes, &pr->list_node);
> +    hmap_insert(routes, &pr->key_node, uuid_hash(&od->key));
>      return pr;
>  }
>
> -static void
> -parsed_routes_destroy(struct ovs_list *routes)
> -{
> -    struct parsed_route *pr;
> -    LIST_FOR_EACH_SAFE (pr, list_node, routes) {
> -        ovs_list_remove(&pr->list_node);
> -        free(pr);
> -    }
> -}
> -
>  struct ecmp_route_list_node {
>      struct ovs_list list_node;
>      uint16_t id; /* starts from 1 */
> @@ -12700,7 +12673,8 @@ static void
>  build_static_route_flows_for_lrouter(
>          struct ovn_datapath *od, const struct chassis_features *features,
>          struct lflow_table *lflows, const struct hmap *lr_ports,
> -        const struct hmap *bfd_connections,
> +        struct hmap *parsed_routes,
> +        struct simap *route_tables,
>          struct lflow_ref *lflow_ref)
>  {
>      ovs_assert(od->nbr);
> @@ -12714,22 +12688,16 @@ build_static_route_flows_for_lrouter(
>
>      struct hmap ecmp_groups = HMAP_INITIALIZER(&ecmp_groups);
>      struct hmap unique_routes = HMAP_INITIALIZER(&unique_routes);
> -    struct ovs_list parsed_routes = OVS_LIST_INITIALIZER(&parsed_routes);
> -    struct simap route_tables = SIMAP_INITIALIZER(&route_tables);
>      struct ecmp_groups_node *group;
>
>      for (int i = 0; i < od->nbr->n_ports; i++) {
>          build_route_table_lflow(od, lflows, od->nbr->ports[i],
> -                                &route_tables, lflow_ref);
> +                                route_tables, lflow_ref);
>      }
>
> -    for (int i = 0; i < od->nbr->n_static_routes; i++) {
> -        struct parsed_route *route =
> -            parsed_routes_add(od, lr_ports, &parsed_routes,
&route_tables,
> -                              od->nbr->static_routes[i],
bfd_connections);
> -        if (!route) {
> -            continue;
> -        }
> +    struct parsed_route *route;
> +    HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
> +                             parsed_routes) {
>          group = ecmp_groups_find(&ecmp_groups, route);
>          if (group) {
>              ecmp_groups_add_route(group, route);
> @@ -12758,8 +12726,6 @@ build_static_route_flows_for_lrouter(
>      }
>      ecmp_groups_destroy(&ecmp_groups);
>      unique_routes_destroy(&unique_routes);
> -    parsed_routes_destroy(&parsed_routes);
> -    simap_destroy(&route_tables);
>  }
>
>  /* IP Multicast lookup. Here we set the output port, adjust TTL and
> @@ -12866,6 +12832,47 @@ build_mcast_lookup_flows_for_lrouter(
>      }
>  }
>
> +void
> +build_route_policies(struct ovn_datapath *od, struct hmap *lr_ports,
> +                     struct hmap *bfd_connections, struct hmap
*route_policies)
> +{
> +    for (int i = 0; i < od->nbr->n_policies; i++) {
> +        const struct nbrec_logical_router_policy *rule =
od->nbr->policies[i];
> +        size_t n_valid_nexthops = 0;
> +        char **valid_nexthops = NULL;
> +
> +        if (!strcmp(rule->action, "reroute")) {
> +            size_t n_nexthops = rule->n_nexthops ? rule->n_nexthops : 1;
> +
> +            valid_nexthops = xcalloc(n_nexthops, sizeof *valid_nexthops);
> +            for (size_t j = 0; j < n_nexthops; j++) {
> +                char *nexthop = rule->n_nexthops
> +                    ? rule->nexthops[j] : rule->nexthop;
> +                struct ovn_port *out_port =
> +                    get_outport_for_routing_policy_nexthop(
> +                            od, lr_ports, rule->priority, nexthop);
> +                if (!out_port ||
> +                    !check_bfd_state(rule, bfd_connections, out_port,
> +                                     nexthop)) {
> +                    continue;
> +                }
> +                valid_nexthops[n_valid_nexthops++] = nexthop;
> +            }
> +
> +            if (!n_valid_nexthops) {
> +                free(valid_nexthops);
> +                continue;
> +            }
> +        }
> +
> +        struct route_policy *rp = xzalloc(sizeof *rp);
> +        rp->rule = rule;
> +        rp->n_valid_nexthops = n_valid_nexthops;
> +        rp->valid_nexthops = valid_nexthops;
> +        hmap_insert(route_policies, &rp->key_node, uuid_hash(&od->key));
> +    }
> +}
> +
>  /* Logical router ingress table POLICY: Policy.
>   *
>   * A packet that arrives at this table is an IP packet that should be
> @@ -12879,7 +12886,7 @@ static void
>  build_ingress_policy_flows_for_lrouter(
>          struct ovn_datapath *od, struct lflow_table *lflows,
>          const struct hmap *lr_ports,
> -        const struct hmap *bfd_connections,
> +        struct hmap *route_policies,
>          struct lflow_ref *lflow_ref)
>  {
>      ovs_assert(od->nbr);
> @@ -12896,21 +12903,20 @@ build_ingress_policy_flows_for_lrouter(
>
>      /* Convert routing policies to flows. */
>      uint16_t ecmp_group_id = 1;
> -    for (int i = 0; i < od->nbr->n_policies; i++) {
> -        const struct nbrec_logical_router_policy *rule
> -            = od->nbr->policies[i];
> +    struct route_policy *rp;
> +    HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
> +                             route_policies) {
> +        const struct nbrec_logical_router_policy *rule = rp->rule;
>          bool is_ecmp_reroute =
>              (!strcmp(rule->action, "reroute") && rule->n_nexthops > 1);
>
>          if (is_ecmp_reroute) {
> -            build_ecmp_routing_policy_flows(lflows, od, lr_ports, rule,
> -                                            bfd_connections,
ecmp_group_id,
> -                                            lflow_ref);
> +            build_ecmp_routing_policy_flows(lflows, od, lr_ports, rp,
> +                                            ecmp_group_id, lflow_ref);
>              ecmp_group_id++;
>          } else {
> -            build_routing_policy_flow(lflows, od, lr_ports, rule,
> -                                      bfd_connections, &rule->header_,
> -                                      lflow_ref);
> +            build_routing_policy_flow(lflows, od, lr_ports, rp,
> +                                      &rule->header_, lflow_ref);
>          }
>      }
>  }
> @@ -15864,6 +15870,9 @@ struct lswitch_flow_build_info {
>      struct ds actions;
>      size_t thread_lflow_counter;
>      const char *svc_monitor_mac;
> +    struct hmap *parsed_routes;
> +    struct hmap *route_policies;
> +    struct simap *route_tables;
>  };
>
>  /* Helper function to combine all lflow generation which is iterated by
> @@ -15910,12 +15919,13 @@ build_lswitch_and_lrouter_iterate_by_lr(struct
ovn_datapath *od,
>      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
>      build_static_route_flows_for_lrouter(od, lsi->features,
>                                           lsi->lflows, lsi->lr_ports,
> -                                         lsi->bfd_connections,
> +                                         lsi->parsed_routes,
> +                                         lsi->route_tables,
>                                           NULL);
>      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
>                                           &lsi->actions, NULL);
>      build_ingress_policy_flows_for_lrouter(od, lsi->lflows,
lsi->lr_ports,
> -                                           lsi->bfd_connections, NULL);
> +                                           lsi->route_policies, NULL);
>      build_arp_resolve_flows_for_lrouter(od, lsi->lflows, NULL);
>      build_check_pkt_len_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports,
>                                            &lsi->match, &lsi->actions,
> @@ -16231,7 +16241,10 @@ build_lswitch_and_lrouter_flows(
>      const struct hmap *svc_monitor_map,
>      const struct hmap *bfd_connections,
>      const struct chassis_features *features,
> -    const char *svc_monitor_mac)
> +    const char *svc_monitor_mac,
> +    struct hmap *parsed_routes,
> +    struct hmap *route_policies,
> +    struct simap *route_tables)
>  {
>
>      char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
> @@ -16265,6 +16278,9 @@ build_lswitch_and_lrouter_flows(
>              lsiv[index].svc_check_match = svc_check_match;
>              lsiv[index].thread_lflow_counter = 0;
>              lsiv[index].svc_monitor_mac = svc_monitor_mac;
> +            lsiv[index].parsed_routes = parsed_routes;
> +            lsiv[index].route_tables = route_tables;
> +            lsiv[index].route_policies = route_policies;
>              ds_init(&lsiv[index].match);
>              ds_init(&lsiv[index].actions);
>
> @@ -16305,6 +16321,9 @@ build_lswitch_and_lrouter_flows(
>              .features = features,
>              .svc_check_match = svc_check_match,
>              .svc_monitor_mac = svc_monitor_mac,
> +            .parsed_routes = parsed_routes,
> +            .route_tables = route_tables,
> +            .route_policies = route_policies,
>              .match = DS_EMPTY_INITIALIZER,
>              .actions = DS_EMPTY_INITIALIZER,
>          };
> @@ -16466,7 +16485,10 @@ void build_lflows(struct ovsdb_idl_txn
*ovnsb_txn,
>                                      input_data->svc_monitor_map,
>                                      input_data->bfd_connections,
>                                      input_data->features,
> -                                    input_data->svc_monitor_mac);
> +                                    input_data->svc_monitor_mac,
> +                                    input_data->parsed_routes,
> +                                    input_data->route_policies,
> +                                    input_data->route_tables);
>
>      if (parallelization_state == STATE_INIT_HASH_SIZES) {
>          parallelization_state = STATE_USE_PARALLELIZATION;
> @@ -17542,6 +17564,20 @@ northd_init(struct northd_data *data)
>      init_northd_tracked_data(data);
>  }
>
> +void
> +bfd_consumer_init(struct bfd_consumer_data *data)
> +{
> +    hmap_init(&data->parsed_routes);
> +    hmap_init(&data->route_policies);
> +    simap_init(&data->route_tables);
> +}
> +
> +void
> +bfd_init(struct bfd_data *data)
> +{
> +    hmap_init(&data->bfd_connections);
> +}
> +
>  void
>  northd_destroy(struct northd_data *data)
>  {
> @@ -17581,6 +17617,36 @@ northd_destroy(struct northd_data *data)
>      destroy_northd_tracked_data(data);
>  }
>
> +void
> +bfd_consumer_destroy(struct bfd_consumer_data *data)
> +{
> +    struct parsed_route *r;
> +    HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) {
> +        free(r);
> +    }
> +    hmap_destroy(&data->parsed_routes);
> +
> +    struct route_policy *rp;
> +    HMAP_FOR_EACH_POP (rp, key_node, &data->route_policies) {
> +        free(rp->valid_nexthops);
> +        free(rp);
> +    };
> +    hmap_destroy(&data->route_policies);
> +
> +    simap_destroy(&data->route_tables);
> +}
> +
> +void
> +bfd_destroy(struct bfd_data *data)
> +{
> +    struct bfd_entry *bfd_e;
> +
> +    HMAP_FOR_EACH_POP (bfd_e, hmap_node, &data->bfd_connections) {
> +        free(bfd_e);
> +    }
> +    hmap_destroy(&data->bfd_connections);
> +}
> +
>  void
>  ovnnb_db_run(struct northd_input *input_data,
>               struct northd_data *data,
> diff --git a/northd/northd.h b/northd/northd.h
> index 940926945..8c66283a5 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -23,6 +23,7 @@
>  #include "northd/en-port-group.h"
>  #include "northd/ipam.h"
>  #include "openvswitch/hmap.h"
> +#include "simap.h"
>  #include "ovs-thread.h"
>
>  struct northd_input {
> @@ -160,6 +161,23 @@ struct northd_data {
>      struct northd_tracked_data trk_data;
>  };
>
> +struct route_policy {
> +    struct hmap_node key_node;
> +    const struct nbrec_logical_router_policy *rule;
> +    size_t n_valid_nexthops;
> +    char **valid_nexthops;
> +};
> +
> +struct bfd_consumer_data {
> +    struct hmap parsed_routes;
> +    struct hmap route_policies;
> +    struct simap route_tables;
> +};
> +
> +struct bfd_data {
> +    struct hmap bfd_connections;
> +};
> +
>  struct lr_nat_table;
>
>  struct lflow_input {
> @@ -190,6 +208,9 @@ struct lflow_input {
>      const struct hmap *svc_monitor_map;
>      bool ovn_internal_version_changed;
>      const char *svc_monitor_mac;
> +    struct hmap *parsed_routes;
> +    struct hmap *route_policies;
> +    struct simap *route_tables;
>  };
>
>  extern int parallelization_state;
> @@ -661,6 +682,18 @@ struct ovn_port {
>      struct lflow_ref *stateful_lflow_ref;
>  };
>
> +struct parsed_route {
> +    struct hmap_node key_node;
> +    struct in6_addr prefix;
> +    unsigned int plen;
> +    bool is_src_route;
> +    uint32_t route_table_id;
> +    uint32_t hash;
> +    const struct nbrec_logical_router_static_route *route;
> +    bool ecmp_symmetric_reply;
> +    bool is_discard_route;
> +};
> +
>  void ovnnb_db_run(struct northd_input *input_data,
>                    struct northd_data *data,
>                    struct ovsdb_idl_txn *ovnnb_txn,
> @@ -682,6 +715,17 @@ void northd_init(struct northd_data *data);
>  void northd_indices_create(struct northd_data *data,
>                             struct ovsdb_idl *ovnsb_idl);
>
> +struct parsed_route *parsed_routes_add(struct ovn_datapath *,
> +        const struct hmap *, struct hmap *, struct simap *,
> +        const struct nbrec_logical_router_static_route *,
> +        const struct hmap *);
> +uint32_t get_route_table_id(struct simap *, const char *);
> +void bfd_consumer_init(struct bfd_consumer_data *);
> +void bfd_consumer_destroy(struct bfd_consumer_data *);
> +
> +void bfd_init(struct bfd_data *);
> +void bfd_destroy(struct bfd_data *);
> +
>  struct lflow_table;
>  struct lr_stateful_tracked_data;
>  struct ls_stateful_tracked_data;
> @@ -719,7 +763,9 @@ bool northd_handle_lb_data_changes(struct
tracked_lb_data *,
>                                     struct hmap *lbgrp_datapaths_map,
>                                     struct northd_tracked_data *);
>
> -void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> +void build_route_policies(struct ovn_datapath *, struct hmap *, struct
hmap *,
> +                          struct hmap *);
> +bool build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>                       const struct nbrec_bfd_table *,
>                       const struct sbrec_bfd_table *,
>                       const struct hmap *lr_ports,
> --
> 2.44.0
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

> On Thu, May 2, 2024 at 8:22 AM Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> wrote:
> >
> > Introduce bfd and bfd_consumer nodes to northd I-P engine to track bfd
> > connections and northd static_route/policy_route changes.
> >
> > Reported-at: https://issues.redhat.com/browse/FDP-600
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> 
> Hi Lorenzo,

Hi Han,

Thx for the review.

> 
> Thanks for the patch. Could you explain in more detail about the motivation
> and background information of this patch? I followed the link in
> Reported-at but still not clear.
> I assume it is for performance reasons, so could you explain what scenarios
> would benefit from this?

The main motivation for this patch is to address a request from Dumitru [0] 
for FDP-56 [1]. In particular, in the new ECMP_nexthop monitor [2], we need to
recalculate the static routes map, so he requested to compute this hashmap in
advance and reuse it in the lflow codebase and in the new ECMP_nexthop monitor
codebase. Since we will need even the BFD info there, I moved the code out of
lflow generation codebase and introduced two dedicated nodes (bfd and
bfd-consumer) in the Northd IP engine.

@Dumitru: is my explanation correct?

> 
> For the patch, I haven't looked into details yet, but it seems you didn't
> add any change handler for the two new nodes. So will there be follow up
> patches for the real performance improvement?

for BFD node the logic is already in en_bfd_run() since we do not always
return EN_UPDATED but if there are no valid changes, we will return
EN_UNCHANGED. Am I missing something?
For bfd-consumer node I think we actually do not need it. Am I wrong?

> But even for the current patch, with this: engine_add_input(&en_bfd,
> &en_northd, NULL); does it mean any en_northd change would trigger a lflow
> recompute because of the dependency en_northd -> en_bfd -> en_lflow. Would
> this be a performance regression? If not, why?

I guess no, otherwise the IP ovn tests will fail, right?

> 
> For the noop handler used, please provide a detailed explanation to justify
> it: why the node depends on the input but doesn't need to process when the
> input changes?

I think for the bfd-consumer node we have just an order dependency with northd
one, right? I will review this part.

Regards,
Lorenzo

> 
> Thanks,
> Han

[0] https://patchwork.ozlabs.org/project/ovn/patch/f31fb6630039c9b310b7fa489247d354f8a46719.1710257650.git.lorenzo.bianconi@redhat.com/
[1] https://issues.redhat.com/browse/FDP-56
[2] https://patchwork.ozlabs.org/project/ovn/cover/cover.1710257649.git.lorenzo.bianconi@redhat.com/

> 
> > ---
> >  northd/en-lflow.c        |  19 +--
> >  northd/en-northd.c       |  92 +++++++++++++
> >  northd/en-northd.h       |   8 ++
> >  northd/inc-proc-northd.c |  21 ++-
> >  northd/northd.c          | 274 ++++++++++++++++++++++++---------------
> >  northd/northd.h          |  48 ++++++-
> >  6 files changed, 344 insertions(+), 118 deletions(-)
> >
> > diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> > index c4b927fb8..9efbd5117 100644
> > --- a/northd/en-lflow.c
> > +++ b/northd/en-lflow.c
> > @@ -41,6 +41,9 @@ lflow_get_input_data(struct engine_node *node,
> >                       struct lflow_input *lflow_input)
> >  {
> >      struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > +    struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> > +    struct bfd_consumer_data *bfd_consumer_data =
> > +        engine_get_input_data("bfd_consumer", node);
> >      struct port_group_data *pg_data =
> >          engine_get_input_data("port_group", node);
> >      struct sync_meters_data *sync_meters_data =
> > @@ -78,7 +81,10 @@ lflow_get_input_data(struct engine_node *node,
> >      lflow_input->meter_groups = &sync_meters_data->meter_groups;
> >      lflow_input->lb_datapaths_map = &northd_data->lb_datapaths_map;
> >      lflow_input->svc_monitor_map = &northd_data->svc_monitor_map;
> > -    lflow_input->bfd_connections = NULL;
> > +    lflow_input->bfd_connections = &bfd_data->bfd_connections;
> > +    lflow_input->parsed_routes = &bfd_consumer_data->parsed_routes;
> > +    lflow_input->route_tables = &bfd_consumer_data->route_tables;
> > +    lflow_input->route_policies = &bfd_consumer_data->route_policies;
> >
> >      struct ed_type_global_config *global_config =
> >          engine_get_input_data("global_config", node);
> > @@ -95,25 +101,14 @@ void en_lflow_run(struct engine_node *node, void
> *data)
> >      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;
> > -
> >      stopwatch_start(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
> >
> >      struct lflow_data *lflow_data = data;
> >      lflow_table_clear(lflow_data->lflow_table);
> >      lflow_reset_northd_refs(&lflow_input);
> >
> > -    build_bfd_table(eng_ctx->ovnsb_idl_txn,
> > -                    lflow_input.nbrec_bfd_table,
> > -                    lflow_input.sbrec_bfd_table,
> > -                    lflow_input.lr_ports,
> > -                    &bfd_connections);
> >      build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input,
> >                   lflow_data->lflow_table);
> > -    bfd_cleanup_connections(lflow_input.nbrec_bfd_table,
> > -                            &bfd_connections);
> > -    hmap_destroy(&bfd_connections);
> >      stopwatch_stop(BUILD_LFLOWS_STOPWATCH_NAME, time_msec());
> >
> >      engine_set_node_state(node, EN_UPDATED);
> > diff --git a/northd/en-northd.c b/northd/en-northd.c
> > index 4479b4aff..2d8c05608 100644
> > --- a/northd/en-northd.c
> > +++ b/northd/en-northd.c
> > @@ -236,6 +236,66 @@ northd_global_config_handler(struct engine_node
> *node, void *data OVS_UNUSED)
> >      return true;
> >  }
> >
> > +void
> > +en_bfd_consumer_run(struct engine_node *node, void *data)
> > +{
> > +
> > +    struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > +    struct bfd_data *bfd_data = engine_get_input_data("bfd", node);
> > +    const struct nbrec_bfd_table *nbrec_bfd_table =
> > +        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> > +    struct bfd_consumer_data *bfd_consumer_data = data;
> > +
> > +    bfd_consumer_destroy(data);
> > +    bfd_consumer_init(data);
> > +
> > +    struct ovn_datapath *od;
> > +    HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) {
> > +        for (int i = 0; i < od->nbr->n_ports; i++) {
> > +            const char *route_table_name =
> > +                smap_get(&od->nbr->ports[i]->options, "route_table");
> > +            get_route_table_id(&bfd_consumer_data->route_tables,
> > +                               route_table_name);
> > +        }
> > +        for (int i = 0; i < od->nbr->n_static_routes; i++) {
> > +                parsed_routes_add(od, &northd_data->lr_ports,
> > +                                  &bfd_consumer_data->parsed_routes,
> > +                                  &bfd_consumer_data->route_tables,
> > +                                  od->nbr->static_routes[i],
> > +                                  &bfd_data->bfd_connections);
> > +        }
> > +        build_route_policies(od, &northd_data->lr_ports,
> > +                             &bfd_data->bfd_connections,
> > +                             &bfd_consumer_data->route_policies);
> > +    }
> > +    bfd_cleanup_connections(nbrec_bfd_table, &bfd_data->bfd_connections);
> > +
> > +    engine_set_node_state(node, EN_UPDATED);
> > +}
> > +
> > +void
> > +en_bfd_run(struct engine_node *node, void *data)
> > +{
> > +    struct northd_data *northd_data = engine_get_input_data("northd",
> node);
> > +    const struct engine_context *eng_ctx = engine_get_context();
> > +    struct bfd_data *bfd_data = data;
> > +    const struct nbrec_bfd_table *nbrec_bfd_table =
> > +        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> > +    const struct sbrec_bfd_table *sbrec_bfd_table =
> > +        EN_OVSDB_GET(engine_get_input("SB_bfd", node));
> > +
> > +    bfd_destroy(data);
> > +    bfd_init(data);
> > +    if (build_bfd_table(eng_ctx->ovnsb_idl_txn,
> > +                        nbrec_bfd_table, sbrec_bfd_table,
> > +                        &northd_data->lr_ports,
> > +                        &bfd_data->bfd_connections)) {
> > +        engine_set_node_state(node, EN_UPDATED);
> > +    } else {
> > +        engine_set_node_state(node, EN_UNCHANGED);
> > +    }
> > +}
> > +
> >  void
> >  *en_northd_init(struct engine_node *node OVS_UNUSED,
> >                  struct engine_arg *arg OVS_UNUSED)
> > @@ -247,6 +307,26 @@ void
> >      return data;
> >  }
> >
> > +void
> > +*en_bfd_consumer_init(struct engine_node *node OVS_UNUSED,
> > +                      struct engine_arg *arg OVS_UNUSED)
> > +{
> > +    struct bfd_consumer_data *data = xzalloc(sizeof *data);
> > +
> > +    bfd_consumer_init(data);
> > +    return data;
> > +}
> > +
> > +void
> > +*en_bfd_init(struct engine_node *node OVS_UNUSED,
> > +             struct engine_arg *arg OVS_UNUSED)
> > +{
> > +    struct bfd_data *data = xzalloc(sizeof *data);
> > +
> > +    bfd_init(data);
> > +    return data;
> > +}
> > +
> >  void
> >  en_northd_cleanup(void *data)
> >  {
> > @@ -259,3 +339,15 @@ en_northd_clear_tracked_data(void *data_)
> >      struct northd_data *data = data_;
> >      destroy_northd_data_tracked_changes(data);
> >  }
> > +
> > +void
> > +en_bfd_consumer_cleanup(void *data)
> > +{
> > +    bfd_consumer_destroy(data);
> > +}
> > +
> > +void
> > +en_bfd_cleanup(void *data)
> > +{
> > +    bfd_destroy(data);
> > +}
> > diff --git a/northd/en-northd.h b/northd/en-northd.h
> > index 9b7bda32a..55b9dd83c 100644
> > --- a/northd/en-northd.h
> > +++ b/northd/en-northd.h
> > @@ -19,5 +19,13 @@ bool northd_nb_logical_switch_handler(struct
> engine_node *, void *data);
> >  bool northd_nb_logical_router_handler(struct engine_node *, void *data);
> >  bool northd_sb_port_binding_handler(struct engine_node *, void *data);
> >  bool northd_lb_data_handler(struct engine_node *, void *data);
> > +void *en_bfd_consumer_init(struct engine_node *node OVS_UNUSED,
> > +                           struct engine_arg *arg OVS_UNUSED);
> > +void en_bfd_consumer_cleanup(void *data);
> > +void en_bfd_consumer_run(struct engine_node *node, void *data);
> > +void *en_bfd_init(struct engine_node *node OVS_UNUSED,
> > +                  struct engine_arg *arg OVS_UNUSED);
> > +void en_bfd_cleanup(void *data);
> > +void en_bfd_run(struct engine_node *node, void *data);
> >
> >  #endif /* EN_NORTHD_H */
> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> > index e1073812c..9491c974c 100644
> > --- a/northd/inc-proc-northd.c
> > +++ b/northd/inc-proc-northd.c
> > @@ -61,7 +61,9 @@ static unixctl_cb_func chassis_features_list;
> >      NB_NODE(meter, "meter") \
> >      NB_NODE(bfd, "bfd") \
> >      NB_NODE(static_mac_binding, "static_mac_binding") \
> > -    NB_NODE(chassis_template_var, "chassis_template_var")
> > +    NB_NODE(chassis_template_var, "chassis_template_var") \
> > +    NB_NODE(logical_router_static_route, "logical_router_static_route") \
> > +    NB_NODE(logical_router_policy, "logical_router_policy")
> >
> >      enum nb_engine_node {
> >  #define NB_NODE(NAME, NAME_STR) NB_##NAME,
> > @@ -155,6 +157,8 @@ static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lb_data,
> "lb_data");
> >  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_nat, "lr_nat");
> >  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(lr_stateful, "lr_stateful");
> >  static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(ls_stateful, "ls_stateful");
> > +static ENGINE_NODE(bfd_consumer, "bfd_consumer");
> > +static ENGINE_NODE(bfd, "bfd");
> >
> >  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >                            struct ovsdb_idl_loop *sb)
> > @@ -237,6 +241,19 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >      engine_add_input(&en_fdb_aging, &en_global_config,
> >                       node_global_config_handler);
> >
> > +    engine_add_input(&en_bfd, &en_nb_bfd, NULL);
> > +    engine_add_input(&en_bfd, &en_sb_bfd, NULL);
> > +    engine_add_input(&en_bfd, &en_northd, NULL);
> > +    engine_add_input(&en_bfd, &en_nb_logical_router_policy, NULL);
> > +    engine_add_input(&en_bfd, &en_nb_logical_router_static_route, NULL);
> > +
> > +    engine_add_input(&en_bfd_consumer, &en_bfd, NULL);
> > +    engine_add_input(&en_bfd_consumer, &en_northd, engine_noop_handler);
> > +    engine_add_input(&en_bfd_consumer, &en_nb_bfd, NULL);
> > +    engine_add_input(&en_bfd_consumer, &en_nb_logical_router_policy,
> NULL);
> > +    engine_add_input(&en_bfd_consumer,
> &en_nb_logical_router_static_route,
> > +                     NULL);
> > +
> >      engine_add_input(&en_sync_meters, &en_nb_acl, NULL);
> >      engine_add_input(&en_sync_meters, &en_nb_meter, NULL);
> >      engine_add_input(&en_sync_meters, &en_sb_meter, NULL);
> > @@ -255,6 +272,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> >      engine_add_input(&en_lflow, &en_port_group,
> lflow_port_group_handler);
> >      engine_add_input(&en_lflow, &en_lr_stateful,
> lflow_lr_stateful_handler);
> >      engine_add_input(&en_lflow, &en_ls_stateful,
> lflow_ls_stateful_handler);
> > +    engine_add_input(&en_lflow, &en_bfd_consumer, NULL);
> > +    engine_add_input(&en_lflow, &en_bfd, NULL);
> >
> >      engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL);
> >      engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, NULL);
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 5e12fd1e8..6f9aa3ff9 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -9780,47 +9780,53 @@ bfd_cleanup_connections(const struct
> nbrec_bfd_table *nbrec_bfd_table,
> >              nbrec_bfd_set_status(nb_bt, "admin_down");
> >          }
> >      }
> > -
> > -    HMAP_FOR_EACH_POP (bfd_e, hmap_node, bfd_map) {
> > -        free(bfd_e);
> > -    }
> >  }
> >
> >  #define BFD_DEF_MINTX       1000 /* 1s */
> >  #define BFD_DEF_MINRX       1000 /* 1s */
> >  #define BFD_DEF_DETECT_MULT 5
> >
> > -static void
> > +static bool
> >  build_bfd_update_sb_conf(const struct nbrec_bfd *nb_bt,
> >                           const struct sbrec_bfd *sb_bt)
> >  {
> > +    bool ret = false;
> > +
> >      if (strcmp(nb_bt->dst_ip, sb_bt->dst_ip)) {
> >          sbrec_bfd_set_dst_ip(sb_bt, nb_bt->dst_ip);
> > +        ret = true;
> >      }
> >
> >      if (strcmp(nb_bt->logical_port, sb_bt->logical_port)) {
> >          sbrec_bfd_set_logical_port(sb_bt, nb_bt->logical_port);
> > +        ret = true;
> >      }
> >
> >      if (strcmp(nb_bt->status, sb_bt->status)) {
> >          sbrec_bfd_set_status(sb_bt, nb_bt->status);
> > +        ret = true;
> >      }
> >
> >      int detect_mult = nb_bt->n_detect_mult ? nb_bt->detect_mult[0]
> >                                             : BFD_DEF_DETECT_MULT;
> >      if (detect_mult != sb_bt->detect_mult) {
> >          sbrec_bfd_set_detect_mult(sb_bt, detect_mult);
> > +        ret = true;
> >      }
> >
> >      int min_tx = nb_bt->n_min_tx ? nb_bt->min_tx[0] : BFD_DEF_MINTX;
> >      if (min_tx != sb_bt->min_tx) {
> >          sbrec_bfd_set_min_tx(sb_bt, min_tx);
> > +        ret = true;
> >      }
> >
> >      int min_rx = nb_bt->n_min_rx ? nb_bt->min_rx[0] : BFD_DEF_MINRX;
> >      if (min_rx != sb_bt->min_rx) {
> >          sbrec_bfd_set_min_rx(sb_bt, min_rx);
> > +        ret = true;
> >      }
> > +
> > +    return ret;
> >  }
> >
> >  /* RFC 5881 section 4
> > @@ -9846,7 +9852,7 @@ static int bfd_get_unused_port(unsigned long
> *bfd_src_ports)
> >      return port + BFD_UDP_SRC_PORT_START;
> >  }
> >
> > -void
> > +bool
> >  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,
> > @@ -9856,6 +9862,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >      const struct sbrec_bfd *sb_bt;
> >      unsigned long *bfd_src_ports;
> >      struct bfd_entry *bfd_e;
> > +    bool ret = false;
> >      uint32_t hash;
> >
> >      bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN);
> > @@ -9901,6 +9908,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >              int d_mult = nb_bt->n_detect_mult ? nb_bt->detect_mult[0]
> >                                                : BFD_DEF_DETECT_MULT;
> >              sbrec_bfd_set_detect_mult(sb_bt, d_mult);
> > +            ret = true;
> >          } else {
> >              if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
> >                  if (!strcmp(nb_bt->status, "admin_down") ||
> > @@ -9909,12 +9917,16 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >                  } else {
> >                      nbrec_bfd_set_status(nb_bt, bfd_e->sb_bt->status);
> >                  }
> > +                ret = true;
> > +            }
> > +            if (build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt)) {
> > +                ret = true;
> >              }
> > -            build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
> >              if (op && op->sb && op->sb->chassis &&
> >                  strcmp(op->sb->chassis->name,
> bfd_e->sb_bt->chassis_name)) {
> >                  sbrec_bfd_set_chassis_name(bfd_e->sb_bt,
> >                                             op->sb->chassis->name);
> > +                ret = true;
> >              }
> >
> >              hmap_remove(&sb_only, &bfd_e->hmap_node);
> > @@ -9937,10 +9949,13 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >          }
> >          sbrec_bfd_delete(bfd_e->sb_bt);
> >          free(bfd_e);
> > +        ret = true;
> >      }
> >      hmap_destroy(&sb_only);
> >
> >      bitmap_free(bfd_src_ports);
> > +
> > +    return ret;
> >  }
> >
> >  /* Returns a string of the IP address of the router port 'op' that
> > @@ -10032,20 +10047,22 @@ static bool check_bfd_state(
> >
> >  static void
> >  build_routing_policy_flow(struct lflow_table *lflows, struct
> ovn_datapath *od,
> > -                          const struct hmap *lr_ports,
> > -                          const struct nbrec_logical_router_policy *rule,
> > -                          const struct hmap *bfd_connections,
> > +                          const struct hmap *lr_ports, struct
> route_policy *rp,
> >                            const struct ovsdb_idl_row *stage_hint,
> >                            struct lflow_ref *lflow_ref)
> >  {
> > +    const struct nbrec_logical_router_policy *rule = rp->rule;
> >      struct ds match = DS_EMPTY_INITIALIZER;
> >      struct ds actions = DS_EMPTY_INITIALIZER;
> >
> >      if (!strcmp(rule->action, "reroute")) {
> >          ovs_assert(rule->n_nexthops <= 1);
> >
> > -        char *nexthop =
> > -            (rule->n_nexthops == 1 ? rule->nexthops[0] : rule->nexthop);
> > +        if (!rp->n_valid_nexthops) {
> > +            return;
> > +        }
> > +
> > +        char *nexthop = rp->valid_nexthops[0];
> >          struct ovn_port *out_port =
> get_outport_for_routing_policy_nexthop(
> >               od, lr_ports, rule->priority, nexthop);
> >          if (!out_port) {
> > @@ -10061,10 +10078,6 @@ build_routing_policy_flow(struct lflow_table
> *lflows, struct ovn_datapath *od,
> >              return;
> >          }
> >
> > -        if (!check_bfd_state(rule, bfd_connections, out_port, nexthop)) {
> > -            return;
> > -        }
> > -
> >          uint32_t pkt_mark = smap_get_uint(&rule->options, "pkt_mark", 0);
> >          if (pkt_mark) {
> >              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
> > @@ -10107,19 +10120,18 @@ static void
> >  build_ecmp_routing_policy_flows(struct lflow_table *lflows,
> >                                  struct ovn_datapath *od,
> >                                  const struct hmap *lr_ports,
> > -                                const struct nbrec_logical_router_policy
> *rule,
> > -                                const struct hmap *bfd_connections,
> > +                                struct route_policy *rp,
> >                                  uint16_t ecmp_group_id,
> >                                  struct lflow_ref *lflow_ref)
> >  {
> > -    ovs_assert(rule->n_nexthops > 1);
> > -
> >      bool nexthops_is_ipv4 = true;
> > +    const struct nbrec_logical_router_policy *rule = rp->rule;
> > +    ovs_assert(rule->n_nexthops > 1);
> >
> >      /* Check that all the nexthops belong to the same addr family before
> >       * adding logical flows. */
> > -    for (uint16_t i = 0; i < rule->n_nexthops; i++) {
> > -        bool is_ipv4 = strchr(rule->nexthops[i], '.') ? true : false;
> > +    for (uint16_t i = 0; i < rp->n_valid_nexthops; i++) {
> > +        bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false;
> >
> >          if (i == 0) {
> >              nexthops_is_ipv4 = is_ipv4;
> > @@ -10130,7 +10142,7 @@ build_ecmp_routing_policy_flows(struct
> lflow_table *lflows,
> >              VLOG_WARN_RL(&rl, "nexthop [%s] of the router policy with "
> >                           "the match [%s] do not belong to the same
> address "
> >                           "family as other next hops",
> > -                         rule->nexthops[i], rule->match);
> > +                         rp->valid_nexthops[i], rule->match);
> >              return;
> >          }
> >      }
> > @@ -10138,40 +10150,30 @@ build_ecmp_routing_policy_flows(struct
> lflow_table *lflows,
> >      struct ds match = DS_EMPTY_INITIALIZER;
> >      struct ds actions = DS_EMPTY_INITIALIZER;
> >
> > -    size_t *valid_nexthops = xcalloc(rule->n_nexthops, sizeof
> *valid_nexthops);
> > -    size_t n_valid_nexthops = 0;
> > -
> > -    for (size_t i = 0; i < rule->n_nexthops; i++) {
> > +    for (size_t i = 0; i < rp->n_valid_nexthops; i++) {
> >          struct ovn_port *out_port =
> get_outport_for_routing_policy_nexthop(
> > -             od, lr_ports, rule->priority, rule->nexthops[i]);
> > +             od, lr_ports, rule->priority, rp->valid_nexthops[i]);
> >          if (!out_port) {
> >              goto cleanup;
> >          }
> >
> >          const char *lrp_addr_s =
> > -            find_lrp_member_ip(out_port, rule->nexthops[i]);
> > +            find_lrp_member_ip(out_port, rp->valid_nexthops[i]);
> >          if (!lrp_addr_s) {
> >              static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5,
> 1);
> >              VLOG_WARN_RL(&rl, "lrp_addr not found for routing policy "
> >                              " priority %"PRId64" nexthop %s",
> > -                            rule->priority, rule->nexthops[i]);
> > +                            rule->priority, rp->valid_nexthops[i]);
> >              goto cleanup;
> >          }
> >
> > -        if (!check_bfd_state(rule, bfd_connections, out_port,
> > -                             rule->nexthops[i])) {
> > -            continue;
> > -        }
> > -
> > -        valid_nexthops[n_valid_nexthops++] = i + 1;
> > -
> >          ds_clear(&actions);
> >          uint32_t pkt_mark = smap_get_uint(&rule->options, "pkt_mark", 0);
> >          if (pkt_mark) {
> >              ds_put_format(&actions, "pkt.mark = %u; ", pkt_mark);
> >          }
> >
> > -        bool is_ipv4 = strchr(rule->nexthops[i], '.') ? true : false;
> > +        bool is_ipv4 = strchr(rp->valid_nexthops[i], '.') ? true : false;
> >
> >          ds_put_format(&actions, "%s = %s; "
> >                        "%s = %s; "
> > @@ -10180,7 +10182,7 @@ build_ecmp_routing_policy_flows(struct
> lflow_table *lflows,
> >                        "flags.loopback = 1; "
> >                        "next;",
> >                        is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> > -                      rule->nexthops[i],
> > +                      rp->valid_nexthops[i],
> >                        is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
> >                        lrp_addr_s,
> >                        out_port->lrp_networks.ea_s,
> > @@ -10196,37 +10198,30 @@ build_ecmp_routing_policy_flows(struct
> lflow_table *lflows,
> >                                  lflow_ref);
> >      }
> >
> > -    if (!n_valid_nexthops) {
> > -        goto cleanup;
> > -    }
> > -
> >      ds_clear(&actions);
> > -    if (n_valid_nexthops > 1) {
> > +    if (rp->n_valid_nexthops > 1) {
> >          ds_put_format(&actions, "%s = %"PRIu16
> >                        "; %s = select(", REG_ECMP_GROUP_ID, ecmp_group_id,
> >                        REG_ECMP_MEMBER_ID);
> >
> > -        for (size_t i = 0; i < n_valid_nexthops; i++) {
> > +        for (size_t i = 0; i < rp->n_valid_nexthops; i++) {
> >              if (i > 0) {
> >                  ds_put_cstr(&actions, ", ");
> >              }
> >
> > -            ds_put_format(&actions, "%"PRIuSIZE, valid_nexthops[i]);
> > +            ds_put_format(&actions, "%"PRIuSIZE, i + 1);
> >          }
> >          ds_put_cstr(&actions, ");");
> >      } else {
> >          ds_put_format(&actions, "%s = %"PRIu16
> > -                      "; %s = %"PRIuSIZE"; next;", REG_ECMP_GROUP_ID,
> > -                      ecmp_group_id, REG_ECMP_MEMBER_ID,
> > -                      valid_nexthops[0]);
> > +                      "; %s = 1; next;", REG_ECMP_GROUP_ID,
> > +                      ecmp_group_id, REG_ECMP_MEMBER_ID);
> >      }
> >      ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_POLICY,
> >                              rule->priority, rule->match,
> >                              ds_cstr(&actions), &rule->header_,
> >                              lflow_ref);
> > -
> >  cleanup:
> > -    free(valid_nexthops);
> >      ds_destroy(&match);
> >      ds_destroy(&actions);
> >  }
> > @@ -10251,7 +10246,7 @@ route_table_add(struct simap *route_tables, const
> char *route_table_name)
> >      return rtb_id;
> >  }
> >
> > -static uint32_t
> > +uint32_t
> >  get_route_table_id(struct simap *route_tables, const char
> *route_table_name)
> >  {
> >      if (!route_table_name || !route_table_name[0]) {
> > @@ -10292,18 +10287,6 @@ build_route_table_lflow(struct ovn_datapath *od,
> struct lflow_table *lflows,
> >      ds_destroy(&actions);
> >  }
> >
> > -struct parsed_route {
> > -    struct ovs_list list_node;
> > -    struct in6_addr prefix;
> > -    unsigned int plen;
> > -    bool is_src_route;
> > -    uint32_t route_table_id;
> > -    uint32_t hash;
> > -    const struct nbrec_logical_router_static_route *route;
> > -    bool ecmp_symmetric_reply;
> > -    bool is_discard_route;
> > -};
> > -
> >  static uint32_t
> >  route_hash(struct parsed_route *route)
> >  {
> > @@ -10318,9 +10301,9 @@ find_static_route_outport(struct ovn_datapath
> *od, const struct hmap *lr_ports,
> >
> >  /* Parse and validate the route. Return the parsed route if successful.
> >   * Otherwise return NULL. */
> > -static struct parsed_route *
> > +struct parsed_route *
> >  parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> > -                  struct ovs_list *routes, struct simap *route_tables,
> > +                  struct hmap *routes, struct simap *route_tables,
> >                    const struct nbrec_logical_router_static_route *route,
> >                    const struct hmap *bfd_connections)
> >  {
> > @@ -10410,20 +10393,10 @@ parsed_routes_add(struct ovn_datapath *od,
> const struct hmap *lr_ports,
> >      pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> >                                               "ecmp_symmetric_reply",
> false);
> >      pr->is_discard_route = is_discard_route;
> > -    ovs_list_insert(routes, &pr->list_node);
> > +    hmap_insert(routes, &pr->key_node, uuid_hash(&od->key));
> >      return pr;
> >  }
> >
> > -static void
> > -parsed_routes_destroy(struct ovs_list *routes)
> > -{
> > -    struct parsed_route *pr;
> > -    LIST_FOR_EACH_SAFE (pr, list_node, routes) {
> > -        ovs_list_remove(&pr->list_node);
> > -        free(pr);
> > -    }
> > -}
> > -
> >  struct ecmp_route_list_node {
> >      struct ovs_list list_node;
> >      uint16_t id; /* starts from 1 */
> > @@ -12700,7 +12673,8 @@ static void
> >  build_static_route_flows_for_lrouter(
> >          struct ovn_datapath *od, const struct chassis_features *features,
> >          struct lflow_table *lflows, const struct hmap *lr_ports,
> > -        const struct hmap *bfd_connections,
> > +        struct hmap *parsed_routes,
> > +        struct simap *route_tables,
> >          struct lflow_ref *lflow_ref)
> >  {
> >      ovs_assert(od->nbr);
> > @@ -12714,22 +12688,16 @@ build_static_route_flows_for_lrouter(
> >
> >      struct hmap ecmp_groups = HMAP_INITIALIZER(&ecmp_groups);
> >      struct hmap unique_routes = HMAP_INITIALIZER(&unique_routes);
> > -    struct ovs_list parsed_routes = OVS_LIST_INITIALIZER(&parsed_routes);
> > -    struct simap route_tables = SIMAP_INITIALIZER(&route_tables);
> >      struct ecmp_groups_node *group;
> >
> >      for (int i = 0; i < od->nbr->n_ports; i++) {
> >          build_route_table_lflow(od, lflows, od->nbr->ports[i],
> > -                                &route_tables, lflow_ref);
> > +                                route_tables, lflow_ref);
> >      }
> >
> > -    for (int i = 0; i < od->nbr->n_static_routes; i++) {
> > -        struct parsed_route *route =
> > -            parsed_routes_add(od, lr_ports, &parsed_routes,
> &route_tables,
> > -                              od->nbr->static_routes[i],
> bfd_connections);
> > -        if (!route) {
> > -            continue;
> > -        }
> > +    struct parsed_route *route;
> > +    HMAP_FOR_EACH_WITH_HASH (route, key_node, uuid_hash(&od->key),
> > +                             parsed_routes) {
> >          group = ecmp_groups_find(&ecmp_groups, route);
> >          if (group) {
> >              ecmp_groups_add_route(group, route);
> > @@ -12758,8 +12726,6 @@ build_static_route_flows_for_lrouter(
> >      }
> >      ecmp_groups_destroy(&ecmp_groups);
> >      unique_routes_destroy(&unique_routes);
> > -    parsed_routes_destroy(&parsed_routes);
> > -    simap_destroy(&route_tables);
> >  }
> >
> >  /* IP Multicast lookup. Here we set the output port, adjust TTL and
> > @@ -12866,6 +12832,47 @@ build_mcast_lookup_flows_for_lrouter(
> >      }
> >  }
> >
> > +void
> > +build_route_policies(struct ovn_datapath *od, struct hmap *lr_ports,
> > +                     struct hmap *bfd_connections, struct hmap
> *route_policies)
> > +{
> > +    for (int i = 0; i < od->nbr->n_policies; i++) {
> > +        const struct nbrec_logical_router_policy *rule =
> od->nbr->policies[i];
> > +        size_t n_valid_nexthops = 0;
> > +        char **valid_nexthops = NULL;
> > +
> > +        if (!strcmp(rule->action, "reroute")) {
> > +            size_t n_nexthops = rule->n_nexthops ? rule->n_nexthops : 1;
> > +
> > +            valid_nexthops = xcalloc(n_nexthops, sizeof *valid_nexthops);
> > +            for (size_t j = 0; j < n_nexthops; j++) {
> > +                char *nexthop = rule->n_nexthops
> > +                    ? rule->nexthops[j] : rule->nexthop;
> > +                struct ovn_port *out_port =
> > +                    get_outport_for_routing_policy_nexthop(
> > +                            od, lr_ports, rule->priority, nexthop);
> > +                if (!out_port ||
> > +                    !check_bfd_state(rule, bfd_connections, out_port,
> > +                                     nexthop)) {
> > +                    continue;
> > +                }
> > +                valid_nexthops[n_valid_nexthops++] = nexthop;
> > +            }
> > +
> > +            if (!n_valid_nexthops) {
> > +                free(valid_nexthops);
> > +                continue;
> > +            }
> > +        }
> > +
> > +        struct route_policy *rp = xzalloc(sizeof *rp);
> > +        rp->rule = rule;
> > +        rp->n_valid_nexthops = n_valid_nexthops;
> > +        rp->valid_nexthops = valid_nexthops;
> > +        hmap_insert(route_policies, &rp->key_node, uuid_hash(&od->key));
> > +    }
> > +}
> > +
> >  /* Logical router ingress table POLICY: Policy.
> >   *
> >   * A packet that arrives at this table is an IP packet that should be
> > @@ -12879,7 +12886,7 @@ static void
> >  build_ingress_policy_flows_for_lrouter(
> >          struct ovn_datapath *od, struct lflow_table *lflows,
> >          const struct hmap *lr_ports,
> > -        const struct hmap *bfd_connections,
> > +        struct hmap *route_policies,
> >          struct lflow_ref *lflow_ref)
> >  {
> >      ovs_assert(od->nbr);
> > @@ -12896,21 +12903,20 @@ build_ingress_policy_flows_for_lrouter(
> >
> >      /* Convert routing policies to flows. */
> >      uint16_t ecmp_group_id = 1;
> > -    for (int i = 0; i < od->nbr->n_policies; i++) {
> > -        const struct nbrec_logical_router_policy *rule
> > -            = od->nbr->policies[i];
> > +    struct route_policy *rp;
> > +    HMAP_FOR_EACH_WITH_HASH (rp, key_node, uuid_hash(&od->key),
> > +                             route_policies) {
> > +        const struct nbrec_logical_router_policy *rule = rp->rule;
> >          bool is_ecmp_reroute =
> >              (!strcmp(rule->action, "reroute") && rule->n_nexthops > 1);
> >
> >          if (is_ecmp_reroute) {
> > -            build_ecmp_routing_policy_flows(lflows, od, lr_ports, rule,
> > -                                            bfd_connections,
> ecmp_group_id,
> > -                                            lflow_ref);
> > +            build_ecmp_routing_policy_flows(lflows, od, lr_ports, rp,
> > +                                            ecmp_group_id, lflow_ref);
> >              ecmp_group_id++;
> >          } else {
> > -            build_routing_policy_flow(lflows, od, lr_ports, rule,
> > -                                      bfd_connections, &rule->header_,
> > -                                      lflow_ref);
> > +            build_routing_policy_flow(lflows, od, lr_ports, rp,
> > +                                      &rule->header_, lflow_ref);
> >          }
> >      }
> >  }
> > @@ -15864,6 +15870,9 @@ struct lswitch_flow_build_info {
> >      struct ds actions;
> >      size_t thread_lflow_counter;
> >      const char *svc_monitor_mac;
> > +    struct hmap *parsed_routes;
> > +    struct hmap *route_policies;
> > +    struct simap *route_tables;
> >  };
> >
> >  /* Helper function to combine all lflow generation which is iterated by
> > @@ -15910,12 +15919,13 @@ build_lswitch_and_lrouter_iterate_by_lr(struct
> ovn_datapath *od,
> >      build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> >      build_static_route_flows_for_lrouter(od, lsi->features,
> >                                           lsi->lflows, lsi->lr_ports,
> > -                                         lsi->bfd_connections,
> > +                                         lsi->parsed_routes,
> > +                                         lsi->route_tables,
> >                                           NULL);
> >      build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> >                                           &lsi->actions, NULL);
> >      build_ingress_policy_flows_for_lrouter(od, lsi->lflows,
> lsi->lr_ports,
> > -                                           lsi->bfd_connections, NULL);
> > +                                           lsi->route_policies, NULL);
> >      build_arp_resolve_flows_for_lrouter(od, lsi->lflows, NULL);
> >      build_check_pkt_len_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports,
> >                                            &lsi->match, &lsi->actions,
> > @@ -16231,7 +16241,10 @@ build_lswitch_and_lrouter_flows(
> >      const struct hmap *svc_monitor_map,
> >      const struct hmap *bfd_connections,
> >      const struct chassis_features *features,
> > -    const char *svc_monitor_mac)
> > +    const char *svc_monitor_mac,
> > +    struct hmap *parsed_routes,
> > +    struct hmap *route_policies,
> > +    struct simap *route_tables)
> >  {
> >
> >      char *svc_check_match = xasprintf("eth.dst == %s", svc_monitor_mac);
> > @@ -16265,6 +16278,9 @@ build_lswitch_and_lrouter_flows(
> >              lsiv[index].svc_check_match = svc_check_match;
> >              lsiv[index].thread_lflow_counter = 0;
> >              lsiv[index].svc_monitor_mac = svc_monitor_mac;
> > +            lsiv[index].parsed_routes = parsed_routes;
> > +            lsiv[index].route_tables = route_tables;
> > +            lsiv[index].route_policies = route_policies;
> >              ds_init(&lsiv[index].match);
> >              ds_init(&lsiv[index].actions);
> >
> > @@ -16305,6 +16321,9 @@ build_lswitch_and_lrouter_flows(
> >              .features = features,
> >              .svc_check_match = svc_check_match,
> >              .svc_monitor_mac = svc_monitor_mac,
> > +            .parsed_routes = parsed_routes,
> > +            .route_tables = route_tables,
> > +            .route_policies = route_policies,
> >              .match = DS_EMPTY_INITIALIZER,
> >              .actions = DS_EMPTY_INITIALIZER,
> >          };
> > @@ -16466,7 +16485,10 @@ void build_lflows(struct ovsdb_idl_txn
> *ovnsb_txn,
> >                                      input_data->svc_monitor_map,
> >                                      input_data->bfd_connections,
> >                                      input_data->features,
> > -                                    input_data->svc_monitor_mac);
> > +                                    input_data->svc_monitor_mac,
> > +                                    input_data->parsed_routes,
> > +                                    input_data->route_policies,
> > +                                    input_data->route_tables);
> >
> >      if (parallelization_state == STATE_INIT_HASH_SIZES) {
> >          parallelization_state = STATE_USE_PARALLELIZATION;
> > @@ -17542,6 +17564,20 @@ northd_init(struct northd_data *data)
> >      init_northd_tracked_data(data);
> >  }
> >
> > +void
> > +bfd_consumer_init(struct bfd_consumer_data *data)
> > +{
> > +    hmap_init(&data->parsed_routes);
> > +    hmap_init(&data->route_policies);
> > +    simap_init(&data->route_tables);
> > +}
> > +
> > +void
> > +bfd_init(struct bfd_data *data)
> > +{
> > +    hmap_init(&data->bfd_connections);
> > +}
> > +
> >  void
> >  northd_destroy(struct northd_data *data)
> >  {
> > @@ -17581,6 +17617,36 @@ northd_destroy(struct northd_data *data)
> >      destroy_northd_tracked_data(data);
> >  }
> >
> > +void
> > +bfd_consumer_destroy(struct bfd_consumer_data *data)
> > +{
> > +    struct parsed_route *r;
> > +    HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) {
> > +        free(r);
> > +    }
> > +    hmap_destroy(&data->parsed_routes);
> > +
> > +    struct route_policy *rp;
> > +    HMAP_FOR_EACH_POP (rp, key_node, &data->route_policies) {
> > +        free(rp->valid_nexthops);
> > +        free(rp);
> > +    };
> > +    hmap_destroy(&data->route_policies);
> > +
> > +    simap_destroy(&data->route_tables);
> > +}
> > +
> > +void
> > +bfd_destroy(struct bfd_data *data)
> > +{
> > +    struct bfd_entry *bfd_e;
> > +
> > +    HMAP_FOR_EACH_POP (bfd_e, hmap_node, &data->bfd_connections) {
> > +        free(bfd_e);
> > +    }
> > +    hmap_destroy(&data->bfd_connections);
> > +}
> > +
> >  void
> >  ovnnb_db_run(struct northd_input *input_data,
> >               struct northd_data *data,
> > diff --git a/northd/northd.h b/northd/northd.h
> > index 940926945..8c66283a5 100644
> > --- a/northd/northd.h
> > +++ b/northd/northd.h
> > @@ -23,6 +23,7 @@
> >  #include "northd/en-port-group.h"
> >  #include "northd/ipam.h"
> >  #include "openvswitch/hmap.h"
> > +#include "simap.h"
> >  #include "ovs-thread.h"
> >
> >  struct northd_input {
> > @@ -160,6 +161,23 @@ struct northd_data {
> >      struct northd_tracked_data trk_data;
> >  };
> >
> > +struct route_policy {
> > +    struct hmap_node key_node;
> > +    const struct nbrec_logical_router_policy *rule;
> > +    size_t n_valid_nexthops;
> > +    char **valid_nexthops;
> > +};
> > +
> > +struct bfd_consumer_data {
> > +    struct hmap parsed_routes;
> > +    struct hmap route_policies;
> > +    struct simap route_tables;
> > +};
> > +
> > +struct bfd_data {
> > +    struct hmap bfd_connections;
> > +};
> > +
> >  struct lr_nat_table;
> >
> >  struct lflow_input {
> > @@ -190,6 +208,9 @@ struct lflow_input {
> >      const struct hmap *svc_monitor_map;
> >      bool ovn_internal_version_changed;
> >      const char *svc_monitor_mac;
> > +    struct hmap *parsed_routes;
> > +    struct hmap *route_policies;
> > +    struct simap *route_tables;
> >  };
> >
> >  extern int parallelization_state;
> > @@ -661,6 +682,18 @@ struct ovn_port {
> >      struct lflow_ref *stateful_lflow_ref;
> >  };
> >
> > +struct parsed_route {
> > +    struct hmap_node key_node;
> > +    struct in6_addr prefix;
> > +    unsigned int plen;
> > +    bool is_src_route;
> > +    uint32_t route_table_id;
> > +    uint32_t hash;
> > +    const struct nbrec_logical_router_static_route *route;
> > +    bool ecmp_symmetric_reply;
> > +    bool is_discard_route;
> > +};
> > +
> >  void ovnnb_db_run(struct northd_input *input_data,
> >                    struct northd_data *data,
> >                    struct ovsdb_idl_txn *ovnnb_txn,
> > @@ -682,6 +715,17 @@ void northd_init(struct northd_data *data);
> >  void northd_indices_create(struct northd_data *data,
> >                             struct ovsdb_idl *ovnsb_idl);
> >
> > +struct parsed_route *parsed_routes_add(struct ovn_datapath *,
> > +        const struct hmap *, struct hmap *, struct simap *,
> > +        const struct nbrec_logical_router_static_route *,
> > +        const struct hmap *);
> > +uint32_t get_route_table_id(struct simap *, const char *);
> > +void bfd_consumer_init(struct bfd_consumer_data *);
> > +void bfd_consumer_destroy(struct bfd_consumer_data *);
> > +
> > +void bfd_init(struct bfd_data *);
> > +void bfd_destroy(struct bfd_data *);
> > +
> >  struct lflow_table;
> >  struct lr_stateful_tracked_data;
> >  struct ls_stateful_tracked_data;
> > @@ -719,7 +763,9 @@ bool northd_handle_lb_data_changes(struct
> tracked_lb_data *,
> >                                     struct hmap *lbgrp_datapaths_map,
> >                                     struct northd_tracked_data *);
> >
> > -void build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> > +void build_route_policies(struct ovn_datapath *, struct hmap *, struct
> hmap *,
> > +                          struct hmap *);
> > +bool build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
> >                       const struct nbrec_bfd_table *,
> >                       const struct sbrec_bfd_table *,
> >                       const struct hmap *lr_ports,
> > --
> > 2.44.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev