[ovs-dev,v3] northd: Add bfd, static_routes and route_policies I-P nodes.

Introduce bfd, static_routes and route_policies 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>
---
Changes since v2:
- add incremental processing routines
- split bfd_consumer node in static_routes and route_policies nodes
Changes since v1:
- add incremental processing logic for bfd_consumer node to avoid a full
  recompute
---
 northd/en-lflow.c        |  25 +-
 northd/en-northd.c       | 192 ++++++++++++
 northd/en-northd.h       |  15 +
 northd/inc-proc-northd.c |  31 +-
 northd/northd.c          | 648 +++++++++++++++++++++++++++------------
 northd/northd.h          |  62 +++-
 6 files changed, 757 insertions(+), 216 deletions(-)

On Wed, Jun 5, 2024 at 10:55 AM Lorenzo Bianconi
<lorenzo.bianconi@redhat.com> wrote:
>
> Introduce bfd, static_routes and route_policies 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>
> ---
> Changes since v2:
> - add incremental processing routines
> - split bfd_consumer node in static_routes and route_policies nodes
> Changes since v1:
> - add incremental processing logic for bfd_consumer node to avoid a full
>   recompute

Hi Lorenzo,

Thanks for the v3.  Please see below for some comments.

This patch is missing the test cases for the I-P handling.  Please see
the existing test cases [1]
in ovn-northd.at and please add test cases accordingly, making sure
that ovn-northd generates
the logical flows  as expected when a change occurs and then after recompute.
Please also check the engine stats.

[1] - https://github.com/ovn-org/ovn/blob/main/tests/ovn-northd.at#L11076

Thanks
Numan

> ---
>  northd/en-lflow.c        |  25 +-
>  northd/en-northd.c       | 192 ++++++++++++
>  northd/en-northd.h       |  15 +
>  northd/inc-proc-northd.c |  31 +-
>  northd/northd.c          | 648 +++++++++++++++++++++++++++------------
>  northd/northd.h          |  62 +++-
>  6 files changed, 757 insertions(+), 216 deletions(-)
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index c4b927fb8..3dba5034b 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -41,6 +41,11 @@ 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 static_routes_data *static_routes_data =
> +        engine_get_input_data("static_routes", node);
> +    struct route_policies_data *route_policies_data =
> +        engine_get_input_data("route_policies", node);
>      struct port_group_data *pg_data =
>          engine_get_input_data("port_group", node);
>      struct sync_meters_data *sync_meters_data =
> @@ -50,10 +55,6 @@ lflow_get_input_data(struct engine_node *node,
>      struct ed_type_ls_stateful *ls_stateful_data =
>          engine_get_input_data("ls_stateful", node);
>
> -    lflow_input->nbrec_bfd_table =
> -        EN_OVSDB_GET(engine_get_input("NB_bfd", node));
> -    lflow_input->sbrec_bfd_table =
> -        EN_OVSDB_GET(engine_get_input("SB_bfd", node));
>      lflow_input->sbrec_logical_flow_table =
>          EN_OVSDB_GET(engine_get_input("SB_logical_flow", node));
>      lflow_input->sbrec_multicast_group_table =
> @@ -78,7 +79,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 = &static_routes_data->parsed_routes;
> +    lflow_input->route_tables = &static_routes_data->route_tables;
> +    lflow_input->route_policies = &route_policies_data->route_policies;
>
>      struct ed_type_global_config *global_config =
>          engine_get_input_data("global_config", node);
> @@ -95,25 +99,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..a4de71ba1 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -236,6 +236,150 @@ northd_global_config_handler(struct engine_node *node, void *data OVS_UNUSED)
>      return true;
>  }
>
> +bool
> +route_policies_change_handler(struct engine_node *node, void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct route_policies_data *route_policies_data = data;
> +    enum engine_node_state state = EN_UNCHANGED;
> +
> +    struct ovn_datapath *od;
> +    HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) {

I don't think it makes sense to loop through all the lr datapaths and
build the route policies when the
northd engine node changes.  Since there is not much differerent
between this handler function
and the en_route_policies_run(), there is no real value in this handler.

Ideally northd engine node (see the 'struct northd_tracked_data' in
northd.h should include the changed
or tracked logical routers in its tracked data.  Presently northd
engine node falls back to full recompute
when a logical router or logical router port changes.  So it makes
sense for this handler to return true
if there is tracked data in northd engine node.  This means that no
logical router or router policy changed
and hence this handler can return true.

Please see lr_stateful_northd_handler() in the file en-lr-stateful.c.

The handler route_policies_northd_change_handler() would look
something like below

-----
bool
route_policies_northd_change_handler(struct engine_node *node, void
*data OVS_UNUSED)
{
    struct northd_data *northd_data = engine_get_input_data("northd", node);
    if (!northd_has_tracked_data(&northd_data->trk_data)) {
        return false;
    }

    /*
     *  Comments explaining why it is returning true if there is tracked data.
     * */
    return true;
}

-----

Same comments apply to static_routes_change_handler() and bfd_change_handler().



> +        if (build_route_policies(od, &northd_data->lr_ports,
> +                                 &route_policies_data->route_policies)) {
> +            state = EN_UPDATED;
> +        }
> +    }
> +    engine_set_node_state(node, state);
> +
> +    return true;
> +}


