svn commit: r204808 - head/sys/net

[Luigi Rizzo]

On Sun, Mar 07, 2010 at 03:15:28PM +0000, Bjoern A. Zeeb wrote:
> On Sun, 7 Mar 2010, Bruce Cran wrote:
> 
> >On Sat, Mar 06, 2010 at 09:27:26PM +0000, Bjoern A. Zeeb wrote:
> >>Author: bz
> >>Date: Sat Mar  6 21:27:26 2010
> >>New Revision: 204808
> >>URL: http://svn.freebsd.org/changeset/base/204808
> >>
> >>Log:
> >>  Introduce a function rn_detachhead() that will free the
> >>  radix table root nodes.  This is only needed (and available)
> >>  in the virtualization case to free the resources when tearing
> >>  down a virtual network stack.
> >>
> >>  Sponsored by:	ISPsystem
> >>  Reviewed by:	julian, zec
> >>  MFC after:	5 days
> >>
> >>Modified:
> >>  head/sys/net/radix.c
> >>  head/sys/net/radix.h
> >>
> >>Modified: head/sys/net/radix.c
> >>==============================================================================
> >>--- head/sys/net/radix.c	Sat Mar  6 21:24:32 2010	(r204807)
> >>+++ head/sys/net/radix.c	Sat Mar  6 21:27:26 2010	(r204808)
> >>@@ -1161,6 +1161,24 @@ rn_inithead(head, off)
> >> 	return (1);
> >> }
> >>
> >>+#ifdef VIMAGE
> >>+int
> >>+rn_detachhead(void **head)
> >>+{
> >>+	struct radix_node_head *rnh;
> >>+
> >>+	KASSERT((head != NULL && *head != NULL),
> >>+	    ("%s: head already freed", __func__));
> >>+	rnh = *head;
> >>+
> >>+	/* Free <left,root,right> nodes. */
> >>+	Free(rnh);
> >>+
> >>+	*head = NULL;
> >>+	return (1);
> >>+}
> >>+#endif
> >
> >Is this sufficient to free all the memory? From what I can see, 'Free' is
> >just freeing the pointer and not walking the tree to free the nodes too.
> >
> >I don't know if the memory allocation is being done differently, but I
> >fixed the same issue on Windows by introducing a 'rn_free_subtree'
> >function - I don't know if it's entirely correct but the code can be
> >seen at 
> >http://www.bluestop.org/viewvc/repos/sctpDrv/net/radix.c?r1=24&r2=23&pathrev=24
> >
> >I also found that rn_zeros wasn't being freed when the driver got
> >unloaded.
> 
> You will notice that it's not called from anywhere yet;)
> 
> I have another dozen of patches to fix various places and free things.
> Freeing of the tree is (will be) done from route.c (from my memory).
> 
> I am just flushing the "easy" ones from my patch queue while slowly
> reaching timeout for the others that I had sent out for review.
> In case you want to have a look at the complete set send me a mail to
> bz@ and I'll forward you the remaining set.

given that you are looking at the radix.c code, here is one thing
to figure out and either document or fix:

the code uses a number of static variables:

	static int      max_keylen;
	static struct radix_mask *rn_mkfreelist;
	static struct radix_node_head *mask_rnhead;
	static char *rn_zeros, *rn_ones, *addmask_key;

and I am not quite sure of how many of them are subject to races.
I managed to understand (and document) that max_keylen, rn_zeros
and rn_ones are safe because they are effectively readonly.

But the remaining two look a bit dangerous to me, and i have no
idea where and how they get used in our code.

The radix code is used for ipfw "tables", which are not protected
by the same lock as the routing code. Also what about different
vimages, do they run multiple instances of the routing code ?

cheers
luigi