Subject: Re: [PATCH] lib/idr.c: Zero memory properly in idr_remove_all
From: Kristian =?ISO-8859-1?Q?H=F8gsberg?= <krh@redhat.com>
To: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Andrew Morton <akpm@linux-foundation.org>,
       Manfred Spraul <manfred@colorfullife.com>, dcm@acm.org,
       Nadia.Derbey@bull.net, linux1394-devel@lists.sourceforge.net,
       linux-kernel@vger.kernel.org, paulmck@us.ibm.com
In-Reply-To: <496E111A.9010808@s5r6.in-berlin.de>
References: <1231571060.3538.18.camel@localhost.localdomain>
	 <49686465.70501@s5r6.in-berlin.de>
	 <20090110011557.9d94e111.akpm@linux-foundation.org>
	 <496872E0.9030007@s5r6.in-berlin.de>
	 <1231773620.6365.21.camel@gaara.bos.redhat.com>
	 <496B9FC7.3090108@colorfullife.com>
	 <1231792712.6365.31.camel@gaara.bos.redhat.com>
	 <496BAD1C.5060201@colorfullife.com>
	 <20090113144802.39dc4c23.akpm@linux-foundation.org>
	 <1231942990.8542.7.camel@gaara.bos.redhat.com>
	 <496E111A.9010808@s5r6.in-berlin.de>
Content-Type: text/plain; charset="UTF-8"
Date: Wed, 14 Jan 2009 11:33:22 -0500
Message-Id: <1231950802.9435.4.camel@gaara.bos.redhat.com>
Mime-Version: 1.0
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1438
Lines: 33

On Wed, 2009-01-14 at 17:21 +0100, Stefan Richter wrote:
> Kristian Høgsberg wrote:
> > On Tue, 2009-01-13 at 14:48 -0800, Andrew Morton wrote:
> >> What do we think of just removing the constructor and using
> >> kmem_cache_zalloc()?
> > 
> > We still need to zero out the idr_layer before returning it to the idr's
> > internal free list.
> 
> I think so too.  But a more robust solution would IMO be to initialize
> an idr_layer /before/ use, not /after/ use.  Will send a patch later.

The reason it was done this way is that normally, when the layers are
returned to the free list they're already zero'ed out (since their
elements have been removed one by one), so no need to do this on later
re-initialization or when freeing.  It's a silly little
sup-optimization.  idr_remove_all() is different in that it doesn't
incrementally zero out the layer, and so it has to do it using a memset.

If you want rework how this works, I'd suggest just not using the free
list except for in the idr_pre_get()+idr_get() sequence.  When a layer
is no longer used, just free it, don't put it back on the free list.
And use kmem_cache_zalloc() in idr_pre_get() as Andrew suggests.

cheers,
Kristian


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/