On 12/12/24 4:52 PM, Felix Huettner via dev wrote:
> We already determine the outport of a parsed_route in parsed_routes_add.
> However we previously ignored it and recalculated it later on.
>
> Now we just store it for later usage.
>
> Signed-off-by: Felix Huettner <[email protected]>
> ---
> v1->v2: fix potential for doublefree of ecmp_selection_fields
>
> northd/northd.c | 99 ++++++++++++++++++++++++-------------------------
> northd/northd.h | 2 +
> 2 files changed, 51 insertions(+), 50 deletions(-)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 665277d50..48f4869f7 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11135,12 +11135,28 @@ parsed_route_lookup(struct hmap *routes, size_t
> hash,
> continue;
> }
>
> + if (pr->out_port != new_pr->out_port) {
> + continue;
> + }
> +
> + if (!nullable_string_is_equal(pr->lrp_addr_s,
> + new_pr->lrp_addr_s)) {
> + continue;
> + }
> +
> return pr;
> }
>
> return NULL;
> }
>
> +static void
> +parsed_route_free(struct parsed_route *pr) {
> + free(pr->lrp_addr_s);
> + sset_destroy(&pr->ecmp_selection_fields);
> + free(pr);
> +}
> +
> static void
> parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports,
> const struct nbrec_logical_router_static_route *route,
> @@ -11194,10 +11210,12 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> }
>
> /* Verify that ip_prefix and nexthop are on the same network. */
> + const char *lrp_addr_s = NULL;
> + struct ovn_port *out_port = NULL;
> if (!is_discard_route &&
> !find_static_route_outport(od, lr_ports, route,
> IN6_IS_ADDR_V4MAPPED(&prefix),
> - NULL, NULL)) {
> + &lrp_addr_s, &out_port)) {
> return;
> }
>
> @@ -11261,14 +11279,18 @@ parsed_routes_add(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> sset_destroy(&field_set);
> }
>
> + if (!is_discard_route) {
> + new_pr->lrp_addr_s = xstrdup(lrp_addr_s);
> + }
> + new_pr->out_port = out_port;
> +
> 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);
> } else {
> pr->stale = false;
> - sset_destroy(&new_pr->ecmp_selection_fields);
> - free(new_pr);
> + parsed_route_free(new_pr);
> }
> }
>
> @@ -11297,7 +11319,7 @@ build_parsed_routes(struct ovn_datapath *od, const
> struct hmap *lr_ports,
> }
>
> hmap_remove(routes, &pr->key_node);
> - free(pr);
> + parsed_route_free(pr);
> }
> }
>
> @@ -11565,7 +11587,7 @@ static void
> add_ecmp_symmetric_reply_flows(struct lflow_table *lflows,
> struct ovn_datapath *od,
> const char *port_ip,
> - struct ovn_port *out_port,
> + const struct ovn_port *out_port,
> const struct parsed_route *route,
> struct ds *route_match,
> struct lflow_ref *lflow_ref)
> @@ -11661,8 +11683,8 @@ add_ecmp_symmetric_reply_flows(struct lflow_table
> *lflows,
>
> static void
> build_ecmp_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> - const struct hmap *lr_ports, struct ecmp_groups_node
> *eg,
> - struct lflow_ref *lflow_ref, const char *protocol)
> + struct ecmp_groups_node *eg, struct lflow_ref
> *lflow_ref,
> + const char *protocol)
>
> {
> bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&eg->prefix);
> @@ -11739,20 +11761,14 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> struct ovn_datapath *od,
> LIST_FOR_EACH (er, list_node, &eg->route_list) {
> const struct parsed_route *route_ = er->route;
> const struct nbrec_logical_router_static_route *route =
> route_->route;
> - /* Find the outgoing port. */
> - const char *lrp_addr_s = NULL;
> - struct ovn_port *out_port = NULL;
> - if (!find_static_route_outport(od, lr_ports, route, is_ipv4,
> - &lrp_addr_s, &out_port)) {
> - continue;
> - }
> /* Symmetric ECMP reply is only usable on gateway routers.
> * It is NOT usable on distributed routers with a gateway port.
> */
> if (smap_get(&od->nbr->options, "chassis") &&
> route_->ecmp_symmetric_reply && sset_add(&visited_ports,
> - out_port->key)) {
> - add_ecmp_symmetric_reply_flows(lflows, od, lrp_addr_s, out_port,
> + route_->out_port->key))
> {
> + add_ecmp_symmetric_reply_flows(lflows, od, route_->lrp_addr_s,
> + route_->out_port,
> route_, &route_match,
> lflow_ref);
> }
> @@ -11769,9 +11785,9 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> struct ovn_datapath *od,
> is_ipv4 ? REG_NEXT_HOP_IPV4 : REG_NEXT_HOP_IPV6,
> route->nexthop,
> is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6,
> - lrp_addr_s,
> - out_port->lrp_networks.ea_s,
> - out_port->json_key);
> + route_->lrp_addr_s,
> + route_->out_port->lrp_networks.ea_s,
> + route_->out_port->json_key);
> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_IN_IP_ROUTING_ECMP, 100,
> ds_cstr(&match), ds_cstr(&actions),
> &route->header_, lflow_ref);
> @@ -11851,35 +11867,22 @@ add_route(struct lflow_table *lflows, struct
> ovn_datapath *od,
>
> static void
> build_static_route_flow(struct lflow_table *lflows, struct ovn_datapath *od,
> - const struct hmap *lr_ports,
> const struct parsed_route *route_,
> const struct sset *bfd_ports,
> struct lflow_ref *lflow_ref)
> {
> - const char *lrp_addr_s = NULL;
> - struct ovn_port *out_port = NULL;
> -
> const struct nbrec_logical_router_static_route *route = route_->route;
>
> - /* Find the outgoing port. */
> - if (!route_->is_discard_route) {
> - if (!find_static_route_outport(od, lr_ports, route,
> - IN6_IS_ADDR_V4MAPPED(&route_->prefix),
> - &lrp_addr_s, &out_port)) {
> - return;
> - }
> - }
> -
> int ofs = !strcmp(smap_get_def(&route->options, "origin", ""),
> ROUTE_ORIGIN_CONNECTED) ? ROUTE_PRIO_OFFSET_CONNECTED
> : ROUTE_PRIO_OFFSET_STATIC;
>
> char *prefix_s = build_route_prefix_s(&route_->prefix, route_->plen);
> - add_route(lflows, route_->is_discard_route ? od : out_port->od, out_port,
> - lrp_addr_s, prefix_s, route_->plen, route->nexthop,
> - route_->is_src_route, route_->route_table_id,
> - bfd_ports, &route->header_, route_->is_discard_route,
> - ofs, lflow_ref);
> + add_route(lflows, route_->is_discard_route ? od : route_->out_port->od,
> + route_->out_port, route_->lrp_addr_s, prefix_s,
> + route_->plen, route->nexthop, route_->is_src_route,
> + route_->route_table_id, bfd_ports, &route->header_,
> + route_->is_discard_route, ofs, lflow_ref);
>
> free(prefix_s);
> }
> @@ -13742,7 +13745,7 @@ build_ip_routing_flows_for_lrp(struct ovn_port *op,
> static void
> build_static_route_flows_for_lrouter(
> struct ovn_datapath *od, struct lflow_table *lflows,
> - const struct hmap *lr_ports, struct hmap *parsed_routes,
> + struct hmap *parsed_routes,
> struct simap *route_tables, const struct sset *bfd_ports,
> struct lflow_ref *lflow_ref)
> {
> @@ -13791,23 +13794,20 @@ build_static_route_flows_for_lrouter(
> HMAP_FOR_EACH (group, hmap_node, &ecmp_groups) {
> /* add a flow in IP_ROUTING, and one flow for each member in
> * IP_ROUTING_ECMP. */
> - build_ecmp_route_flow(lflows, od, lr_ports, group, lflow_ref, NULL);
> + build_ecmp_route_flow(lflows, od, group, lflow_ref, NULL);
>
> /* If src or dst port is specified for selection_fields, install
> * separate ECMP flows with protocol match of TCP, UDP and SCTP */
> if (sset_contains(&group->selection_fields, "tp_src") ||
> sset_contains(&group->selection_fields, "tp_dst")) {
> - build_ecmp_route_flow(lflows, od, lr_ports, group, lflow_ref,
> - "tcp");
> - build_ecmp_route_flow(lflows, od, lr_ports, group, lflow_ref,
> - "udp");
> - build_ecmp_route_flow(lflows, od, lr_ports, group, lflow_ref,
> - "sctp");
> + build_ecmp_route_flow(lflows, od, group, lflow_ref, "tcp");
> + build_ecmp_route_flow(lflows, od, group, lflow_ref, "udp");
> + build_ecmp_route_flow(lflows, od, group, lflow_ref, "sctp");
> }
> }
> const struct unique_routes_node *ur;
> HMAP_FOR_EACH (ur, hmap_node, &unique_routes) {
> - build_static_route_flow(lflows, od, lr_ports, ur->route,
> + build_static_route_flow(lflows, od, ur->route,
> bfd_ports, lflow_ref);
> }
> ecmp_groups_destroy(&ecmp_groups);
> @@ -17293,7 +17293,7 @@ build_lswitch_and_lrouter_iterate_by_lr(struct
> ovn_datapath *od,
> lsi->meter_groups, NULL);
> build_ND_RA_flows_for_lrouter(od, lsi->lflows, NULL);
> build_ip_routing_pre_flows_for_lrouter(od, lsi->lflows, NULL);
> - build_static_route_flows_for_lrouter(od, lsi->lflows, lsi->lr_ports,
> + build_static_route_flows_for_lrouter(od, lsi->lflows,
> lsi->parsed_routes,
> lsi->route_tables,
> lsi->bfd_ports, NULL);
> build_mcast_lookup_flows_for_lrouter(od, lsi->lflows, &lsi->match,
> @@ -17884,7 +17884,7 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn,
> /* Parallel build may result in a suboptimal hash. Resize the
> * lflow map to a correct size before doing lookups */
> lflow_table_expand(lflows);
> -
> +
Nit: unrelated.
I removed this chunk and applied the patch to main.
Regards,
Dumitru
> stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec());
> lflow_table_sync_to_sb(lflows, ovnsb_txn, input_data->ls_datapaths,
> input_data->lr_datapaths,
> @@ -19028,8 +19028,7 @@ routes_destroy(struct routes_data *data)
> {
> struct parsed_route *r;
> HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) {
> - sset_destroy(&r->ecmp_selection_fields);
> - free(r);
> + parsed_route_free(r);
> }
> hmap_destroy(&data->parsed_routes);
>
> diff --git a/northd/northd.h b/northd/northd.h
> index d60c944df..e6e9868b2 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -708,6 +708,8 @@ struct parsed_route {
> const struct nbrec_logical_router *nbr;
> bool stale;
> struct sset ecmp_selection_fields;
> + char *lrp_addr_s;
> + const struct ovn_port *out_port;
> };
>
> void ovnnb_db_run(struct northd_input *input_data,
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
