On Tue, Feb 25, 2025 at 02:00:43PM +0100, Dumitru Ceara wrote:
> 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.
Hi Dumitru,
thanks a lot for the catch.
I'll fix it in v4.
Thanks,
Felix
>
> > +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
