Re: SLOB lockup (was: Re: [tip:core/locking] lockdep: annotate reclaim context (__GFP_NOFS), fix SLOB)

[Matt Mackall <mpm@selenic.com>]

On Sun, 2009-03-15 at 20:06 +1100, Nick Piggin wrote:
> On Sunday 15 March 2009 17:48:18 Ingo Molnar wrote:
> 
> > > Cc: Nick Piggin <npiggin@suse.de>
> > > Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > > LKML-Reference: <20090128135457.350751756@chello.nl>
> > > Signed-off-by: Ingo Molnar <mingo@elte.hu>
> >
> > and with this fixed, and with SLOB now being tested in -tip, the
> > new lockdep assert attached below (followed by a real lockup)
> > pops up.
> >
> > Seems like a genuine SLOB bug, probably present upstream as
> > well.
> 
> Hmmf. debugobjects calls back into the slab allocator from the page
> allocator. The following patch would improve SLOB, but I think it
> would be a good idea to avoid a dependency in that direction. Can
> debugobjects defer this freeing?

Yeah. I don't think any of the allocators are designed with recursion in
mind. That the others aren't (visibly) failing here is blind luck.

Nick, not really sure what your patch is accomplishing. It narrows the
lock window, but it doesn't eliminate it. But I think we can take the
page allocator case out from under the lock entirely, no?

diff -r 8e0f1cee0a71 mm/slob.c
--- a/mm/slob.c	Sat Jan 24 15:41:13 2009 -0600
+++ b/mm/slob.c	Sun Mar 15 09:50:42 2009 -0500
@@ -387,8 +387,6 @@
 	sp = (struct slob_page *)virt_to_page(block);
 	units = SLOB_UNITS(size);
 
-	spin_lock_irqsave(&slob_lock, flags);
-
 	if (sp->units + units == SLOB_UNITS(PAGE_SIZE)) {
 		/* Go directly to page allocator. Do not pass slob allocator */
 		if (slob_page_free(sp))
@@ -396,9 +394,11 @@
 		clear_slob_page(sp);
 		free_slob_page(sp);
 		free_page((unsigned long)b);
-		goto out;
+		return;
 	}
 
+	spin_lock_irqsave(&slob_lock, flags);
+
 	if (!slob_page_free(sp)) {
 		/* This slob page is about to become partially free. Easy! */
 		sp->units = units;

-- 
http://selenic.com : development and support for Mercurial and Linux