> +
> +void
> +en_route_policies_run(struct engine_node *node, void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct route_policies_data *route_policies_data = data;
> +

Normally the engine node run function destroys the old data and then builds it.
Looks like you're doing the clear for bfd engine node but not for
router_policies and
state_routes engine nodes ?  Any reason for that ?  I think you should clean  up
the old data and (re)build the data in the run functions.

> +    struct ovn_datapath *od;
> +    HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) {
> +        build_route_policies(od, &northd_data->lr_ports,
> +                             &route_policies_data->route_policies);
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +}
> +
> +bool
> +static_routes_change_handler(struct engine_node *node, void *data)
> +{

Please see above my comments in route_policies_change_handler()

> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct static_routes_data *static_routes_data = data;
> +    enum engine_node_state state = EN_UNCHANGED;
> +
> +    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++) {

Please declare "i" of type size_t.

> +            const char *route_table_name =
> +                smap_get(&od->nbr->ports[i]->options, "route_table");
> +            get_route_table_id(&static_routes_data->route_tables,
> +                               route_table_name);
> +        }
> +
> +        if (build_parsed_routes(od, &northd_data->lr_ports,
> +                                &static_routes_data->parsed_routes,
> +                                &static_routes_data->route_tables)) {
> +            state = EN_UPDATED;
> +        }
> +    }
> +    engine_set_node_state(node, state);
> +
> +    return true;
> +}
> +
> +void
> +en_static_routes_run(struct engine_node *node, void *data)
> +{
> +
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct static_routes_data *static_routes_data = 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(&static_routes_data->route_tables,
> +                               route_table_name);
> +        }
> +
> +        build_parsed_routes(od, &northd_data->lr_ports,
> +                            &static_routes_data->parsed_routes,
> +                            &static_routes_data->route_tables);
> +    }
> +
> +    engine_set_node_state(node, EN_UPDATED);
> +}
> +
> +bool
> +bfd_change_handler(struct engine_node *node, void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct bfd_data *bfd_data = data;
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    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));
> +    struct engine_node *nb_lr_sr_node =
> +        engine_get_input("NB_logical_router_static_route", node);
> +    const struct nbrec_logical_router_static_route_table *
> +        nbrec_static_route_table = EN_OVSDB_GET(nb_lr_sr_node);
> +    struct engine_node *nb_lr_policy_node =
> +        engine_get_input("NB_logical_router_policy", node);
> +    const struct nbrec_logical_router_policy_table *
> +        nbrec_router_policy_table = EN_OVSDB_GET(nb_lr_policy_node);
> +
> +    if (build_bfd_table(eng_ctx->ovnsb_idl_txn,
> +                        nbrec_bfd_table, sbrec_bfd_table,
> +                        nbrec_static_route_table, nbrec_router_policy_table,
> +                        &northd_data->lr_ports, &bfd_data->bfd_connections)) {
> +        engine_set_node_state(node, EN_UPDATED);
> +    } else {
> +        engine_set_node_state(node, EN_UNCHANGED);
> +    }
> +
> +    return true;
> +}
> +
> +void
> +en_bfd_run(struct engine_node *node, void *data)
> +{
> +    struct northd_data *northd_data = engine_get_input_data("northd", node);
> +    struct bfd_data *bfd_data = data;
> +    const struct engine_context *eng_ctx = engine_get_context();
> +    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));
> +    struct engine_node *nb_lr_sr_node =
> +        engine_get_input("NB_logical_router_static_route", node);
> +    const struct nbrec_logical_router_static_route_table *
> +        nbrec_static_route_table = EN_OVSDB_GET(nb_lr_sr_node);
> +    struct engine_node *nb_lr_policy_node =
> +        engine_get_input("NB_logical_router_policy", node);
> +    const struct nbrec_logical_router_policy_table *
> +        nbrec_router_policy_table = EN_OVSDB_GET(nb_lr_policy_node);
> +
> +    bfd_destroy(data);
> +    bfd_init(data);
> +    build_bfd_table(eng_ctx->ovnsb_idl_txn,
> +                    nbrec_bfd_table, sbrec_bfd_table,
> +                    nbrec_static_route_table, nbrec_router_policy_table,
> +                    &northd_data->lr_ports, &bfd_data->bfd_connections);
> +    engine_set_node_state(node, EN_UPDATED);
> +}
> +
>  void
>  *en_northd_init(struct engine_node *node OVS_UNUSED,
>                  struct engine_arg *arg OVS_UNUSED)
> @@ -247,6 +391,36 @@ void
>      return data;
>  }
>
> +void
> +*en_route_policies_init(struct engine_node *node OVS_UNUSED,
> +                        struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct route_policies_data *data = xzalloc(sizeof *data);
> +
> +    route_policies_init(data);
> +    return data;
> +}
> +
> +void
> +*en_static_routes_init(struct engine_node *node OVS_UNUSED,
> +                      struct engine_arg *arg OVS_UNUSED)
> +{
> +    struct static_routes_data *data = xzalloc(sizeof *data);
> +
> +    static_routes_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 +433,21 @@ en_northd_clear_tracked_data(void *data_)
>      struct northd_data *data = data_;
>      destroy_northd_data_tracked_changes(data);
>  }
> +
> +void
> +en_route_policies_cleanup(void *data)
> +{
> +    route_policies_destroy(data);
> +}
> +
> +void
> +en_static_routes_cleanup(void *data)
> +{
> +    static_routes_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..424209c2f 100644
> --- a/northd/en-northd.h
> +++ b/northd/en-northd.h
> @@ -19,5 +19,20 @@ 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_static_routes_init(struct engine_node *node OVS_UNUSED,
> +                            struct engine_arg *arg OVS_UNUSED);
> +void en_route_policies_cleanup(void *data);
> +bool route_policies_change_handler(struct engine_node *node, void *data);
> +void en_route_policies_run(struct engine_node *node, void *data);
> +void *en_route_policies_init(struct engine_node *node OVS_UNUSED,
> +                             struct engine_arg *arg OVS_UNUSED);
> +void en_static_routes_cleanup(void *data);
> +bool static_routes_change_handler(struct engine_node *node, void *data);
> +void en_static_routes_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);
> +bool bfd_change_handler(struct engine_node *node, 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 d56e9783a..d907da14d 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,9 @@ 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(route_policies, "route_policies");
> +static ENGINE_NODE(static_routes, "static_routes");
> +static ENGINE_NODE(bfd, "bfd");
>
>  void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
>                            struct ovsdb_idl_loop *sb)
> @@ -237,20 +242,40 @@ 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_nb_logical_router_policy, NULL);
> +    engine_add_input(&en_bfd, &en_nb_logical_router_static_route, NULL);
> +    engine_add_input(&en_bfd, &en_sb_bfd, NULL);
> +    engine_add_input(&en_bfd, &en_northd, bfd_change_handler);

