slab: fix the DEADLOCK issue on l3 alien lock

PATCH Applied Michael Wang 4 participants 7 messages Sep 5, 2012 – Sep 11, 2012 1 message not in the archive Memory management

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Michael Wang

author

2012-09-05 02:33 UTC #1 lore raw

PATCH 1/1 slab: fix the DEADLOCK issue on l3 alien lock

From: Michael Wang <[email protected]>

DEADLOCK will be report while running a kernel with NUMA and LOCKDEP enabled, the process of this fake report is:

	   kmem_cache_free()	//free obj in cachep
	-> cache_free_alien()	//acquire cachep's l3 alien lock
	-> __drain_alien_cache()
	-> free_block()
	-> slab_destroy()
	-> kmem_cache_free()	//free slab in cachep->slabp_cache
	-> cache_free_alien()	//acquire cachep->slabp_cache's l3 alien lock

Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.

This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.

However, init_lock_keys() was invoked at a wrong position which is before we invoke enable_cpucache() on each cache.

Since until set slab_state to be FULL, we won't invoke enable_cpucache() on caches to build their l3 alien while creating them, so although we invoked init_lock_keys(), the l3 alien lock class won't change since we don't have them until invoked enable_cpucache() later.

This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.

Signed-off-by Michael Wang <[email protected]>

---
 mm/slab.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

mm/slab.c +3 -3

index d4715e5..cc679ef 100644
@@ -1780,9 +1780,6 @@ void __init kmem_cache_init_late(void)
 
 	slab_state = UP;
 
-	/* Annotate slab for lockdep -- annotate the malloc caches */
-	init_lock_keys();
-
 	/* 6) resize the head arrays to their final sizes */
 	mutex_lock(&slab_mutex);
 	list_for_each_entry(cachep, &slab_caches, list)

@@ -1790,6 +1787,9 @@ void __init kmem_cache_init_late(void)
 			BUG();
 	mutex_unlock(&slab_mutex);
 
+	/* Annotate slab for lockdep -- annotate the malloc caches */
+	init_lock_keys();
+
 	/* Done! */
 	slab_state = FULL;

Signature · 7 lines

1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Christoph Lameter

Acked-by on v1 1/1

2012-09-05 13:55 UTC #2 lore raw

Quoted 2 lines · Michael Wang

Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.

Ahh... That is a key insight into why this occurs.

Quoted 2 lines

This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.

Right. I was wondering why we still get intermitted reports on this.

Quoted 2 lines

This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.

Acked-by Christoph Lameter <[email protected]>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Michael Wang

author

2012-09-06 03:05 UTC #3 lore raw

Quoted 10 lines · Christoph Lameter

Quoted 2 lines · Michael Wang
Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.
Ahh... That is a key insight into why this occurs.
Quoted 2 lines
This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.

Quoted 6 lines

Right. I was wondering why we still get intermitted reports on this.
Quoted 2 lines
This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.
Acked-by Christoph Lameter <[email protected]>

Thanks for your review.

And add Paul to the cc list(my skills on mailing is really poor...).

Regards,
Michael Wang

Quoted 6 lines

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

Paul E. McKenney

Tested-by

2012-09-06 22:29 UTC #4 lore raw

Quoted 13 lines · Michael Wang

Quoted 12 lines · Christoph Lameter
Quoted 2 lines · Michael Wang
Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.
Ahh... That is a key insight into why this occurs.
Quoted 2 lines
This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.
Right. I was wondering why we still get intermitted reports on this.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.
Acked-by Christoph Lameter <[email protected]>
Thanks for your review.
And add Paul to the cc list(my skills on mailing is really poor...).

Tested-by Paul E. McKenney <[email protected]>

Pekka Enberg

2012-09-08 08:39 UTC #5 lore raw

Quoted 17 lines · Paul E. McKenney

Quoted 16 lines · Michael Wang
Quoted 15 lines · Christoph Lameter
Quoted 2 lines · Michael Wang
Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.
Ahh... That is a key insight into why this occurs.
Quoted 2 lines
This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.
Right. I was wondering why we still get intermitted reports on this.
Quoted 2 lines
This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Acked-by Christoph Lameter <[email protected]>
Thanks for your review.
And add Paul to the cc list(my skills on mailing is really poor...).
Tested-by Paul E. McKenney <[email protected]>

I'd also like to tag this for the stable tree to avoid bogus lockdep reports. How far back in release history should we queue this?

Michael Wang

author

2012-09-11 02:50 UTC #6 lore raw

Quoted 21 lines · Pekka Enberg

Quoted 19 lines · Paul E. McKenney
Quoted 18 lines · Michael Wang
Quoted 16 lines · Christoph Lameter
Quoted 2 lines · Michael Wang
Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.
Ahh... That is a key insight into why this occurs.
Quoted 2 lines
This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.
Right. I was wondering why we still get intermitted reports on this.
Quoted 2 lines
This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.
Acked-by Christoph Lameter <[email protected]>

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Thanks for your review.
And add Paul to the cc list(my skills on mailing is really poor...).
Tested-by Paul E. McKenney <[email protected]>
I'd also like to tag this for the stable tree to avoid bogus lockdep reports. How far back in release history should we queue this?

