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.
--Daniel
