[PATCH] Babel: Remove unecessary FIB_ITERATE restart

[Ondrej Zajicek]

On Mon, Jan 30, 2023 at 11:07:59PM +0100, dxld at darkboxed.org wrote:
> Hi Toke,
> 
> On Mon, Jan 30, 2023 at 10:50:14PM +0100, Toke Høiland-Jørgensen wrote:
> > Daniel Gröber <dxld at darkboxed.org> writes:
> > 
> > > The route expiration code appears to have been stolen from rip.c, in that
> > > code the rt_notify function actually does modify the rtable fib by calling
> > > fib_get. The babel code however does no such thing, so this inefficient
> > > restart is just entirely uneccesarry.
> > 
> > Erm, huh? The babel rt_notify() function calls babel_get_entry(), which
> > calls fib_get()? It's quite similar to the rip one, in fact (which, as
> > you note, is no coincidence).
> 
> Rats, looks like I missed that codepath in my review. Interesting that my
> test code never blew up because of it though.
> 
> Looking at it again it should blow up when rt_notify gets a route that's
> announced from another protocol, not babel, as babel_announce_rte will
> always already have a babel_entry in hand.
> 
> Is it perhaps that the problematic sync callchain only happens with babel
> routes and so we always already have an entry?

The general reason to use FIB_ITERATE_PUT() here is that we are leaving
the local code path, going to completely different module (nest) that may
do callbacks back to Babel code. These callbacks may or may not use/modify
Babel FIB table, but assuming they would not and keeping the data structure
'dirty' (with active iterators) makes the code super brittle and even if
done correctly it may break later during some refactorization.


> > More to the point, what problem are you really trying to solve here? Did
> > you observe any particular performance issue with the current code, for
> > instance?
> 
> With my recent "announce all possible routes to the core" changes I'm
> concerned about cubic blowup here. Since now every expired route (not just
> the selected one) causes a restart.

Are you aware that it is not really restart, as FIB_ITERATE_START()
continues from last stored FIB_ITERATE_PUT() position, so it is only
a restart for that babel_entry?

-- 
Elen sila lumenn' omentielvo

Ondrej 'Santiago' Zajicek (email: santiago at crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."