From owner-svn-src-head@FreeBSD.ORG Sun Mar 7 15:33:51 2010 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id BE20D106566C; Sun, 7 Mar 2010 15:33:51 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id 60F638FC0C; Sun, 7 Mar 2010 15:33:50 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 682E6730A1; Sun, 7 Mar 2010 16:43:20 +0100 (CET) Date: Sun, 7 Mar 2010 16:43:20 +0100 From: Luigi Rizzo To: "Bjoern A. Zeeb" Message-ID: <20100307154320.GA17292@onelab2.iet.unipi.it> References: <201003062127.o26LRQ6J042057@svn.freebsd.org> <20100307150909.GA6554@muon.cran.org.uk> <20100307151125.P33454@maildrop.int.zabbadoz.net> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20100307151125.P33454@maildrop.int.zabbadoz.net> User-Agent: Mutt/1.4.2.3i Cc: Bruce Cran , svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r204808 - head/sys/net X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 07 Mar 2010 15:33:51 -0000 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 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