| Message ID | 7ca930ff80e1a7be2eb36a84ea0ff9731bc22c92.1732630355.git.felix.huettner@stackit.cloud |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | OVN Fabric integration | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | fail | apply and check: fail |
On 11/26/24 3:37 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 <felix.huettner@stackit.cloud> > --- Hi Felix, This needs a rebase since https://github.com/ovn-org/ovn/commit/84030e9 was merged. We probably need to move some of the cleanup logic from there into parsed_route_free(). > v2->v3: updated based on comments > v1->v2: > * fix memory leak > * fix incorrect comparison of struct parsed_route > > lib/ovn-util.h | 17 ++++++++++ > northd/northd.c | 84 ++++++++++++++++++++++++------------------------- > northd/northd.h | 2 ++ > 3 files changed, 61 insertions(+), 42 deletions(-) > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > index 7b98b9b9a..3f956fb80 100644 > --- a/lib/ovn-util.h > +++ b/lib/ovn-util.h > @@ -484,4 +484,21 @@ void ovn_exit_args_finish(struct ovn_exit_args *exit_args); > bool ovn_update_swconn_at(struct rconn *swconn, const char *target, > int probe_interval, const char *where); > > +/* Return true if both strings are equal to each other. > + * In contrast to checking with strcmp NULL and "" also count as equal */ > +static inline bool > +streq(const char *s1, const char *s2) > +{ > + bool s1_empty = !s1 || strlen(s1) == 0; > + bool s2_empty = !s2 || strlen(s2) == 0; > + if (s1_empty != s2_empty) { > + return false; > + } > + if (s1_empty) { > + return true; > + } > + return !strcmp(s1, s2); > +} Is this better? return !strcmp(s1 ? s1 : "", s2 ? s2 : ""); However, do we really need this new helper? Can't we change the code so we're always consistent and not have to handle NULL as ""? We have nullable_string_is_equal() for those cases. Thanks, Dumitru > + > + > #endif /* OVN_UTIL_H */ > diff --git a/northd/northd.c b/northd/northd.c > index 8ba7b8c33..552e471b5 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -11066,12 +11066,26 @@ parsed_route_lookup(struct hmap *routes, size_t hash, > continue; > } > > + if (pr->out_port != new_pr->out_port) { > + continue; > + } > + > + if (!streq(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); > + free(pr); > +} > + > static void > parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > const struct nbrec_logical_router_static_route *route, > @@ -11125,10 +11139,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; > } > > @@ -11175,6 +11191,10 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > "ecmp_symmetric_reply", > false); > new_pr->is_discard_route = is_discard_route; > + 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); > @@ -11182,7 +11202,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > hmap_insert(routes, &new_pr->key_node, hash); > } else { > pr->stale = false; > - free(new_pr); > + parsed_route_free(new_pr); > } > } > > @@ -11211,7 +11231,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); > } > } > > @@ -11463,7 +11483,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) > @@ -11559,8 +11579,7 @@ 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) > + struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref) > > { > bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&eg->prefix); > @@ -11608,20 +11627,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); > } > @@ -11638,9 +11651,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); > @@ -11720,35 +11733,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); > } > @@ -13523,7 +13523,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) > { > @@ -13572,11 +13572,11 @@ 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); > + build_ecmp_route_flow(lflows, od, group, lflow_ref); > } > 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); > @@ -17062,7 +17062,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, > @@ -17653,7 +17653,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); > - > + > 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, > @@ -18833,7 +18833,7 @@ static_routes_destroy(struct static_routes_data *data) > { > struct parsed_route *r; > HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) { > - free(r); > + parsed_route_free(r); > } > hmap_destroy(&data->parsed_routes); > > diff --git a/northd/northd.h b/northd/northd.h > index 8f76d642d..3e22e4f14 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -707,6 +707,8 @@ struct parsed_route { > bool is_discard_route; > const struct nbrec_logical_router *nbr; > bool stale; > + char *lrp_addr_s; > + const struct ovn_port *out_port; > }; > > void ovnnb_db_run(struct northd_input *input_data,
On Mon, Dec 02, 2024 at 11:11:16AM +0100, Dumitru Ceara wrote: > On 11/26/24 3:37 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 <felix.huettner@stackit.cloud> > > --- > > Hi Felix, > > This needs a rebase since https://github.com/ovn-org/ovn/commit/84030e9 > was merged. We probably need to move some of the cleanup logic from > there into parsed_route_free(). Hi Dimitru, yes thanks. > > > v2->v3: updated based on comments > > v1->v2: > > * fix memory leak > > * fix incorrect comparison of struct parsed_route > > > > lib/ovn-util.h | 17 ++++++++++ > > northd/northd.c | 84 ++++++++++++++++++++++++------------------------- > > northd/northd.h | 2 ++ > > 3 files changed, 61 insertions(+), 42 deletions(-) > > > > diff --git a/lib/ovn-util.h b/lib/ovn-util.h > > index 7b98b9b9a..3f956fb80 100644 > > --- a/lib/ovn-util.h > > +++ b/lib/ovn-util.h > > @@ -484,4 +484,21 @@ void ovn_exit_args_finish(struct ovn_exit_args *exit_args); > > bool ovn_update_swconn_at(struct rconn *swconn, const char *target, > > int probe_interval, const char *where); > > > > +/* Return true if both strings are equal to each other. > > + * In contrast to checking with strcmp NULL and "" also count as equal */ > > +static inline bool > > +streq(const char *s1, const char *s2) > > +{ > > + bool s1_empty = !s1 || strlen(s1) == 0; > > + bool s2_empty = !s2 || strlen(s2) == 0; > > + if (s1_empty != s2_empty) { > > + return false; > > + } > > + if (s1_empty) { > > + return true; > > + } > > + return !strcmp(s1, s2); > > +} > > Is this better? > > return !strcmp(s1 ? s1 : "", s2 ? s2 : ""); > > However, do we really need this new helper? Can't we change the code so > we're always consistent and not have to handle NULL as ""? We have > nullable_string_is_equal() for those cases. thanks a lot, i just did not knew this existed. Will change it in the next version. > > Thanks, > Dumitru > > > + > > + > > #endif /* OVN_UTIL_H */ > > diff --git a/northd/northd.c b/northd/northd.c > > index 8ba7b8c33..552e471b5 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -11066,12 +11066,26 @@ parsed_route_lookup(struct hmap *routes, size_t hash, > > continue; > > } > > > > + if (pr->out_port != new_pr->out_port) { > > + continue; > > + } > > + > > + if (!streq(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); > > + free(pr); > > +} > > + > > static void > > parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > > const struct nbrec_logical_router_static_route *route, > > @@ -11125,10 +11139,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; > > } > > > > @@ -11175,6 +11191,10 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > > "ecmp_symmetric_reply", > > false); > > new_pr->is_discard_route = is_discard_route; > > + 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); > > @@ -11182,7 +11202,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, > > hmap_insert(routes, &new_pr->key_node, hash); > > } else { > > pr->stale = false; > > - free(new_pr); > > + parsed_route_free(new_pr); > > } > > } > > > > @@ -11211,7 +11231,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); > > } > > } > > > > @@ -11463,7 +11483,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) > > @@ -11559,8 +11579,7 @@ 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) > > + struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref) > > > > { > > bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&eg->prefix); > > @@ -11608,20 +11627,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); > > } > > @@ -11638,9 +11651,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); > > @@ -11720,35 +11733,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); > > } > > @@ -13523,7 +13523,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) > > { > > @@ -13572,11 +13572,11 @@ 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); > > + build_ecmp_route_flow(lflows, od, group, lflow_ref); > > } > > 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); > > @@ -17062,7 +17062,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, > > @@ -17653,7 +17653,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); > > - > > + > > 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, > > @@ -18833,7 +18833,7 @@ static_routes_destroy(struct static_routes_data *data) > > { > > struct parsed_route *r; > > HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) { > > - free(r); > > + parsed_route_free(r); > > } > > hmap_destroy(&data->parsed_routes); > > > > diff --git a/northd/northd.h b/northd/northd.h > > index 8f76d642d..3e22e4f14 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -707,6 +707,8 @@ struct parsed_route { > > bool is_discard_route; > > const struct nbrec_logical_router *nbr; > > bool stale; > > + char *lrp_addr_s; > > + const struct ovn_port *out_port; > > }; > > > > void ovnnb_db_run(struct northd_input *input_data, > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/ovn-util.h b/lib/ovn-util.h index 7b98b9b9a..3f956fb80 100644 --- a/lib/ovn-util.h +++ b/lib/ovn-util.h @@ -484,4 +484,21 @@ void ovn_exit_args_finish(struct ovn_exit_args *exit_args); bool ovn_update_swconn_at(struct rconn *swconn, const char *target, int probe_interval, const char *where); +/* Return true if both strings are equal to each other. + * In contrast to checking with strcmp NULL and "" also count as equal */ +static inline bool +streq(const char *s1, const char *s2) +{ + bool s1_empty = !s1 || strlen(s1) == 0; + bool s2_empty = !s2 || strlen(s2) == 0; + if (s1_empty != s2_empty) { + return false; + } + if (s1_empty) { + return true; + } + return !strcmp(s1, s2); +} + + #endif /* OVN_UTIL_H */ diff --git a/northd/northd.c b/northd/northd.c index 8ba7b8c33..552e471b5 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -11066,12 +11066,26 @@ parsed_route_lookup(struct hmap *routes, size_t hash, continue; } + if (pr->out_port != new_pr->out_port) { + continue; + } + + if (!streq(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); + free(pr); +} + static void parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, const struct nbrec_logical_router_static_route *route, @@ -11125,10 +11139,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; } @@ -11175,6 +11191,10 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, "ecmp_symmetric_reply", false); new_pr->is_discard_route = is_discard_route; + 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); @@ -11182,7 +11202,7 @@ parsed_routes_add(struct ovn_datapath *od, const struct hmap *lr_ports, hmap_insert(routes, &new_pr->key_node, hash); } else { pr->stale = false; - free(new_pr); + parsed_route_free(new_pr); } } @@ -11211,7 +11231,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); } } @@ -11463,7 +11483,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) @@ -11559,8 +11579,7 @@ 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) + struct ecmp_groups_node *eg, struct lflow_ref *lflow_ref) { bool is_ipv4 = IN6_IS_ADDR_V4MAPPED(&eg->prefix); @@ -11608,20 +11627,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); } @@ -11638,9 +11651,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); @@ -11720,35 +11733,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); } @@ -13523,7 +13523,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) { @@ -13572,11 +13572,11 @@ 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); + build_ecmp_route_flow(lflows, od, group, lflow_ref); } 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); @@ -17062,7 +17062,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, @@ -17653,7 +17653,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); - + 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, @@ -18833,7 +18833,7 @@ static_routes_destroy(struct static_routes_data *data) { struct parsed_route *r; HMAP_FOR_EACH_POP (r, key_node, &data->parsed_routes) { - free(r); + parsed_route_free(r); } hmap_destroy(&data->parsed_routes); diff --git a/northd/northd.h b/northd/northd.h index 8f76d642d..3e22e4f14 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -707,6 +707,8 @@ struct parsed_route { bool is_discard_route; const struct nbrec_logical_router *nbr; bool stale; + char *lrp_addr_s; + const struct ovn_port *out_port; }; void ovnnb_db_run(struct northd_input *input_data,
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 <felix.huettner@stackit.cloud> --- v2->v3: updated based on comments v1->v2: * fix memory leak * fix incorrect comparison of struct parsed_route lib/ovn-util.h | 17 ++++++++++ northd/northd.c | 84 ++++++++++++++++++++++++------------------------- northd/northd.h | 2 ++ 3 files changed, 61 insertions(+), 42 deletions(-)