Please rename the handler function name to "bfd_northd_change_handler"
which indicates
that this handler handles the northd engine input changes.

> +
> +    engine_add_input(&en_route_policies, &en_bfd, NULL);
> +    engine_add_input(&en_route_policies, &en_nb_bfd, NULL);
> +    engine_add_input(&en_route_policies, &en_nb_logical_router_policy, NULL);
> +    engine_add_input(&en_route_policies, &en_northd,
> +                     route_policies_change_handler);

Same here.  Please rename it to "route_policies_northd_change_handler".

> +
> +    engine_add_input(&en_static_routes, &en_bfd, NULL);
> +    engine_add_input(&en_static_routes, &en_northd,
> +                     static_routes_change_handler);

Same here.  Please rename it to "static_routes_northd_change_handler".

Move the above "add_input" at the end so that all the NULL inputs are ordered
first and then the inputs with handlers.

> +    engine_add_input(&en_static_routes, &en_nb_bfd, NULL);
> +    engine_add_input(&en_static_routes,
> +                     &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);
>
> -    engine_add_input(&en_lflow, &en_nb_bfd, NULL);
>      engine_add_input(&en_lflow, &en_nb_acl, NULL);
>      engine_add_input(&en_lflow, &en_sync_meters, NULL);
> -    engine_add_input(&en_lflow, &en_sb_bfd, NULL);
>      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_sb_logical_dp_group, NULL);
>      engine_add_input(&en_lflow, &en_global_config,
>                       node_global_config_handler);
> +    engine_add_input(&en_lflow, &en_route_policies, NULL);
> +    engine_add_input(&en_lflow, &en_static_routes, NULL);
> +    engine_add_input(&en_lflow, &en_bfd, NULL);

Move the above "add_input"s before "en_global_config" input.

Thanks
Numan


