> 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? > > For the patch, I haven't looked into details yet, but it seems you didn't > add any change handler for the two new nodes. So will there be follow up > patches for the real performance improvement? for BFD node the logic is already in en_bfd_run() since we do not always return EN_UPDATED but if there are no valid changes, we will return EN_UNCHANGED. Am I missing something? For bfd-consumer node I think we actually do not need it. Am I wrong? > But even for the current patch, with this: engine_add_input(&en_bfd, > &en_northd, NULL); does it mean any en_northd change would trigger a lflow > recompute because of the dependency en_northd -> en_bfd -> en_lflow. Would > this be a performance regression? If not, why? I guess no, otherwise the IP ovn tests will fail, right? > > For the noop handler used, please provide a detailed explanation to justify > it: why the node depends on the input but doesn't need to process when the > input changes? I think for the bfd-consumer node we have just an order dependency with northd one, right? I will review this part. Regards, Lorenzo > > Thanks, > Han [0] https://patchwork.ozlabs.org/project/ovn/patch/f31fb6630039c9b310b7fa489247d354f8a46719.1710257650.git.lorenzo.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
