lkml.org 
[lkml]   [2012]   [Oct]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mm, slab: release slab_mutex earlier in kmem_cache_destroy() (was Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()"))
    On 10/03/2012 06:15 AM, Jiri Kosina wrote:
    > On Tue, 2 Oct 2012, Paul E. McKenney wrote:
    >
    >> On Wed, Oct 03, 2012 at 01:48:21AM +0200, Jiri Kosina wrote:
    >>> On Tue, 2 Oct 2012, Paul E. McKenney wrote:
    >>>
    >>>> Indeed. Slab seems to be doing an rcu_barrier() in a CPU hotplug
    >>>> notifier, which doesn't sit so well with rcu_barrier() trying to exclude
    >>>> CPU hotplug events. I could go back to the old approach, but it is
    >>>> significantly more complex. I cannot say that I am all that happy about
    >>>> anyone calling rcu_barrier() from a CPU hotplug notifier because it
    >>>> doesn't help CPU hotplug latency, but that is a separate issue.
    >>>>
    >>>> But the thing is that rcu_barrier()'s assumptions work just fine if either
    >>>> (1) it excludes hotplug operations or (2) if it is called from a hotplug
    >>>> notifier. You see, either way, the CPU cannot go away while rcu_barrier()
    >>>> is executing. So the right way to resolve this seems to be to do the
    >>>> get_online_cpus() only if rcu_barrier() is -not- executing in the context
    >>>> of a hotplug notifier. Should be fixable without too much hassle...
    >>>
    >>> Sorry, I don't think I understand what you are proposing just yet.
    >>>
    >>> If I understand it correctly, you are proposing to introduce some magic
    >>> into _rcu_barrier() such as (pseudocode of course):
    >>>
    >>> if (!being_called_from_hotplug_notifier_callback)
    >>> get_online_cpus()
    >>>
    >>> How does that protect from the scenario I've outlined before though?
    >>>
    >>> CPU 0 CPU 1
    >>> kmem_cache_destroy()
    >>> mutex_lock(slab_mutex)
    >>> _cpu_up()
    >>> cpu_hotplug_begin()
    >>> mutex_lock(cpu_hotplug.lock)
    >>> rcu_barrier()
    >>> _rcu_barrier()
    >>> get_online_cpus()
    >>> mutex_lock(cpu_hotplug.lock)
    >>> (blocks, CPU 1 has the mutex)
    >>> __cpu_notify()
    >>> mutex_lock(slab_mutex)
    >>>
    >>> CPU 0 grabs both locks anyway (it's not running from notifier callback).
    >>> CPU 1 grabs both locks as well, as there is no _rcu_barrier() being called
    >>> from notifier callback either.
    >>>
    >>> What did I miss?
    >>
    >> You didn't miss anything, I was suffering a failure to read carefully.
    >>
    >> So my next stupid question is "Why can't kmem_cache_destroy drop
    >> slab_mutex early?" like the following:
    >>
    >> void kmem_cache_destroy(struct kmem_cache *cachep)
    >> {
    >> BUG_ON(!cachep || in_interrupt());
    >>
    >> /* Find the cache in the chain of caches. */
    >> get_online_cpus();
    >> mutex_lock(&slab_mutex);
    >> /*
    >> * the chain is never empty, cache_cache is never destroyed
    >> */
    >> list_del(&cachep->list);
    >> if (__cache_shrink(cachep)) {
    >> slab_error(cachep, "Can't free all objects");
    >> list_add(&cachep->list, &slab_caches);
    >> mutex_unlock(&slab_mutex);
    >> put_online_cpus();
    >> return;
    >> }
    >> mutex_unlock(&slab_mutex);
    >>
    >> if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
    >> rcu_barrier();
    >>
    >> __kmem_cache_destroy(cachep);
    >> put_online_cpus();
    >> }
    >>
    >> Or did I miss some reason why __kmem_cache_destroy() needs that lock?
    >> Looks to me like it is just freeing now-disconnected memory.
    >
    > Good question. I believe it should be safe to drop slab_mutex earlier, as
    > cachep has already been unlinked. I am adding slab people and linux-mm to
    > CC (the whole thread on LKML can be found at
    > https://lkml.org/lkml/2012/10/2/296 for reference).
    >
    > How about the patch below? Pekka, Christoph, please?
    >
    > It makes the lockdep happy again, and obviously removes the deadlock (I
    > tested it).
    >
    >
    >
    > From: Jiri Kosina <jkosina@suse.cz>
    > Subject: 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().
    >
    > This opens a possibilty for deadlock:
    >
    > CPU 0 CPU 1
    > kmem_cache_destroy()
    > mutex_lock(slab_mutex)
    > _cpu_up()
    > cpu_hotplug_begin()
    > mutex_lock(cpu_hotplug.lock)
    > rcu_barrier()
    > _rcu_barrier()
    > get_online_cpus()
    > mutex_lock(cpu_hotplug.lock)
    > (blocks, CPU 1 has the mutex)
    > __cpu_notify()
    > mutex_lock(slab_mutex)

    Hmm.. no, this should *never* happen IMHO!

    If I am seeing the code right, kmem_cache_destroy() wraps its entire content
    inside get/put_online_cpus(), which means it cannot run concurrently with cpu_up()
    or cpu_down(). Are we really hitting a corner case where the refcounting logic
    in get/put_online_cpus() is failing and allowing a hotplug writer to run in
    parallel with a hotplug reader? If yes, *that* is the problem we have to fix..

    Regards,
    Srivatsa S. Bhat

    >
    > It turns out that slab's kmem_cache_destroy() might release slab_mutex
    > earlier before calling out to rcu_barrier(), as cachep has already been
    > unlinked.
    >
    > This patch removes the AB-BA dependency by calling rcu_barrier() with
    > slab_mutex already unlocked.
    >
    > Signed-off-by: Jiri Kosina <jkosina@suse.cz>
    > ---
    > mm/slab.c | 2 +-
    > 1 files changed, 1 insertions(+), 1 deletions(-)
    >
    > diff --git a/mm/slab.c b/mm/slab.c
    > index 1133911..693c7cb 100644
    > --- a/mm/slab.c
    > +++ b/mm/slab.c
    > @@ -2801,12 +2801,12 @@ void kmem_cache_destroy(struct kmem_cache *cachep)
    > put_online_cpus();
    > return;
    > }
    > + mutex_unlock(&slab_mutex);
    >
    > if (unlikely(cachep->flags & SLAB_DESTROY_BY_RCU))
    > rcu_barrier();
    >
    > __kmem_cache_destroy(cachep);
    > - mutex_unlock(&slab_mutex);
    > put_online_cpus();
    > }
    > EXPORT_SYMBOL(kmem_cache_destroy);
    >


    --
    Regards,
    Srivatsa S. Bhat



    \
     
     \ /
      Last update: 2012-10-03 06:41    [W:3.411 / U:0.092 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site