On Wed, Apr 30, 2025 at 7:54 AM Ales Musil <amu...@redhat.com> wrote:
> Replace route list with vector. The change allows us to avoid small
> allocations just to keep to have the list reference. The vector can
> store elements directly and allocates in batches rather than one by
> one. This also simplifies the code to some extent.
>
> Signed-off-by: Ales Musil <amu...@redhat.com>
> Acked-by: Mark Michelson <mmich...@redhat.com>
> ---
> v4: Rebase on top of current main.
> Add ack from Mark.
> v3: Rebase on top of current main.
> v2: Rebase on top of current main.
> ---
> northd/en-group-ecmp-route.c | 38 ++++++++++++++++--------------------
> northd/en-group-ecmp-route.h | 4 ++--
> northd/northd.c | 11 +++++------
> 3 files changed, 24 insertions(+), 29 deletions(-)
>
> diff --git a/northd/en-group-ecmp-route.c b/northd/en-group-ecmp-route.c
> index c55bbdd8b..ee1297947 100644
> --- a/northd/en-group-ecmp-route.c
> +++ b/northd/en-group-ecmp-route.c
> @@ -39,11 +39,7 @@ ecmp_groups_node_free(struct ecmp_groups_node *eg)
> return;
> }
>
> - struct ecmp_route_list_node *er;
> - LIST_FOR_EACH_SAFE (er, list_node, &eg->route_list) {
> - ovs_list_remove(&er->list_node);
> - free(er);
> - }
> + vector_destroy(&eg->route_list);
> sset_destroy(&eg->selection_fields);
> free(eg);
> }
> @@ -218,9 +214,10 @@ ecmp_groups_add_route(struct ecmp_groups_node *group,
> return;
> }
>
> - struct ecmp_route_list_node *er = xmalloc(sizeof *er);
> - er->route = route;
> - er->id = ++group->route_count;
> + struct ecmp_route_list_node er = (struct ecmp_route_list_node) {
> + .route = route,
> + .id = ++group->route_count,
> + };
>
> if (group->route_count == 1) {
> sset_clone(&group->selection_fields,
> &route->ecmp_selection_fields);
> @@ -229,7 +226,7 @@ ecmp_groups_add_route(struct ecmp_groups_node *group,
> &route->ecmp_selection_fields);
> }
>
> - ovs_list_insert(&group->route_list, &er->list_node);
> + vector_push(&group->route_list, &er);
> }
>
> /* Removes a route from an ecmp group. If the ecmp group should persist
> @@ -240,13 +237,14 @@ ecmp_groups_remove_route(struct ecmp_groups_node
> *group,
> const struct parsed_route *pr)
> {
> struct ecmp_route_list_node *er;
> - LIST_FOR_EACH (er, list_node, &group->route_list) {
> + size_t i = 0;
> + VECTOR_FOR_EACH_PTR (&group->route_list, er) {
> if (er->route == pr) {
> const struct parsed_route *found_route = er->route;
> - ovs_list_remove(&er->list_node);
> - free(er);
> + vector_remove_fast(&group->route_list, i, NULL);
> return found_route;
> }
> + i++;
> }
>
> return NULL;
> @@ -257,9 +255,8 @@ ecmp_group_update_ids(struct ecmp_groups_node *group)
> {
> struct ecmp_route_list_node *er;
> size_t i = 0;
> - LIST_FOR_EACH (er, list_node, &group->route_list) {
> - er->id = i;
> - i++;
> + VECTOR_FOR_EACH_PTR (&group->route_list, er) {
> + er->id = i++;
> }
> group->route_count = i;
> }
> @@ -283,8 +280,8 @@ ecmp_groups_add(struct group_ecmp_datapath *gn,
> eg->is_src_route = route->is_src_route;
> eg->source = route->source;
> eg->route_table_id = route->route_table_id;
> + eg->route_list = VECTOR_EMPTY_INITIALIZER(struct
> ecmp_route_list_node);
> sset_init(&eg->selection_fields);
> - ovs_list_init(&eg->route_list);
> ecmp_groups_add_route(eg, route);
>
> return eg;
> @@ -311,7 +308,7 @@ static bool
> ecmp_group_has_symmetric_reply(struct ecmp_groups_node *eg)
> {
> struct ecmp_route_list_node *er;
> - LIST_FOR_EACH (er, list_node, &eg->route_list) {
> + VECTOR_FOR_EACH_PTR (&eg->route_list, er) {
> if (er->route->ecmp_symmetric_reply) {
> return true;
> }
> @@ -430,7 +427,7 @@ handle_deleted_route(struct group_ecmp_route_data
> *data,
> return false;
> }
>
> - size_t ecmp_members = ovs_list_size(&eg->route_list);
> + size_t ecmp_members = vector_len(&eg->route_list);
> if (ecmp_members == 1) {
> /* The route is the only ecmp member, we remove the whole
> group. */
> hmap_remove(&node->ecmp_groups, &eg->hmap_node);
> @@ -444,9 +441,8 @@ handle_deleted_route(struct group_ecmp_route_data
> *data,
> if (ecmp_group_has_symmetric_reply(eg)) {
> ecmp_group_update_ids(eg);
> } else {
> - struct ecmp_route_list_node *er = CONTAINER_OF(
> - ovs_list_front(&eg->route_list),
> - struct ecmp_route_list_node, list_node);
> + const struct ecmp_route_list_node *er =
> + vector_get_ptr(&eg->route_list, 0);
> unique_routes_add(node, er->route);
> hmap_remove(&node->ecmp_groups, &eg->hmap_node);
> ecmp_groups_node_free(eg);
> diff --git a/northd/en-group-ecmp-route.h b/northd/en-group-ecmp-route.h
> index b85e6014e..a7040addd 100644
> --- a/northd/en-group-ecmp-route.h
> +++ b/northd/en-group-ecmp-route.h
> @@ -20,10 +20,10 @@
> #include "openvswitch/hmap.h"
> #include "openvswitch/list.h"
> #include "northd/northd.h"
> +#include "vec.h"
> #include <netinet/in.h>
>
> struct ecmp_route_list_node {
> - struct ovs_list list_node;
> uint16_t id; /* starts from 1 */
> const struct parsed_route *route;
> };
> @@ -37,7 +37,7 @@ struct ecmp_groups_node {
> enum route_source source;
> uint32_t route_table_id;
> uint16_t route_count;
> - struct ovs_list route_list; /* Contains ecmp_route_list_node */
> + struct vector route_list; /* Contains ecmp_route_list_node */
> struct sset selection_fields;
> };
>
> diff --git a/northd/northd.c b/northd/northd.c
> index 9a49fb8f1..c20ddb786 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -11941,7 +11941,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> {
> bool is_ipv4_prefix = IN6_IS_ADDR_V4MAPPED(&eg->prefix);
> uint16_t priority;
> - struct ecmp_route_list_node *er;
> + const struct ecmp_route_list_node *er;
> struct ds route_match = DS_EMPTY_INITIALIZER;
>
> char *prefix_s = build_route_prefix_s(&eg->prefix, eg->plen);
> @@ -11955,7 +11955,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> ds_put_format(&actions, "ip.ttl--; flags.loopback = 1; %s = %"PRIu16
> "; %s = ", REG_ECMP_GROUP_ID, eg->id,
> REG_ECMP_MEMBER_ID);
>
> - if (!ovs_list_is_singleton(&eg->route_list)) {
> + if (vector_len(&eg->route_list) > 1) {
> if (protocol && !sset_is_empty(&eg->selection_fields)) {
> ds_put_format(&route_match, " && %s", protocol);
> }
> @@ -11963,7 +11963,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> struct ds values = DS_EMPTY_INITIALIZER;
> bool is_first = true;
>
> - LIST_FOR_EACH (er, list_node, &eg->route_list) {
> + VECTOR_FOR_EACH_PTR (&eg->route_list, er) {
> if (is_first) {
> is_first = false;
> } else {
> @@ -11997,8 +11997,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> ds_put_cstr(&actions, ");");
> ds_destroy(&values);
> } else {
> - er = CONTAINER_OF(ovs_list_front(&eg->route_list),
> - struct ecmp_route_list_node, list_node);
> + er = vector_get_ptr(&eg->route_list, 0);
> ds_put_format(&actions, "%"PRIu16"; next;", er->id);
> }
>
> @@ -12009,7 +12008,7 @@ build_ecmp_route_flow(struct lflow_table *lflows,
> /* Add per member flow */
> struct ds match = DS_EMPTY_INITIALIZER;
> struct sset visited_ports = SSET_INITIALIZER(&visited_ports);
> - LIST_FOR_EACH (er, list_node, &eg->route_list) {
> + VECTOR_FOR_EACH_PTR (&eg->route_list, er) {
> const struct parsed_route *route = er->route;
> bool is_ipv4_nexthop = IN6_IS_ADDR_V4MAPPED(route->nexthop);
> /* Symmetric ECMP reply is only usable on gateway routers.
> --
> 2.49.0
>
>
Recheck-request: github-robot
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
