On 5/16/24 15:08, Lorenzo Bianconi wrote: >> On Thu, May 2, 2024 at 8:22 AM Lorenzo Bianconi <[email protected]> >> 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 <[email protected]> >> >> 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? >
I think that's accurate. >> >> 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? > Like Lorenzo mentioned above, en_bfd will be marked as UPDATED only if the BFD state actually changed. Without this patch we were already triggering an lflow recompute: engine_add_input(&en_lflow, &en_sb_bfd, NULL); Which makes me wonder, Lorenzo, why do we still have this dependency? >> >> 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? +1, a detailed comment should be added, I agree with Han, it's not always obvious. > > 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 Regards, Dumitru > > [0] > https://patchwork.ozlabs.org/project/ovn/patch/f31fb6630039c9b310b7fa489247d354f8a46719.1710257650.git.lorenzo.bianc...@redhat.com/ > [1] https://issues.redhat.com/browse/FDP-56 > [2] > https://patchwork.ozlabs.org/project/ovn/cover/[email protected]/ > >> >>> --- >>> 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 >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
