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 <[email protected]>
> > ---
>
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
