On 10/03/2012 08:04 PM, Jiri Kosina wrote:
> On Wed, 3 Oct 2012, Christoph Lameter wrote:
>
>>> How about the patch below? Pekka, Christoph, please?
>>
>> Looks fine for -stable. For upstream there is going to be a move to
>> slab_common coming in this merge period. We would need a fix against -next
>> or Pekka's tree too.
>
> Thanks Christoph. Patch against Pekka's slab/for-linus branch below.
>
> I have kept the Acked-by/Reviewed-by from the version of the patch against
> current Linus' tree, if anyone object, please shout loudly. Ideally should
> go in during this merge window to keep lockdep happy.
>
>
>
>
>
> From: Jiri Kosina <jkosina@xxxxxxx>
> Subject: [PATCH] mm, slab: release slab_mutex earlier in kmem_cache_destroy()
>
> Commit 1331e7a1bbe1 ("rcu: Remove _rcu_barrier() dependency on
> __stop_machine()") introduced slab_mutex -> cpu_hotplug.lock
> dependency through kmem_cache_destroy() -> rcu_barrier() ->
> _rcu_barrier() -> get_online_cpus().
>
> Lockdep thinks that this might actually result in ABBA deadlock,
> and reports it as below:
>
[...]
> Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@xxxxxxxxxxxxxxxxxx>
> Reviewed-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> Acked-by: Christoph Lameter <cl@xxxxxxxxx>
> Signed-off-by: Jiri Kosina <jkosina@xxxxxxx>
> ---
> mm/slab_common.c | 4 +++-
> 1 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 9c21725..90c3053 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -166,6 +166,7 @@ void kmem_cache_destroy(struct kmem_cache *s)
> s->refcount--;
> if (!s->refcount) {
> list_del(&s->list);
> + mutex_unlock(&slab_mutex);
>
> if (!__kmem_cache_shutdown(s)) {
__kmem_cache_shutdown() calls __cache_shrink(). And __cache_shrink() has this
comment over it:
/* Called with slab_mutex held to protect against cpu hotplug */
So, I guess the question is whether to modify your patch to hold the slab_mutex
while calling this function, or to update the comment on top of this function
saying that we are OK to call this function (even without slab_mutex) when we
are inside a get/put_online_cpus() section.
> if (s->flags & SLAB_DESTROY_BY_RCU)
> @@ -179,8 +180,9 @@ void kmem_cache_destroy(struct kmem_cache *s)
> s->name);
> dump_stack();
There is a list_add() before this dump_stack(). I assume we need to hold the
slab_mutex while calling it.
> }
> + } else {
> + mutex_unlock(&slab_mutex);
> }
> - mutex_unlock(&slab_mutex);
> put_online_cpus();
> }
> EXPORT_SYMBOL(kmem_cache_destroy);
>
Regards,
Srivatsa S. Bhat
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>