On 2/24/25 4:23 PM, Felix Huettner via dev wrote:
> This adds initial incremental support for the new group_ecmp_route
> engine. It only supports incremental processing for learned routes for
> now as other route types miss the incremental handling up the chain.
>
> Signed-off-by: Felix Huettner <felix.huettner@stackit.cloud>
> ---
Hi Felix,
There's only one problem with this patch, a memory leak, the rest looks
good to me.
> +static bool
> +handle_deleted_route(struct group_ecmp_route_data *data,
> + const struct parsed_route *pr,
> + struct hmapx *updated_routes)
> +{
> + struct group_ecmp_datapath *node = group_ecmp_datapath_lookup(data,
> + pr->od);
> + if (!node) {
> + /* This should not happen since we should know the datapath. */
> + return false;
> + }
> +
> + const struct parsed_route *existing = unique_routes_remove(node, pr);
> + if (!existing) {
> + /* The route must be part of an ecmp group. */
> + if (pr->source == ROUTE_SOURCE_CONNECTED) {
> + /* Connected routes are never part of an ecmp group.
> + * We should recompute. */
> + return false;
> + }
> +
> + struct ecmp_groups_node *eg = ecmp_groups_find(node, pr);
> + if (!eg) {
> + /* We neither found the route as unique nor as ecmp group.
> + * We should recompute. */
> + return false;
> + }
> +
> + size_t ecmp_members = ovs_list_size(&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);
> + ecmp_groups_node_free(eg);
> + } else if (ecmp_members == 2) {
> + /* There is only one other member. If it does not have
> + * ecmp_symmetric_reply configured, we convert it to a
> + * unique route. Otherwise it stays an ecmp group with just one
> + * member. */
> + ecmp_groups_remove_route(eg, pr);
> + 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);
> + unique_routes_add(node, er->route);
> + hmap_remove(&node->ecmp_groups, &eg->hmap_node);
> + ecmp_groups_node_free(eg);
> + }
> + } else {
> + /* We can just remove the member from the group. We need to
> update
> + * the indices of all routes so that future insertions directly
> + * have a new index. */
> + ecmp_groups_remove_route(eg, pr);
> + ecmp_group_update_ids(eg);
> + }
> +
Nit: no need for an empty line here.
> + }
> +
> + hmapx_add(updated_routes, node);
> + return true;
> +}
> +
> +bool
> +group_ecmp_route_learned_route_change_handler(struct engine_node *eng_node,
> + void *_data)
> +{
> + struct group_ecmp_route_data *data = _data;
> + struct learned_route_sync_data *learned_route_data
> + = engine_get_input_data("learned_route_sync", eng_node);
> +
> + if (!learned_route_data->tracked) {
> + data->tracked = false;
> + return false;
> + }
> +
> + data->tracked = true;
> +
> + struct hmapx updated_routes = HMAPX_INITIALIZER(&updated_routes);
> +
> + const struct hmapx_node *hmapx_node;
> + const struct parsed_route *pr;
> + HMAPX_FOR_EACH (hmapx_node,
> + &learned_route_data->trk_data.trk_deleted_parsed_route) {
> + pr = hmapx_node->data;
> + if (!handle_deleted_route(data, pr, &updated_routes)) {
We leak updated_routes here.
> + return false;
> + }
> + }
> +
> + HMAPX_FOR_EACH (hmapx_node,
> + &learned_route_data->trk_data.trk_created_parsed_route) {
> + pr = hmapx_node->data;
> + handle_added_route(data, pr, &updated_routes);
> + }
> +
> + /* Now we need to group the route_nodes based on if there are any routes
> + * left. */
> + HMAPX_FOR_EACH (hmapx_node, &updated_routes) {
> + struct group_ecmp_datapath *node = hmapx_node->data;
> + if (hmap_is_empty(&node->unique_routes) &&
> + hmap_is_empty(&node->ecmp_groups)) {
> + hmapx_add(&data->trk_data.deleted_datapath_routes, node);
> + hmap_remove(&data->datapaths, &node->hmap_node);
> + } else {
> + hmapx_add(&data->trk_data.crupdated_datapath_routes, node);
> + }
> + }
> +
> + hmapx_destroy(&updated_routes);
> +
> + if (!(hmapx_is_empty(&data->trk_data.crupdated_datapath_routes) &&
> + hmapx_is_empty(&data->trk_data.deleted_datapath_routes))) {
> + engine_set_node_state(eng_node, EN_UPDATED);
> + }
> + return true;
> +}
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