>      engine_add_input(&en_lflow, &en_northd, lflow_northd_handler);
>      engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler);
>      engine_add_input(&en_lflow, &en_lr_stateful, lflow_lr_stateful_handler);
> diff --git a/northd/northd.c b/northd/northd.c
> index a78cbcd53..2eb5f2be8 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -9764,9 +9764,33 @@ struct bfd_entry {
>
>      const struct sbrec_bfd *sb_bt;
>
> -    bool ref;
> +    char *logical_port;
> +    char *dst_ip;
> +    bool stale;
>  };
>
> +static struct bfd_entry *
> +bfd_alloc_entry(struct hmap *bfd_connections,
> +                const char *logical_port, const char *dst_ip)
> +{
> +    struct bfd_entry *bfd_e = xzalloc(sizeof *bfd_e);
> +    bfd_e->logical_port = xstrdup(logical_port);
> +    bfd_e->dst_ip = xstrdup(dst_ip);
> +    uint32_t hash = hash_string(dst_ip, 0);
> +    hash = hash_string(logical_port, hash);
> +    hmap_insert(bfd_connections, &bfd_e->hmap_node, hash);
> +
> +    return bfd_e;
> +}
> +
> +static void
> +bfd_erase_entry(struct bfd_entry *bfd_e)
> +{
> +    free(bfd_e->logical_port);
> +    free(bfd_e->dst_ip);
> +    free(bfd_e);
> +}
> +
>  static struct bfd_entry *
>  bfd_port_lookup(const struct hmap *bfd_map, const char *logical_port,
>                  const char *dst_ip)
> @@ -9777,73 +9801,60 @@ bfd_port_lookup(const struct hmap *bfd_map, const char *logical_port,
>      hash = hash_string(dst_ip, 0);
>      hash = hash_string(logical_port, hash);
>      HMAP_FOR_EACH_WITH_HASH (bfd_e, hmap_node, hash, bfd_map) {
> -        if (!strcmp(bfd_e->sb_bt->logical_port, logical_port) &&
> -            !strcmp(bfd_e->sb_bt->dst_ip, dst_ip)) {
> +        if (!strcmp(bfd_e->logical_port, logical_port) &&
> +            !strcmp(bfd_e->dst_ip, dst_ip)) {
>              return bfd_e;
>          }
>      }
> -    return NULL;
> -}
> -
> -void
> -bfd_cleanup_connections(const struct nbrec_bfd_table *nbrec_bfd_table,
> -                        struct hmap *bfd_map)
> -{
> -    const struct nbrec_bfd *nb_bt;
> -    struct bfd_entry *bfd_e;
> -
> -    NBREC_BFD_TABLE_FOR_EACH (nb_bt, nbrec_bfd_table) {
> -        bfd_e = bfd_port_lookup(bfd_map, nb_bt->logical_port, nb_bt->dst_ip);
> -        if (!bfd_e) {
> -            continue;
> -        }
> -
> -        if (!bfd_e->ref && strcmp(nb_bt->status, "admin_down")) {
> -            /* no user for this bfd connection */
> -            nbrec_bfd_set_status(nb_bt, "admin_down");
> -        }
> -    }
>
> -    HMAP_FOR_EACH_POP (bfd_e, hmap_node, bfd_map) {
> -        free(bfd_e);
> -    }
> +    return NULL;
>  }
>
>  #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
> @@ -9869,26 +9880,63 @@ static int bfd_get_unused_port(unsigned long *bfd_src_ports)
>      return port + BFD_UDP_SRC_PORT_START;
>  }
>
> -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,
> -                const struct hmap *lr_ports, struct hmap *bfd_connections)
> +static bool
> +bfd_lookup_consumer(
> +    struct bfd_entry *bfd_e,
> +    const struct nbrec_logical_router_static_route_table *nbrec_sr_table,
> +    const struct nbrec_logical_router_policy_table *nbrec_rp_table)
> +{
> +    const struct nbrec_logical_router_static_route *r;
> +    NBREC_LOGICAL_ROUTER_STATIC_ROUTE_TABLE_FOR_EACH (r, nbrec_sr_table) {
> +        if (r->bfd && !strcmp(r->bfd->logical_port, bfd_e->logical_port) &&
> +            !strcmp(r->bfd->dst_ip, bfd_e->dst_ip)) {
> +            return true;
> +        }
> +    }
> +
> +    const struct nbrec_logical_router_policy *p;
> +    NBREC_LOGICAL_ROUTER_POLICY_TABLE_FOR_EACH (p, nbrec_rp_table) {
> +        for (size_t i = 0; i < p->n_bfd_sessions; i++) {
> +            struct nbrec_bfd *nb_bt = p->bfd_sessions[i];
> +            if (!strcmp(nb_bt->logical_port, bfd_e->logical_port) &&
> +                !strcmp(nb_bt->dst_ip, bfd_e->dst_ip)) {
> +                return true;
> +            }
> +        }
> +    }
> +
> +    return false;
> +}
> +
> +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,
> +    const struct nbrec_logical_router_static_route_table *nbrec_sr_table,
> +    const struct nbrec_logical_router_policy_table *nbrec_rp_table,
> +    const struct hmap *lr_ports, struct hmap *bfd_connections)
>  {
> -    struct hmap sb_only = HMAP_INITIALIZER(&sb_only);
> -    const struct sbrec_bfd *sb_bt;
> -    unsigned long *bfd_src_ports;
> -    struct bfd_entry *bfd_e;
> -    uint32_t hash;
> +    if (!ovnsb_txn) {
> +        return false;
> +    }
>
> -    bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN);
> +    unsigned long *bfd_src_ports = bitmap_allocate(BFD_UDP_SRC_PORT_LEN);
> +    struct bfd_entry *bfd_e;
> +    bool ret = false;
>
> +    /* align bfd map to sb db */
> +    const struct sbrec_bfd *sb_bt;
>      SBREC_BFD_TABLE_FOR_EACH (sb_bt, sbrec_bfd_table) {
> -        bfd_e = xmalloc(sizeof *bfd_e);
> +        bfd_e = bfd_port_lookup(bfd_connections, sb_bt->logical_port,
> +                                sb_bt->dst_ip);
> +        if (!bfd_e) {
> +            bfd_e = bfd_alloc_entry(bfd_connections, sb_bt->logical_port,
> +                                    sb_bt->dst_ip);
> +        }
>          bfd_e->sb_bt = sb_bt;
> -        hash = hash_string(sb_bt->dst_ip, 0);
> -        hash = hash_string(sb_bt->logical_port, hash);
> -        hmap_insert(&sb_only, &bfd_e->hmap_node, hash);
> +        bfd_e->stale = true;
> +        /* we need to check if this entry is even in the BFD nb db table */
>          bitmap_set1(bfd_src_ports, sb_bt->src_port - BFD_UDP_SRC_PORT_START);
>      }
>
> @@ -9900,7 +9948,13 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>          }
>
>          struct ovn_port *op = ovn_port_find(lr_ports, nb_bt->logical_port);
> -        bfd_e = bfd_port_lookup(&sb_only, nb_bt->logical_port, nb_bt->dst_ip);
> +        if (!op || !op->sb) {
> +            /* skip not bounded ports */
> +            continue;
> +        }
> +
> +        bfd_e = bfd_port_lookup(bfd_connections, nb_bt->logical_port,
> +                                nb_bt->dst_ip);
>          if (!bfd_e) {
>              int udp_src = bfd_get_unused_port(bfd_src_ports);
>              if (udp_src < 0) {
> @@ -9913,7 +9967,7 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>              sbrec_bfd_set_disc(sb_bt, 1 + random_uint32());
>              sbrec_bfd_set_src_port(sb_bt, udp_src);
>              sbrec_bfd_set_status(sb_bt, nb_bt->status);
> -            if (op && op->sb && op->sb->chassis) {
> +            if (op->sb->chassis) {
>                  sbrec_bfd_set_chassis_name(sb_bt, op->sb->chassis->name);
>              }
>
> @@ -9924,27 +9978,39 @@ 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);
> +
> +            bfd_e = bfd_alloc_entry(bfd_connections, nb_bt->logical_port,
> +                                    nb_bt->dst_ip);
> +            bfd_e->sb_bt = sb_bt;
> +            ret = true;
>          } else {
> -            if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
> +            bfd_e->stale = false;
> +
> +            if (!bfd_lookup_consumer(bfd_e, nbrec_sr_table, nbrec_rp_table) &&
> +                strcmp(nb_bt->status, "admin_down")) {
> +                /* set status in admin_down if there are no consumers for the
> +                 * bfd connection.
> +                 */
> +                nbrec_bfd_set_status(nb_bt, "admin_down");
> +                sbrec_bfd_set_status(bfd_e->sb_bt, "admin_down");
> +                ret = true;
> +            } else if (strcmp(bfd_e->sb_bt->status, nb_bt->status)) {
>                  if (!strcmp(nb_bt->status, "admin_down") ||
>                      !strcmp(bfd_e->sb_bt->status, "admin_down")) {
>                      sbrec_bfd_set_status(bfd_e->sb_bt, nb_bt->status);
>                  } else {
>                      nbrec_bfd_set_status(nb_bt, bfd_e->sb_bt->status);
>                  }
> +                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)) {
> +
> +            ret |= build_bfd_update_sb_conf(nb_bt, bfd_e->sb_bt);
> +            if (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);
> -            bfd_e->ref = false;
> -            hash = hash_string(bfd_e->sb_bt->dst_ip, 0);
> -            hash = hash_string(bfd_e->sb_bt->logical_port, hash);
> -            hmap_insert(bfd_connections, &bfd_e->hmap_node, hash);
>          }
>
>          if (op) {
> @@ -9952,18 +10018,25 @@ build_bfd_table(struct ovsdb_idl_txn *ovnsb_txn,
>          }
>      }
>
> -    HMAP_FOR_EACH_POP (bfd_e, hmap_node, &sb_only) {
> -        struct ovn_port *op = ovn_port_find(lr_ports,
> -                                            bfd_e->sb_bt->logical_port);
> +    HMAP_FOR_EACH_SAFE (bfd_e, hmap_node, bfd_connections) {
> +        if (!bfd_e->stale) {
> +            continue;
> +        }
> +
> +        struct ovn_port *op = ovn_port_find(lr_ports, bfd_e->logical_port);
>          if (op) {
>              op->has_bfd = false;
>          }
> +
> +        ret = true;
> +        hmap_remove(bfd_connections, &bfd_e->hmap_node);
>          sbrec_bfd_delete(bfd_e->sb_bt);
> -        free(bfd_e);
> +        bfd_erase_entry(bfd_e);
>      }
> -    hmap_destroy(&sb_only);
>
>      bitmap_free(bfd_src_ports);
> +
> +    return ret;
>  }
>
>  /* Returns a string of the IP address of the router port 'op' that
> @@ -10003,11 +10076,8 @@ get_outport_for_routing_policy_nexthop(struct ovn_datapath *od,
>
>  static struct ovs_mutex bfd_lock = OVS_MUTEX_INITIALIZER;
>
> -static bool check_bfd_state(
> -        const struct nbrec_logical_router_policy *rule,
> -        const struct hmap *bfd_connections,
> -        struct ovn_port *out_port,
> -        const char *nexthop)
> +static bool check_bfd_state(const struct nbrec_logical_router_policy *rule,
> +                            struct ovn_port *out_port, const char *nexthop)
>  {
>      struct in6_addr nexthop_v6;
>      bool is_nexthop_v6 = ipv6_parse(nexthop, &nexthop_v6);
> @@ -10033,14 +10103,7 @@ static bool check_bfd_state(
>              continue;
>          }
>
> -        struct bfd_entry *bfd_e = bfd_port_lookup(bfd_connections,
> -                                                  nb_bt->logical_port,
> -                                                  nb_bt->dst_ip);
>          ovs_mutex_lock(&bfd_lock);
> -        if (bfd_e) {
> -            bfd_e->ref = true;
> -        }
> -
>          if (!strcmp(nb_bt->status, "admin_down")) {
>              nbrec_bfd_set_status(nb_bt, "down");
>          }
> @@ -10055,20 +10118,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) {
> @@ -10084,10 +10149,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);
> @@ -10130,19 +10191,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;
> @@ -10153,7 +10213,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;
>          }
>      }
> @@ -10161,40 +10221,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; "
> @@ -10203,7 +10253,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,
> @@ -10219,37 +10269,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);
>  }
> @@ -10274,7 +10317,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]) {
> @@ -10315,18 +10358,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)
>  {
> @@ -10341,11 +10372,51 @@ 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 *
> +parsed_route_lookup(struct hmap *routes, size_t hash,
> +                    struct parsed_route *new_pr)
> +{
> +    struct parsed_route *pr;
> +    HMAP_FOR_EACH_WITH_HASH (pr, key_node, hash, routes) {
> +        if (pr->plen != new_pr->plen) {
> +            continue;
> +        }
> +
> +        if (memcmp(&pr->prefix, &new_pr->prefix, sizeof(struct in6_addr))) {
> +            continue;
> +        }
> +
> +        if (pr->is_src_route != new_pr->is_src_route) {
> +            continue;
> +        }
> +
> +        if (pr->route_table_id != new_pr->route_table_id) {
> +            continue;
> +        }
> +
> +        if (pr->route != new_pr->route) {
> +            continue;
> +        }
> +
> +        if (pr->ecmp_symmetric_reply != new_pr->ecmp_symmetric_reply) {
> +            continue;
> +        }
> +
> +        if (pr->is_discard_route != new_pr->is_discard_route) {
> +            continue;
> +        }
> +
> +        return pr;
> +    }
> +
> +    return NULL;
> +}
> +
> +static bool
>  parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> -                  struct ovs_list *routes, struct simap *route_tables,
> -                  const struct nbrec_logical_router_static_route *route,
> -                  const struct hmap *bfd_connections)
> +                  struct hmap *routes, struct simap *route_tables,
> +                  const struct nbrec_logical_router_static_route *route)
>  {
>      /* Verify that the next hop is an IP address with an all-ones mask. */
>      struct in6_addr nexthop;
> @@ -10358,7 +10429,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>              VLOG_WARN_RL(&rl, "bad 'nexthop' %s in static route "
>                           UUID_FMT, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> -            return NULL;
> +            return false;
>          }
>          if ((IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 32) ||
>              (!IN6_IS_ADDR_V4MAPPED(&nexthop) && plen != 128)) {
> @@ -10366,7 +10437,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>              VLOG_WARN_RL(&rl, "bad next hop mask %s in static route "
>                           UUID_FMT, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> -            return NULL;
> +            return false;
>          }
>      }
>
> @@ -10377,7 +10448,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>          VLOG_WARN_RL(&rl, "bad 'ip_prefix' %s in static route "
>                       UUID_FMT, route->ip_prefix,
>                       UUID_ARGS(&route->header_.uuid));
> -        return NULL;
> +        return false;
>      }
>
>      /* Verify that ip_prefix and nexthop have same address familiy. */
> @@ -10388,7 +10459,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>                           " %s and 'nexthop' %s in static route "UUID_FMT,
>                           route->ip_prefix, route->nexthop,
>                           UUID_ARGS(&route->header_.uuid));
> -            return NULL;
> +            return false;
>          }
>      }
>
> @@ -10397,54 +10468,80 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
>          !find_static_route_outport(od, lr_ports, route,
>                                     IN6_IS_ADDR_V4MAPPED(&prefix),
>                                     NULL, NULL)) {
> -        return NULL;
> +        return false;
>      }
>
>      const struct nbrec_bfd *nb_bt = route->bfd;
>      if (nb_bt && !strcmp(nb_bt->dst_ip, route->nexthop)) {
> -        struct bfd_entry *bfd_e;
> -
> -        bfd_e = bfd_port_lookup(bfd_connections, nb_bt->logical_port,
> -                                nb_bt->dst_ip);
>          ovs_mutex_lock(&bfd_lock);
> -        if (bfd_e) {
> -            bfd_e->ref = true;
> -        }
> -
>          if (!strcmp(nb_bt->status, "admin_down")) {
>              nbrec_bfd_set_status(nb_bt, "down");
>          }
>
>          if (!strcmp(nb_bt->status, "down")) {
>              ovs_mutex_unlock(&bfd_lock);
> -            return NULL;
> +            return false;
>          }
>          ovs_mutex_unlock(&bfd_lock);
>      }
>
> -    struct parsed_route *pr = xzalloc(sizeof *pr);
> -    pr->prefix = prefix;
> -    pr->plen = plen;
> -    pr->route_table_id = get_route_table_id(route_tables, route->route_table);
> -    pr->is_src_route = (route->policy && !strcmp(route->policy,
> -                                                 "src-ip"));
> -    pr->hash = route_hash(pr);
> -    pr->route = route;
> -    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);
> -    return pr;
> +    struct parsed_route *new_pr = xzalloc(sizeof *new_pr);
> +    new_pr->prefix = prefix;
> +    new_pr->plen = plen;
> +    new_pr->route_table_id = get_route_table_id(route_tables,
> +                                                route->route_table);
> +    new_pr->is_src_route = (route->policy &&
> +                            !strcmp(route->policy, "src-ip"));
> +    new_pr->hash = route_hash(new_pr);
> +    new_pr->route = route;
> +    new_pr->nbr = od->nbr;
> +    new_pr->ecmp_symmetric_reply = smap_get_bool(&route->options,
> +                                                 "ecmp_symmetric_reply",
> +                                                 false);
> +    new_pr->is_discard_route = is_discard_route;
> +
> +    size_t hash = uuid_hash(&od->key);
> +    struct parsed_route *pr = parsed_route_lookup(routes, hash, new_pr);
> +    if (!pr) {
> +        hmap_insert(routes, &new_pr->key_node, hash);
> +        return true;
> +    } else {
> +        pr->stale = false;
> +        free(new_pr);
> +    }
> +    return false;
>  }
>
> -static void
> -parsed_routes_destroy(struct ovs_list *routes)
> +bool
> +build_parsed_routes(struct ovn_datapath *od, const struct hmap *lr_ports,
> +                    struct hmap *routes, struct simap *route_tables)
>  {
> +    bool ret = false;
>      struct parsed_route *pr;
> -    LIST_FOR_EACH_SAFE (pr, list_node, routes) {
> -        ovs_list_remove(&pr->list_node);
> +    HMAP_FOR_EACH (pr, key_node, routes) {
> +        if (pr->nbr == od->nbr) {
> +            pr->stale = true;
> +        }
> +    }
> +
> +    for (int i = 0; i < od->nbr->n_static_routes; i++) {
> +        if (parsed_routes_add(od, lr_ports, routes, route_tables,
> +                              od->nbr->static_routes[i])) {
> +            ret = true;
> +        }
> +    }
> +
> +    HMAP_FOR_EACH_SAFE (pr, key_node, routes) {
> +        if (!pr->stale) {
> +            continue;
> +        }
> +
> +        ret = true;
> +        hmap_remove(routes, &pr->key_node);
>          free(pr);
>      }
> +
> +    return ret;
>  }
>
>  struct ecmp_route_list_node {
> @@ -12739,7 +12836,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);
> @@ -12753,22 +12851,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);
> @@ -12797,8 +12889,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
> @@ -12905,6 +12995,116 @@ build_mcast_lookup_flows_for_lrouter(
>      }
>  }
>
> +static struct route_policy *
> +route_policies_lookup(struct hmap *route_policies, size_t hash,
> +                      struct route_policy *new_rp)
> +{
> +    struct route_policy *rp;
> +    HMAP_FOR_EACH_WITH_HASH (rp, key_node, hash, route_policies) {
> +        if (rp->rule != new_rp->rule) {
> +            continue;
> +        }
> +
> +        if (rp->n_valid_nexthops != new_rp->n_valid_nexthops) {
> +            continue;
> +        }
> +
> +        size_t i;
> +        for (i = 0; i < new_rp->n_valid_nexthops; i++) {
> +            size_t j;
> +
> +            for (j = 0; j < rp->n_valid_nexthops; j++) {
> +                if (!strcmp(new_rp->valid_nexthops[i],
> +                            rp->valid_nexthops[j])) {
> +                    break;
> +                }
> +            }
> +
> +            if (j == rp->n_valid_nexthops) {
> +                break;
> +            }
> +        }
> +
> +        if (i == new_rp->n_valid_nexthops) {
> +            return rp;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +bool
> +build_route_policies(struct ovn_datapath *od, struct hmap *lr_ports,
> +                     struct hmap *route_policies)
> +{
> +    struct route_policy *rp;
> +    bool ret = false;
> +
> +    HMAP_FOR_EACH (rp, key_node, route_policies) {
> +        if (rp->nbr == od->nbr) {
> +            rp->stale = true;
> +        }
> +    }
> +
> +    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, out_port, nexthop)) {
> +                    continue;
> +                }
> +                valid_nexthops[n_valid_nexthops++] = nexthop;
> +            }
> +
> +            if (!n_valid_nexthops) {
> +                free(valid_nexthops);
> +                continue;
> +            }
> +        }
> +
> +        struct route_policy *new_rp = xzalloc(sizeof *new_rp);
> +        new_rp->rule = rule;
> +        new_rp->n_valid_nexthops = n_valid_nexthops;
> +        new_rp->valid_nexthops = valid_nexthops;
> +        new_rp->nbr = od->nbr;
> +
> +        size_t hash = uuid_hash(&od->key);
> +        rp = route_policies_lookup(route_policies, hash, new_rp);
> +        if (!rp) {
> +            hmap_insert(route_policies, &new_rp->key_node, hash);
> +            ret = true;
> +        } else {
> +            rp->stale = false;
> +            free(valid_nexthops);
> +            free(new_rp);
> +        }
> +    }
> +
> +    HMAP_FOR_EACH_SAFE (rp, key_node, route_policies) {
> +        if (!rp->stale) {
> +            continue;
> +        }
> +
> +        ret = true;
> +        hmap_remove(route_policies, &rp->key_node);
> +        free(rp->valid_nexthops);
> +        free(rp);
> +    }
> +
> +    return ret;
> +}
> +
>  /* Logical router ingress table POLICY: Policy.
>   *
>   * A packet that arrives at this table is an IP packet that should be
> @@ -12918,7 +13118,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);
> @@ -12935,21 +13135,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);
>          }
>      }
>  }
> @@ -15903,6 +16102,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
> @@ -15949,12 +16151,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,
> @@ -16270,7 +16473,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);
> @@ -16304,6 +16510,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);
>
> @@ -16344,6 +16553,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,
>          };
> @@ -16505,7 +16717,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;
> @@ -17581,6 +17796,25 @@ northd_init(struct northd_data *data)
>      init_northd_tracked_data(data);
>  }
>
> +void
> +route_policies_init(struct route_policies_data *data)
> +{
> +    hmap_init(&data->route_policies);
> +}
> +
> +void
> +static_routes_init(struct static_routes_data *data)
> +{
> +    hmap_init(&data->parsed_routes);
> +    simap_init(&data->route_tables);
> +}
> +
> +void
> +bfd_init(struct bfd_data *data)
> +{
> +    hmap_init(&data->bfd_connections);
> +}
> +
>  void
>  northd_destroy(struct northd_data *data)
>  {
> @@ -17620,6 +17854,40 @@ northd_destroy(struct northd_data *data)
>      destroy_northd_tracked_data(data);
>  }
>
> +void
> +route_policies_destroy(struct route_policies_data *data)
> +{
> +    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);
> +}
> +
> +void
> +static_routes_destroy(struct static_routes_data *data)
> +{
> +    struct parsed_route *r;
> +    HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) {
> +        free(r);
> +    }
> +    hmap_destroy(&data->parsed_routes);
> +
> +    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) {
> +        bfd_erase_entry(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..4c761f0a3 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,14 +161,32 @@ 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;
> +    const struct nbrec_logical_router *nbr;
> +    bool stale;
> +};
> +
> +struct static_routes_data {
> +    struct hmap parsed_routes;
> +    struct simap route_tables;
> +};
> +
> +struct route_policies_data {
> +    struct hmap route_policies;
> +};
> +
> +struct bfd_data {
> +    struct hmap bfd_connections;
> +};
> +
>  struct lr_nat_table;
>
>  struct lflow_input {
> -    /* Northbound table references */
> -    const struct nbrec_bfd_table *nbrec_bfd_table;
> -
>      /* Southbound table references */
> -    const struct sbrec_bfd_table *sbrec_bfd_table;
>      const struct sbrec_logical_flow_table *sbrec_logical_flow_table;
>      const struct sbrec_multicast_group_table *sbrec_multicast_group_table;
>      const struct sbrec_igmp_group_table *sbrec_igmp_group_table;
> @@ -190,6 +209,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 +683,20 @@ 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;
> +    const struct nbrec_logical_router *nbr;
> +    bool stale;
> +};
> +
>  void ovnnb_db_run(struct northd_input *input_data,
>                    struct northd_data *data,
>                    struct ovsdb_idl_txn *ovnnb_txn,
> @@ -682,6 +718,17 @@ void northd_init(struct northd_data *data);
>  void northd_indices_create(struct northd_data *data,
>                             struct ovsdb_idl *ovnsb_idl);
>
> +void route_policies_init(struct route_policies_data *);
> +void route_policies_destroy(struct route_policies_data *);
> +bool build_parsed_routes(struct ovn_datapath *, const struct hmap *,
> +                         struct hmap *, struct simap *);
> +uint32_t get_route_table_id(struct simap *, const char *);
> +void static_routes_init(struct static_routes_data *);
> +void static_routes_destroy(struct static_routes_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,13 +766,14 @@ 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,
> +bool build_route_policies(struct ovn_datapath *, 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 nbrec_logical_router_static_route_table *,
> +                     const struct nbrec_logical_router_policy_table *,
>                       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);
>
>  const struct ovn_datapath *northd_get_datapath_for_port(
> --
> 2.45.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>