[PATCH] slub: fix false-positive lockdep warning in free_partial()

[PATCH] slub: fix false-positive lockdep warning in free_partial()

[Vladimir Davydov]

Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
remove_partial() to be called with n->list_lock held, but free_partial()
called from kmem_cache_close() on cache destruction does not follow this
rule, leading to a warning:

  WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
  Modules linked in:
  CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
  Hardware name:
   0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
   0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
   ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
  Call Trace:
   [<ffffffff816d9583>] dump_stack+0x51/0x6e
   [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
   [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
   [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
   [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
   [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
   [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
   [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
   [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
   [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
   [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
   [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b

Although this cannot actually result in a race, because on cache
destruction there should not be any concurrent frees or allocations from
the cache, let's add spin_lock/unlock to free_partial() just to keep
lockdep happy.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 mm/slub.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 0eeea85034c8..b1054568a76e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3191,6 +3191,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 {
 	struct page *page, *h;
 
+	spin_lock_irq(&n->list_lock);
 	list_for_each_entry_safe(page, h, &n->partial, lru) {
 		if (!page->inuse) {
 			remove_partial(n, page);
@@ -3200,6 +3201,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 			"Objects remaining in %s on kmem_cache_close()");
 		}
 	}
+	spin_unlock_irq(&n->list_lock);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH] slub: fix false-positive lockdep warning in free_partial()

[Christoph Lameter]

On Tue, 4 Feb 2014, Vladimir Davydov wrote:

> Although this cannot actually result in a race, because on cache
> destruction there should not be any concurrent frees or allocations from
> the cache, let's add spin_lock/unlock to free_partial() just to keep
> lockdep happy.

Please add a comment that says this in the source so we know why this was
added.

Re: [PATCH] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Tue, 4 Feb 2014, Christoph Lameter wrote:

> > Although this cannot actually result in a race, because on cache
> > destruction there should not be any concurrent frees or allocations from
> > the cache, let's add spin_lock/unlock to free_partial() just to keep
> > lockdep happy.
> 
> Please add a comment that says this in the source so we know why this was
> added.
> 

Makes sense since there is a comment there already saying we don't need 
the lock, then with this patch we end up taking it away.  The nice thing 
is that there should be no lock contention here :)

I'm not sure we need to disable irqs as in the patch, though.

Re: [PATCH] slub: fix false-positive lockdep warning in free_partial()

[Vladimir Davydov]

On 02/05/2014 04:57 AM, David Rientjes wrote:
> On Tue, 4 Feb 2014, Christoph Lameter wrote:
>
>>> Although this cannot actually result in a race, because on cache
>>> destruction there should not be any concurrent frees or allocations from
>>> the cache, let's add spin_lock/unlock to free_partial() just to keep
>>> lockdep happy.
>> Please add a comment that says this in the source so we know why this was
>> added.
>>
> Makes sense since there is a comment there already saying we don't need 
> the lock, then with this patch we end up taking it away.  The nice thing 
> is that there should be no lock contention here :)
>
> I'm not sure we need to disable irqs as in the patch, though.

I'm afraid we need:

=================================
[ INFO: inconsistent lock state ]
3.14.0-rc1-mm1+ #4 Tainted: G        W  
---------------------------------
inconsistent {IN-HARDIRQ-W} -> {HARDIRQ-ON-W} usage.
modprobe/2760 [HC0[0]:SC0[0]:HE1:SE1] takes:
 (&(&n->list_lock)->rlock){?.-...}, at: [<ffffffff811c7e98>]
__kmem_cache_shutdown+0x68/0x210
{IN-HARDIRQ-W} state was registered at:
  [<ffffffff810d2e21>] __lock_acquire+0x8f1/0x17f0
  [<ffffffff810d3db2>] lock_acquire+0x92/0x120
  [<ffffffff816decc9>] _raw_spin_lock+0x39/0x70
  [<ffffffff811c54cb>] deactivate_slab+0x26b/0x500
  [<ffffffff811c7dfc>] flush_cpu_slab+0x3c/0x70
  [<ffffffff81100232>] generic_smp_call_function_single_interrupt+0x52/0xb0
  [<ffffffff810451c2>] smp_call_function_single_interrupt+0x22/0x40
  [<ffffffff816e96f2>] call_function_single_interrupt+0x72/0x80
  [<ffffffff8101f9ef>] default_idle+0x1f/0xe0
  [<ffffffff8101f346>] arch_cpu_idle+0x26/0x30
  [<ffffffff810e4766>] cpu_startup_entry+0xa6/0x290
  [<ffffffff81046129>] start_secondary+0x1d9/0x290
irq event stamp: 3883
hardirqs last  enabled at (3883): [<ffffffff816dd23f>]
mutex_lock_nested+0x2af/0x3d0
hardirqs last disabled at (3882): [<ffffffff816dd02f>]
mutex_lock_nested+0x9f/0x3d0
softirqs last  enabled at (3866): [<ffffffff810813e2>]
__do_softirq+0x1f2/0x330
softirqs last disabled at (3851): [<ffffffff81081675>] irq_exit+0xd5/0xe0

other info that might help us debug this:
 Possible unsafe locking scenario:

       CPU0
       ----
  lock(&(&n->list_lock)->rlock);
  <Interrupt>
    lock(&(&n->list_lock)->rlock);

 *** DEADLOCK ***

1 lock held by modprobe/2760:
 #0:  (slab_mutex){+.+.+.}, at: [<ffffffff811908b2>]
kmem_cache_destroy+0x22/0xf0

stack backtrace:
CPU: 0 PID: 2760 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #4
Hardware name:
 ffffffff82295780 ffff88003af89c18 ffffffff816d9633 0000000000000002
 ffff88007b2b0000 ffff88003af89c68 ffffffff810d1001 0000000000000000
 ffffffff00000001 0000000000000001 ffffffff822957e8 ffff88007b2b0840
Call Trace:
 [<ffffffff816d9633>] dump_stack+0x51/0x6e
 [<ffffffff810d1001>] print_usage_bug+0x231/0x290
 [<ffffffff810d1c5f>] mark_lock+0x37f/0x420
 [<ffffffff810d2cb9>] __lock_acquire+0x789/0x17f0
 [<ffffffff816db71b>] ? wait_for_completion+0x5b/0x120
 [<ffffffff8134c4b3>] ? cpumask_next_and+0x23/0x40
 [<ffffffff811c7dc0>] ? slab_cpuup_callback+0x120/0x120
 [<ffffffff810ffd4c>] ? smp_call_function_many+0x5c/0x250
 [<ffffffff811c7e98>] ? __kmem_cache_shutdown+0x68/0x210
 [<ffffffff810d3db2>] lock_acquire+0x92/0x120
 [<ffffffff811c7e98>] ? __kmem_cache_shutdown+0x68/0x210
 [<ffffffff811c1160>] ? set_page_slub_counters+0x40/0x40
 [<ffffffff816decc9>] _raw_spin_lock+0x39/0x70
 [<ffffffff811c7e98>] ? __kmem_cache_shutdown+0x68/0x210
 [<ffffffff811c7e98>] __kmem_cache_shutdown+0x68/0x210
 [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
 [<ffffffffa0180455>] xfs_qm_exit+0x15/0x30 [xfs]
 [<ffffffffa018ab25>] exit_xfs_fs+0x9/0x4e4 [xfs]
 [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
 [<ffffffff816dfd98>] ? retint_swapgs+0x13/0x1b
 [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
 [<ffffffff81359fae>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff816e85f9>] system_call_fastpath+0x16/0x1b

[PATCH v2] slub: fix false-positive lockdep warning in free_partial()

[Vladimir Davydov]

Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
remove_partial() to be called with n->list_lock held, but free_partial()
called from kmem_cache_close() on cache destruction does not follow this
rule, leading to a warning:

  WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
  Modules linked in:
  CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
  Hardware name:
   0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
   0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
   ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
  Call Trace:
   [<ffffffff816d9583>] dump_stack+0x51/0x6e
   [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
   [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
   [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
   [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
   [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
   [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
   [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
   [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
   [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
   [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
   [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b

Although this cannot actually result in a race, because on cache
destruction there should not be any concurrent frees or allocations from
the cache, let's add spin_lock/unlock to free_partial() just to keep
lockdep happy.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 v2: add a comment explaining why we need to take the lock

 mm/slub.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/slub.c b/mm/slub.c
index 0eeea85034c8..24bf05e962ff 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3191,6 +3191,11 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 {
 	struct page *page, *h;
 
+	/*
+	 * the lock is for lockdep's sake, not for any actual
+	 * race protection
+	 */
+	spin_lock_irq(&n->list_lock);
 	list_for_each_entry_safe(page, h, &n->partial, lru) {
 		if (!page->inuse) {
 			remove_partial(n, page);
@@ -3200,6 +3205,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 			"Objects remaining in %s on kmem_cache_close()");
 		}
 	}
+	spin_unlock_irq(&n->list_lock);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH v2] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Vladimir Davydov wrote:

> Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
> remove_partial() to be called with n->list_lock held, but free_partial()
> called from kmem_cache_close() on cache destruction does not follow this
> rule, leading to a warning:
> 
>   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
>   Modules linked in:
>   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
>   Hardware name:
>    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
>    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
>    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
>   Call Trace:
>    [<ffffffff816d9583>] dump_stack+0x51/0x6e
>    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
>    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
>    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
>    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
>    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
>    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
>    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
>    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
>    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
>    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
> 
> Although this cannot actually result in a race, because on cache
> destruction there should not be any concurrent frees or allocations from
> the cache, let's add spin_lock/unlock to free_partial() just to keep
> lockdep happy.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
> ---
>  v2: add a comment explaining why we need to take the lock
> 
>  mm/slub.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 0eeea85034c8..24bf05e962ff 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3191,6 +3191,11 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
>  {
>  	struct page *page, *h;
>  
> +	/*
> +	 * the lock is for lockdep's sake, not for any actual
> +	 * race protection
> +	 */

I think Christoph was referring to altering the comment for this function 
which still says "We must be the last thread using the cache and therefore 
we do not need to lock anymore."

> +	spin_lock_irq(&n->list_lock);
>  	list_for_each_entry_safe(page, h, &n->partial, lru) {
>  		if (!page->inuse) {
>  			remove_partial(n, page);
> @@ -3200,6 +3205,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
>  			"Objects remaining in %s on kmem_cache_close()");
>  		}
>  	}
> +	spin_unlock_irq(&n->list_lock);
>  }
>  
>  /*

Re: [PATCH v2] slub: fix false-positive lockdep warning in free_partial()

[Vladimir Davydov]

On 02/05/2014 12:01 PM, David Rientjes wrote:
> On Wed, 5 Feb 2014, Vladimir Davydov wrote:
>
>> Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
>> remove_partial() to be called with n->list_lock held, but free_partial()
>> called from kmem_cache_close() on cache destruction does not follow this
>> rule, leading to a warning:
>>
>>   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
>>   Modules linked in:
>>   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
>>   Hardware name:
>>    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
>>    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
>>    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
>>   Call Trace:
>>    [<ffffffff816d9583>] dump_stack+0x51/0x6e
>>    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
>>    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
>>    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
>>    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
>>    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
>>    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
>>    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
>>    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
>>    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
>>    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>>    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
>>
>> Although this cannot actually result in a race, because on cache
>> destruction there should not be any concurrent frees or allocations from
>> the cache, let's add spin_lock/unlock to free_partial() just to keep
>> lockdep happy.
>>
>> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
>> ---
>>  v2: add a comment explaining why we need to take the lock
>>
>>  mm/slub.c |    6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 0eeea85034c8..24bf05e962ff 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -3191,6 +3191,11 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
>>  {
>>  	struct page *page, *h;
>>  
>> +	/*
>> +	 * the lock is for lockdep's sake, not for any actual
>> +	 * race protection
>> +	 */
> I think Christoph was referring to altering the comment for this function 
> which still says "We must be the last thread using the cache and therefore 
> we do not need to lock anymore."

Oh, sorry, I didn't notice that. Will amend.

Thanks.

>
>> +	spin_lock_irq(&n->list_lock);
>>  	list_for_each_entry_safe(page, h, &n->partial, lru) {
>>  		if (!page->inuse) {
>>  			remove_partial(n, page);
>> @@ -3200,6 +3205,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
>>  			"Objects remaining in %s on kmem_cache_close()");
>>  		}
>>  	}
>> +	spin_unlock_irq(&n->list_lock);
>>  }
>>  
>>  /*

[PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Vladimir Davydov]

Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
remove_partial() to be called with n->list_lock held, but free_partial()
called from kmem_cache_close() on cache destruction does not follow this
rule, leading to a warning:

  WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
  Modules linked in:
  CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
  Hardware name:
   0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
   0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
   ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
  Call Trace:
   [<ffffffff816d9583>] dump_stack+0x51/0x6e
   [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
   [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
   [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
   [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
   [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
   [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
   [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
   [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
   [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
   [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
   [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b

Although this cannot actually result in a race, because on cache
destruction there should not be any concurrent frees or allocations from
the cache, let's add spin_lock/unlock to free_partial() just to keep
lockdep happy.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 v3: fix the comment to free_partial()
 v2: add a comment explaining why we need to take the lock

 mm/slub.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/slub.c b/mm/slub.c
index 0eeea85034c8..65a40d20ec80 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3185,12 +3185,13 @@ static void list_slab_objects(struct kmem_cache *s, struct page *page,
 /*
  * Attempt to free all partial slabs on a node.
  * This is called from kmem_cache_close(). We must be the last thread
- * using the cache and therefore we do not need to lock anymore.
+ * using the cache, but we still have to lock for lockdep's sake.
  */
 static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 {
 	struct page *page, *h;
 
+	spin_lock_irq(&n->list_lock);
 	list_for_each_entry_safe(page, h, &n->partial, lru) {
 		if (!page->inuse) {
 			remove_partial(n, page);
@@ -3200,6 +3201,7 @@ static void free_partial(struct kmem_cache *s, struct kmem_cache_node *n)
 			"Objects remaining in %s on kmem_cache_close()");
 		}
 	}
+	spin_unlock_irq(&n->list_lock);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Vladimir Davydov wrote:

> Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
> remove_partial() to be called with n->list_lock held, but free_partial()
> called from kmem_cache_close() on cache destruction does not follow this
> rule, leading to a warning:
> 
>   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
>   Modules linked in:
>   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
>   Hardware name:
>    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
>    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
>    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
>   Call Trace:
>    [<ffffffff816d9583>] dump_stack+0x51/0x6e
>    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
>    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
>    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
>    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
>    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
>    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
>    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
>    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
>    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
>    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
> 
> Although this cannot actually result in a race, because on cache
> destruction there should not be any concurrent frees or allocations from
> the cache, let's add spin_lock/unlock to free_partial() just to keep
> lockdep happy.
> 
> Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>

Acked-by: David Rientjes <rientjes@google.com>

And thanks for trying the irq-free variant!

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Christoph Lameter]

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Steven Rostedt]

On Wed, Feb 05, 2014 at 12:15:33PM +0400, Vladimir Davydov wrote:
> Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
> remove_partial() to be called with n->list_lock held, but free_partial()
> called from kmem_cache_close() on cache destruction does not follow this
> rule, leading to a warning:
> 
>   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
>   Modules linked in:
>   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
>   Hardware name:
>    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
>    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
>    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
>   Call Trace:
>    [<ffffffff816d9583>] dump_stack+0x51/0x6e
>    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
>    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
>    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
>    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
>    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
>    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
>    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
>    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
>    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
>    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
> 
> Although this cannot actually result in a race, because on cache
> destruction there should not be any concurrent frees or allocations from
> the cache, let's add spin_lock/unlock to free_partial() just to keep
> lockdep happy.

Really? We are adding a spin lock for a case where it is not needed just to
quiet lockdep?

Now if it really isn't needed, then why don't we do the following instead of
adding the overhead of taking a lock?

static inline
__remove_partial(struct kmem_cache_node *n, struct page *page)
{
	list_del(&page->lru);
	n->nr_partial--;
}

static inline remove_partial(struct kmem_cache_node *n,
			     struct page *page)
{
	lockdep_assert_held(&n->list_lock);
	__remove_partial(n, page);
}

And then just call __remove_partial() where we don't need to check if the
lock is held or not with a big comment to it.

That, IMNSHO, is a much better solution.

-- Steve

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Steven Rostedt wrote:

> > Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
> > remove_partial() to be called with n->list_lock held, but free_partial()
> > called from kmem_cache_close() on cache destruction does not follow this
> > rule, leading to a warning:
> > 
> >   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
> >   Modules linked in:
> >   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
> >   Hardware name:
> >    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
> >    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
> >    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
> >   Call Trace:
> >    [<ffffffff816d9583>] dump_stack+0x51/0x6e
> >    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
> >    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
> >    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
> >    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
> >    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
> >    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
> >    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
> >    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
> >    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
> >    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> >    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
> > 
> > Although this cannot actually result in a race, because on cache
> > destruction there should not be any concurrent frees or allocations from
> > the cache, let's add spin_lock/unlock to free_partial() just to keep
> > lockdep happy.
> 
> Really? We are adding a spin lock for a case where it is not needed just to
> quiet lockdep?
> 
> Now if it really isn't needed, then why don't we do the following instead of
> adding the overhead of taking a lock?
> 

There's an extremely small overhead of taking this lock, the cache has 
been destroyed and is the process of being torn down, there will be 
absolutely no contention on n->list_lock.

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Steven Rostedt]

On Wed, 5 Feb 2014 12:32:43 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> On Wed, 5 Feb 2014, Steven Rostedt wrote:

> There's an extremely small overhead of taking this lock, the cache has 
> been destroyed and is the process of being torn down, there will be 
> absolutely no contention on n->list_lock.

But why add it if it isn't necessary? You're even disabling interrupts,
which means that you add to the response latency. That is, this change
does affect other aspects of the kernel!

-- Steve

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Steven Rostedt wrote:

> > There's an extremely small overhead of taking this lock, the cache has 
> > been destroyed and is the process of being torn down, there will be 
> > absolutely no contention on n->list_lock.
> 
> But why add it if it isn't necessary? You're even disabling interrupts,
> which means that you add to the response latency. That is, this change
> does affect other aspects of the kernel!
> 

The functions that manipulate the partial lists was modified by 
c65c1877bd68 ("slub: use lockdep_assert_held") which replaced commentary 
with runtime checking on debug kernels with lockdep enabled.  I'm not sure 
adding more code to do the remove_partial() and __remove_partial() variant 
is the right solution to just bypass the check; if anything, I think we 
should accept the fact that the comment should have been "requires 
n->list_lock if the slab cache can be accessed by other cpus" that makes 
it clear we don't need it for init and destroy paths.

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Steven Rostedt]

On Wed, 5 Feb 2014 13:07:05 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:

> The functions that manipulate the partial lists was modified by 
> c65c1877bd68 ("slub: use lockdep_assert_held") which replaced commentary 
> with runtime checking on debug kernels with lockdep enabled.  I'm not sure 
> adding more code to do the remove_partial() and __remove_partial() variant 
> is the right solution to just bypass the check; if anything, I think we 
> should accept the fact that the comment should have been "requires 
> n->list_lock if the slab cache can be accessed by other cpus" that makes 
> it clear we don't need it for init and destroy paths.

Then add the comment that clears this up. But lets not add spinlocks
just to quiet something if they truly are not needed.

We use "__" variants all the time. That's really not extra code.

Heck, if you want, call it remove_freed_partial() that shows that this
version skips the check because it is freed.

And if you don't want to have remove_freed_partial() being called by
remove_partial() than still keep the "__" variant, add a
"__always_inline" to it, and then do:

static __always_inline
__remove_partial(struct kmem_cache_node *n, struct page *page)
{
        list_del(&page->lru);
        n->nr_partial--;
}

static inline remove_partial(struct kmem_cache_node *n,
                             struct page *page)
{
        lockdep_assert_held(&n->list_lock);
        __remove_partial(n, page);
}


static inline remove_freed_partial(struct kmem_cache_node *n,
                             struct page *page)
{
        __remove_partial(n, page);
}

The naming like this documents itself.

-- Steve

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Steven Rostedt wrote:

> Then add the comment that clears this up. But lets not add spinlocks
> just to quiet something if they truly are not needed.
> 
> We use "__" variants all the time. That's really not extra code.
> 
> Heck, if you want, call it remove_freed_partial() that shows that this
> version skips the check because it is freed.
> 
> And if you don't want to have remove_freed_partial() being called by
> remove_partial() than still keep the "__" variant, add a
> "__always_inline" to it, and then do:
> 
> static __always_inline
> __remove_partial(struct kmem_cache_node *n, struct page *page)
> {
>         list_del(&page->lru);
>         n->nr_partial--;
> }
> 
> static inline remove_partial(struct kmem_cache_node *n,
>                              struct page *page)
> {
>         lockdep_assert_held(&n->list_lock);
>         __remove_partial(n, page);
> }
> 
> 
> static inline remove_freed_partial(struct kmem_cache_node *n,
>                              struct page *page)
> {
>         __remove_partial(n, page);
> }
> 
> The naming like this documents itself.
> 

Looks like you've got something prepared already!  Mind sending it to 
Pekka as a patch based on linux-next?

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Steven Rostedt]

On Wed, 5 Feb 2014 13:25:28 -0800 (PST)
David Rientjes <rientjes@google.com> wrote:
 
> Looks like you've got something prepared already!  Mind sending it to 
> Pekka as a patch based on linux-next?

Sure, and there's another lockdep splat that I'm working on too.

But first, I need to shovel my driveway yet again (We got something
like 18 inches of snow in the last 24 hours).

-- Steve

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[David Rientjes]

On Wed, 5 Feb 2014, Steven Rostedt wrote:

> > Looks like you've got something prepared already!  Mind sending it to 
> > Pekka as a patch based on linux-next?
> 
> Sure, and there's another lockdep splat that I'm working on too.
> 

If it's coming from slub, make sure you've applied 
http://marc.info/?l=linux-kernel&m=139147105027693 first which fixes yet 
another slub lockdep issue related to the lockdep assertion.  It's not in 
linux-next yet because it went through -mm instead of Pekka's tree.

> But first, I need to shovel my driveway yet again (We got something
> like 18 inches of snow in the last 24 hours).
> 

Fun!  It's 33 F here today but nobody cares because we have a Super Bowl 
parade in town!

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Christoph Lameter]

On Wed, 5 Feb 2014, Steven Rostedt wrote:

> Really? We are adding a spin lock for a case where it is not needed just to
> quiet lockdep?

Well its a very rarely used code path. Doesnt matter performance wise
since a slab cache should have no objects when its going to be removed.

> Now if it really isn't needed, then why don't we do the following instead of
> adding the overhead of taking a lock?
>
> static inline
> __remove_partial(struct kmem_cache_node *n, struct page *page)
> {
> 	list_del(&page->lru);
> 	n->nr_partial--;
> }
>
> static inline remove_partial(struct kmem_cache_node *n,
> 			     struct page *page)
> {
> 	lockdep_assert_held(&n->list_lock);
> 	__remove_partial(n, page);
> }
>
> And then just call __remove_partial() where we don't need to check if the
> lock is held or not with a big comment to it.
>
> That, IMNSHO, is a much better solution

Ok with me. It just adds another variant of remove_partial.

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Steven Rostedt]

On Wed, 5 Feb 2014 14:42:26 -0600 (CST)
Christoph Lameter <cl@linux.com> wrote:


> > That, IMNSHO, is a much better solution
> 
> Ok with me. It just adds another variant of remove_partial.

That's a common practice in the kernel though.

-- Steve

Re: [PATCH v3] slub: fix false-positive lockdep warning in free_partial()

[Peter Zijlstra]

On Wed, Feb 05, 2014 at 02:58:37PM -0500, Steven Rostedt wrote:
> On Wed, Feb 05, 2014 at 12:15:33PM +0400, Vladimir Davydov wrote:
> > Commit c65c1877bd68 ("slub: use lockdep_assert_held") requires
> > remove_partial() to be called with n->list_lock held, but free_partial()
> > called from kmem_cache_close() on cache destruction does not follow this
> > rule, leading to a warning:
> > 
> >   WARNING: CPU: 0 PID: 2787 at mm/slub.c:1536 __kmem_cache_shutdown+0x1b2/0x1f0()
> >   Modules linked in:
> >   CPU: 0 PID: 2787 Comm: modprobe Tainted: G        W    3.14.0-rc1-mm1+ #1
> >   Hardware name:
> >    0000000000000600 ffff88003ae1dde8 ffffffff816d9583 0000000000000600
> >    0000000000000000 ffff88003ae1de28 ffffffff8107c107 0000000000000000
> >    ffff880037ab2b00 ffff88007c240d30 ffffea0001ee5280 ffffea0001ee52a0
> >   Call Trace:
> >    [<ffffffff816d9583>] dump_stack+0x51/0x6e
> >    [<ffffffff8107c107>] warn_slowpath_common+0x87/0xb0
> >    [<ffffffff8107c145>] warn_slowpath_null+0x15/0x20
> >    [<ffffffff811c7fe2>] __kmem_cache_shutdown+0x1b2/0x1f0
> >    [<ffffffff811908d3>] kmem_cache_destroy+0x43/0xf0
> >    [<ffffffffa013a123>] xfs_destroy_zones+0x103/0x110 [xfs]
> >    [<ffffffffa0192b54>] exit_xfs_fs+0x38/0x4e4 [xfs]
> >    [<ffffffff811036fa>] SyS_delete_module+0x19a/0x1f0
> >    [<ffffffff816dfcd8>] ? retint_swapgs+0x13/0x1b
> >    [<ffffffff810d2125>] ? trace_hardirqs_on_caller+0x105/0x1d0
> >    [<ffffffff81359efe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
> >    [<ffffffff816e8539>] system_call_fastpath+0x16/0x1b
> > 
> > Although this cannot actually result in a race, because on cache
> > destruction there should not be any concurrent frees or allocations from
> > the cache, let's add spin_lock/unlock to free_partial() just to keep
> > lockdep happy.
> 
> Really? We are adding a spin lock for a case where it is not needed just to
> quiet lockdep?

We do that in other places too, its usually init code that thinks to
'know' there is no concurrency, however since its init code its not
performance critical in any way shape or fashion so we just stick to the
'rules' and don't try to play games.

> Now if it really isn't needed, then why don't we do the following instead of
> adding the overhead of taking a lock?
> 
> static inline
> __remove_partial(struct kmem_cache_node *n, struct page *page)
> {
> 	list_del(&page->lru);
> 	n->nr_partial--;
> }
> 
> static inline remove_partial(struct kmem_cache_node *n,
> 			     struct page *page)
> {
> 	lockdep_assert_held(&n->list_lock);
> 	__remove_partial(n, page);
> }
> 
> And then just call __remove_partial() where we don't need to check if the
> lock is held or not with a big comment to it.
> 
> That, IMNSHO, is a much better solution.

I would say yes if there's a performance angle, but in this case you're
increasing the API footprint for an absolute slow path.
kmem_cache_destroy() isn't something we call (or should call) often.

That said, I don't care much, its up to Christoph and Pekka.