Mailing List Archive

On Wed, 24 Sep 2003, Gilad Arnold wrote:

> Hi,
>
> I believe that this fixes a "reference leak" (not really a memory leak,
> but rather an inaccurate management of route node reference counters
> that may cause excess memory consumption and possible inconsistencies --
> counter wrap-around, etc). Patch is against today's CVS snapshot.
>
> I'll appreciate some feedback on this.

Index: zebra/zebra_rib.c
===================================================================
RCS file: /var/cvsroot/quagga/zebra/zebra_rib.c,v
retrieving revision 1.11
diff -c -u -r1.11 zebra_rib.c
--- zebra/zebra_rib.c 15 Jul 2003 12:52:22 -0000 1.11
+++ zebra/zebra_rib.c 24 Sep 2003 08:43:30 -0000
@@ -1575,11 +1575,14 @@
rn->info = si->next;
if (si->next)
si->next->prev = si->prev;
+ route_unlock_node (rn);

/* Free static route configuration. */
if (ifname)
XFREE (0, si->gate.ifname);
XFREE (MTYPE_STATIC_IPV4, si);
+
+ route_unlock_node (rn);

return 1;
}

interesting.. why is it locked twice?

on a more abstract, long-term, wishful-thinking vain: I'd love to
eradicate a lot of the implicit locking which is done in table.c. Let
the caller decide, something worth doing?

> Thanks,
> Gilad
>
>
> (PS: it holds for zebra code as well, patching is trivial)

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
"I'd crawl over an acre of 'Visual This++' and 'Integrated Development
That' to get to gcc, Emacs, and gdb. Thank you."
(By Vance Petree, Virginia Power)

Hi Paul,


Paul Jakma wrote:

> Index: zebra/zebra_rib.c
> ===================================================================
> RCS file: /var/cvsroot/quagga/zebra/zebra_rib.c,v
> retrieving revision 1.11
> diff -c -u -r1.11 zebra_rib.c
> --- zebra/zebra_rib.c 15 Jul 2003 12:52:22 -0000 1.11
> +++ zebra/zebra_rib.c 24 Sep 2003 08:43:30 -0000
> @@ -1575,11 +1575,14 @@
> rn->info = si->next;
> if (si->next)
> si->next->prev = si->prev;
> + route_unlock_node (rn);
>
> /* Free static route configuration. */
> if (ifname)
> XFREE (0, si->gate.ifname);
> XFREE (MTYPE_STATIC_IPV4, si);
> +
> + route_unlock_node (rn);
>
> return 1;
> }
>
> interesting.. why is it locked twice?

Once due to route_node_lookup(), which locks the matching node before
returning it; twice because you remove an element (static route, in this
case) from the node's info structure. The unlock calls I've added refer
to the latter, then the former, in that order (you could of course bunch
them up before the return of the function, I figured it's more readable
this way though). Basically I've just reversed the effect of
static_add_ipv4().

Does it make sense to you, or is there anything else you see?


> on a more abstract, long-term, wishful-thinking vain: I'd love to
> eradicate a lot of the implicit locking which is done in table.c. Let
> the caller decide, something worth doing?

I don't really agree: this locking may seem too strict from time to
time, however it does assure you that once you've grabbed a reference to
some route node, by some call to the table lib, it can't be invalidated
until you explicitly let go of it. Why would you want to do that?

(And, how's everything? ;-> I seem to have paused my work on recursive
multihop for the time being, I'm extremely overloaded these days,
apologies for that...)

Gilad

On Wed, 24 Sep 2003, Gilad Arnold wrote:

> Hi Paul,

> Once due to route_node_lookup(), which locks the matching node
> before returning it;

arg yes.. this is exactly why i hate the hidden locking.

> twice because you remove an element (static route, in this case)
> from the node's info structure. The unlock calls I've added refer
> to the latter, then the former, in that order (you could of course
> bunch them up before the return of the function, I figured it's
> more readable this way though). Basically I've just reversed the
> effect of static_add_ipv4().

makes perfect sense.

> Does it make sense to you, or is there anything else you see?

your the zebra_rib.c guru :) - makes perfect sense anyway afaict.

> I don't really agree: this locking may seem too strict from time to
> time, however it does assure you that once you've grabbed a
> reference to some route node, by some call to the table lib, it
> can't be invalidated until you explicitly let go of it. Why would
> you want to do that?

Cause its hidden. See above :)

> (And, how's everything? ;-> I seem to have paused my work on
> recursive multihop for the time being, I'm extremely overloaded
> these days, apologies for that...)

no worries. same for everyone i think. so many things to do.. so
little time... - same story everywhere.

> Gilad

regards,
--
Paul Jakma paul@clubi.ie paul@jakma.org Key ID: 64A2FF6A
warning: do not ever send email to spam@dishone.st
Fortune:
In 1750 Issac Newton became discouraged when he fell up a flight of stairs.