[PATCH 1/2] Babel: Add option to support ecmp routes

[Toke Høiland-Jørgensen]

dxld at darkboxed.org writes:

> Hi Toke,
>
> On Mon, May 09, 2022 at 03:21:15PM +0200, Toke Høiland-Jørgensen wrote:
>> >> > +  /* Update all routes to refresh ecmp settings. */
>> >> > +  FIB_WALK(&p->ip6_rtable, struct babel_entry, e) {
>> >> > +    babel_select_route(p, e, NULL);
>> >> > +  }
>> >> > +  FIB_WALK_END;
>> >> > +  FIB_WALK(&p->ip4_rtable, struct babel_entry, e) {
>> >> > +    babel_select_route(p, e, NULL);
>> >> > +  }
>> >> > +  FIB_WALK_END;
>> >> > +
>> >> 
>> >> I don't think this is the right thing to do; you should probably just
>> >> refuse the reconfiguration if ECMP settings changed (see the 0-return at
>> >> the top when TOS changes)?
>> >
>> > Why not? Works fine AFAICT. I tested the reload stuff and the nexthops get
>> > updated when the limit changes just as I would expect.
>> 
>> Because walking the FIB like this is an ugly hack (as you yourself said
>> in the description)? 
>
> Well I really just meant inlining the interations, I hadn't though about
> the iterator invalidation ascpect actually.
>
>> > If I refuse the reconfiguration the user would have to restart bird to
>> > get this to apply, right? I don't want that.
>> 
>> No, if you refuse reconfig, bird will automatically bring the interface
>> down, then up again. This will flush all the neighbours, but I think
>> that's OK; switching ECMP on and off doesn't have to be a
>> non-destructive operation.
>
> Hmm, that might be acceptable. Given that I might need a babel_route->fib
> mapping anyway (see below) this might end up coming for free though.
>
>> Oh, and when looking at this, I noticed that babel_retract_route() only
>> conditionally calls babel_select_route(); this is the case in quite a
>> few places, actually, you'd have to update all of them.
>
> Right to do babel_route removal efficiently I'll probably need some more
> machinery to keep track of which fib route/nexhop a given babel_route maps
> to.
>
> Just iterating over all fib routes to remove one babel_route is definetly
> too inefficient.

I think what you need to do is to add something that's equivalent to
'babel_entry->selected' but for multiple 'babel_route' objects. So that
everywhere there's a 'r == e->selected' check you can do
'babel_is_selected(e, r)', and that will check membership of that set.

One option could just be to add a boolean flag to struct babel_route
'is_selected' which will be set for all routes that are part of an ECMP
route; and make sure babel_select_route() clears that flag for all
routes it does not add as nexthops. Then you can just check that field
all the places where there's a conditional call to babel_select_route();
you don't need to do any separate fib walking apart from what is already
done to expire routes...

-Toke