Hi, Pekka

Sorry for the delayed reply, I try to find out the reason for commit 30765b92 but not get it yet, so I add Peter to the cc list.

The below patch for release 3.0.0 is the one to cause the bogus report.

commit 30765b92ada267c5395fc788623cb15233276f5c

Author: Peter Zijlstra <[email protected]>
Date:   Thu Jul 28 23:22:56 2011 +0200

    slab, lockdep: Annotate the locks before using them

    Fernando found we hit the regular OFF_SLAB 'recursion' before we
    annotate the locks, cure this.

    The relevant portion of the stack-trace:

    > [    0.000000]  [<c085e24f>] rt_spin_lock+0x50/0x56
    > [    0.000000]  [<c04fb406>] __cache_free+0x43/0xc3
    > [    0.000000]  [<c04fb23f>] kmem_cache_free+0x6c/0xdc
    > [    0.000000]  [<c04fb2fe>] slab_destroy+0x4f/0x53
    > [    0.000000]  [<c04fb396>] free_block+0x94/0xc1
    > [    0.000000]  [<c04fc551>] do_tune_cpucache+0x10b/0x2bb
    > [    0.000000]  [<c04fc8dc>] enable_cpucache+0x7b/0xa7
    > [    0.000000]  [<c0bd9d3c>] kmem_cache_init_late+0x1f/0x61
    > [    0.000000]  [<c0bba687>] start_kernel+0x24c/0x363
    > [    0.000000]  [<c0bba0ba>] i386_start_kernel+0xa9/0xaf

    Reported-by: Fernando Lopez-Lezcano <[email protected]>
    Acked-by: Pekka Enberg <[email protected]>
    Signed-off-by: Peter Zijlstra <[email protected]>
    Link: http://lkml.kernel.org/r/1311888176.2617.379.camel@laptop
    Signed-off-by: Ingo Molnar <[email protected]>

It moved init_lock_keys() before we build up the alien, so we failed to reclass it.

Regards,
Michael Wang

Quoted 1 line

Pekka Enberg

Applied

2012-09-11 16:29 UTC #7 lore raw

Quoted 65 lines · Michael Wang

Quoted 21 lines · Pekka Enberg
Quoted 19 lines · Paul E. McKenney
Quoted 18 lines · Michael Wang
Quoted 16 lines · Christoph Lameter
Quoted 2 lines · Michael Wang
Since the cachep and cachep->slabp_cache's l3 alien are in the same lock class, fake report generated.
Ahh... That is a key insight into why this occurs.
Quoted 2 lines
This should not happen since we already have init_lock_keys() which will reassign the lock class for both l3 list and l3 alien.
Right. I was wondering why we still get intermitted reports on this.
Quoted 2 lines
This patch will invoke init_lock_keys() after we done enable_cpucache() instead of before to avoid the fake DEADLOCK report.
Acked-by Christoph Lameter <[email protected]>
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Thanks for your review.
And add Paul to the cc list(my skills on mailing is really poor...).
Tested-by Paul E. McKenney <[email protected]>
I'd also like to tag this for the stable tree to avoid bogus lockdep reports. How far back in release history should we queue this?
Hi, Pekka
Sorry for the delayed reply, I try to find out the reason for commit 30765b92 but not get it yet, so I add Peter to the cc list.
The below patch for release 3.0.0 is the one to cause the bogus report.
commit 30765b92ada267c5395fc788623cb15233276f5c
Author: Peter Zijlstra <[email protected]>
Date:   Thu Jul 28 23:22:56 2011 +0200
    slab, lockdep: Annotate the locks before using them

    Fernando found we hit the regular OFF_SLAB 'recursion' before we
    annotate the locks, cure this.

    The relevant portion of the stack-trace:

    > [    0.000000]  [<c085e24f>] rt_spin_lock+0x50/0x56
    > [    0.000000]  [<c04fb406>] __cache_free+0x43/0xc3
    > [    0.000000]  [<c04fb23f>] kmem_cache_free+0x6c/0xdc
    > [    0.000000]  [<c04fb2fe>] slab_destroy+0x4f/0x53
    > [    0.000000]  [<c04fb396>] free_block+0x94/0xc1
    > [    0.000000]  [<c04fc551>] do_tune_cpucache+0x10b/0x2bb
    > [    0.000000]  [<c04fc8dc>] enable_cpucache+0x7b/0xa7
    > [    0.000000]  [<c0bd9d3c>] kmem_cache_init_late+0x1f/0x61
    > [    0.000000]  [<c0bba687>] start_kernel+0x24c/0x363
    > [    0.000000]  [<c0bba0ba>] i386_start_kernel+0xa9/0xaf

    Reported-by: Fernando Lopez-Lezcano <[email protected]>
    Acked-by: Pekka Enberg <[email protected]>
    Signed-off-by: Peter Zijlstra <[email protected]>
    Link: http://lkml.kernel.org/r/1311888176.2617.379.camel@laptop
    Signed-off-by: Ingo Molnar <[email protected]>

Quoted 2 lines

It moved init_lock_keys() before we build up the alien, so we failed to reclass it.

I've queued the patch for v3.7. Thanks!