Re: [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 Wed, 3 Oct 2012, Jiri Kosina 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. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 02:54 PM, Jiri Kosina wrote:
> On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
>
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(_hotplug.lock);
if (likely(!cpu_hotplug.refcount)) <
This one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(_hotplug.lock);
schedule();
}
}
>>>
>>> I acutally just came to the same conclusion (7 hours of sleep later, the
>>> mind indeed seems to be brighter ... what a poet I am).
>>>
>>> Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
>>> therefore gets confused by the fact that mutual exclusion is actually
>>> achieved through the refcount instead of mutex (and the same apparently
>>> happened to me).
>>
>> No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
>> our refcounting has messed up somewhere. As a result, we really *are* running
>> a hotplug-reader and a hotplug-writer at the same time! We really need to fix
>> *that*! So please try the second debug patch I sent just now (with the
>> BUG_ON()
>> in put_online_cpus()). We need to know who is calling put_online_cpus() twice
>> and fix that culprit!
>
> I don't think so.
>
> Lockdep is complaining, because
>
> (a) during system bootup, the smp_init() -> cpu_up() -> cpuup_callback()
> teaches him about hotplug.lock -> slab_mutex dependency
>
> (b) many many jiffies later, nf_conntrack_cleanup_net() calls
> kmem_cache_destroy(), which introduces slab_mutex -> hotplug.lock
> dependency
>
> Lockdep rightfully (from his POV) sees this as potential ABBA, and reports
> it, it's as simple as that.
> It has no way of knowing the fact that the ABBA can actually never happen,
> because of special semantics of cpu_hotplug.refcount and it's handling in
> cpu_hotplug_begin().
>
Hmm, you are right.
> The "neither cpu_up() nor cpu_down() will proceed past cpu_hotplug_begin()
> until everyone who called get_online_cpus() will call put_online_cpus()"
> is totally invisible to lockdep.
I see your point..
>
>>> So right, now I agree that the deadlock scenario I have come up with is
>>> indeed bogus (*), and we just have to annotate this fact to lockdep
>>> somehow.
>>
>> Yes, the deadlock scenario is bogus, but the refcounting leak is for real
>> and needs fixing.
>
> With your patch applied, the BUG_ON() in put_online_cpus() didn't trigger
> for me at all. Which is what I expected.
Oh, ok..
>
>> I'm fine with this, but the real problem is elsewhere, like I mentioned
>> above.
>> This one is only a good-to-have, not a fix.
>>
>>> (*) I have seen machine locking hard reproducibly, but that was only with
>>> additional Paul's patch, so I guess the lock order there actually was
>>> wrong
>>
>> If refcounting was working fine, Paul's patch wouldn't have caused *any*
>> issues.
>> With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
>> kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
>> get_online_cpus() until the slab cpu hotplug notifier as well as the entire
>> cpu_up operation completes. Absolutely no problem in that! So the fact that
>> you are seeing lock-ups here is another indication that the problem is really
>> elsewhere!
>
> I don't agree. The reason why Paul's patch (1331e7a1bb) started to trigger
> this, is that (b) above doesn't exist in pre-1331e7a1bb kernels.
>
So basically what you are saying is, the calltraces in the lockdep splat are
from
different points in time right? Then I see why its just a false positive and not
a real bug.
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
> >> static void cpu_hotplug_begin(void)
> >> {
> >> cpu_hotplug.active_writer = current;
> >>
> >> for (;;) {
> >> mutex_lock(_hotplug.lock);
> >> if (likely(!cpu_hotplug.refcount)) <
> >> This one!
> >> break;
> >> __set_current_state(TASK_UNINTERRUPTIBLE);
> >> mutex_unlock(_hotplug.lock);
> >> schedule();
> >> }
> >> }
> >
> > I acutally just came to the same conclusion (7 hours of sleep later, the
> > mind indeed seems to be brighter ... what a poet I am).
> >
> > Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
> > therefore gets confused by the fact that mutual exclusion is actually
> > achieved through the refcount instead of mutex (and the same apparently
> > happened to me).
>
> No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
> our refcounting has messed up somewhere. As a result, we really *are* running
> a hotplug-reader and a hotplug-writer at the same time! We really need to fix
> *that*! So please try the second debug patch I sent just now (with the
> BUG_ON()
> in put_online_cpus()). We need to know who is calling put_online_cpus() twice
> and fix that culprit!
I don't think so.
Lockdep is complaining, because
(a) during system bootup, the smp_init() -> cpu_up() -> cpuup_callback()
teaches him about hotplug.lock -> slab_mutex dependency
(b) many many jiffies later, nf_conntrack_cleanup_net() calls
kmem_cache_destroy(), which introduces slab_mutex -> hotplug.lock
dependency
Lockdep rightfully (from his POV) sees this as potential ABBA, and reports
it, it's as simple as that.
It has no way of knowing the fact that the ABBA can actually never happen,
because of special semantics of cpu_hotplug.refcount and it's handling in
cpu_hotplug_begin().
The "neither cpu_up() nor cpu_down() will proceed past cpu_hotplug_begin()
until everyone who called get_online_cpus() will call put_online_cpus()"
is totally invisible to lockdep.
> > So right, now I agree that the deadlock scenario I have come up with is
> > indeed bogus (*), and we just have to annotate this fact to lockdep
> > somehow.
>
> Yes, the deadlock scenario is bogus, but the refcounting leak is for real
> and needs fixing.
With your patch applied, the BUG_ON() in put_online_cpus() didn't trigger
for me at all. Which is what I expected.
> I'm fine with this, but the real problem is elsewhere, like I mentioned above.
> This one is only a good-to-have, not a fix.
>
> > (*) I have seen machine locking hard reproducibly, but that was only with
> > additional Paul's patch, so I guess the lock order there actually was
> > wrong
>
> If refcounting was working fine, Paul's patch wouldn't have caused *any*
> issues.
> With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
> kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
> get_online_cpus() until the slab cpu hotplug notifier as well as the entire
> cpu_up operation completes. Absolutely no problem in that! So the fact that
> you are seeing lock-ups here is another indication that the problem is really
> elsewhere!
I don't agree. The reason why Paul's patch (1331e7a1bb) started to trigger
this, is that (b) above doesn't exist in pre-1331e7a1bb kernels.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 01:49 PM, Jiri Kosina wrote:
> On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
>
>> On 10/03/2012 01:13 PM, Jiri Kosina wrote:
>>> On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
>>>
>>> CPU 0 CPU 1
>>> kmem_cache_destroy()
>>
>> What about the get_online_cpus() right here at CPU0 before
>> calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
>> on CPU1?? I still don't get it... :(
>>
>> (kmem_cache_destroy() uses get/put_online_cpus() around acquiring
>> and releasing slab_mutex).
>
> The problem is that there is a CPU-hotplug notifier for slab, which
> establishes hotplug->slab.
Agreed.
> Then having kmem_cache_destroy() call
> rcu_barrier() under the lock
Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
this point in time, because it has invoked get_online_cpus()! It simply
cannot be running past that point in the presence of a running hotplug
notifier! So, kmem_cache_destroy() should have been sleeping on the
hotplug lock, waiting for the notifier to release it, no?
>>>
>>> Please look carefully at the scenario again. kmem_cache_destroy() calls
>>> get_online_cpus() before the hotplug notifier even starts. Hence it has no
>>> reason to block there (noone is holding hotplug lock).
>>>
>>
>> Agreed.
>>
>>> *Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
>>
>> Ah, that's the problem! The hotplug reader-writer synchronization is not just
>> via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
>> incremented
>> the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
>> immediately
>> and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a
>> hotplug-writer
>> (cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
>>
>>
>> Take a look at the hotplug lock acquire function at the writer side:
>>
>> static void cpu_hotplug_begin(void)
>> {
>> cpu_hotplug.active_writer = current;
>>
>> for (;;) {
>> mutex_lock(_hotplug.lock);
>> if (likely(!cpu_hotplug.refcount)) < This
>> one!
>> break;
>> __set_current_state(TASK_UNINTERRUPTIBLE);
>> mutex_unlock(_hotplug.lock);
>> schedule();
>> }
>> }
>
> I acutally just came to the same conclusion (7 hours of sleep later, the
> mind indeed seems to be brighter ... what a poet I am).
>
> Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
> therefore gets confused by the fact that mutual exclusion is actually
> achieved through the refcount instead of mutex (and the same apparently
> happened to me).
No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
our refcounting has messed up somewhere. As a result, we really *are* running
a hotplug-reader and a hotplug-writer at the same time! We really need to fix
*that*! So please try the second debug patch I sent just now (with the BUG_ON()
in put_online_cpus()). We need to know who is calling put_online_cpus() twice
and fix that culprit!
>
> So right, now I agree that the deadlock scenario I have come up with is
> indeed bogus (*), and we just have to annotate this fact to lockdep
> somehow.
Yes, the deadlock scenario is bogus, but the refcounting leak is for real
and needs fixing.
>
> And I actually believe that moving the slab_mutex around in
> kmem_cache_destroy() is a good anotation (maybe worth a separate comment
> in the code), as it not only makes the lockdep false positive go away, but
> it also reduces the mutex hold time.
>
I'm fine with this, but the real problem is elsewhere, like I mentioned above.
This one is only a good-to-have, not a fix.
> (*) I have seen machine locking hard reproducibly, but that was only with
> additional Paul's patch, so I guess the lock order there actually was
> wrong
If refcounting was working fine, Paul's patch wouldn't have caused *any* issues.
With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
get_online_cpus() until the slab cpu hotplug notifier as well as the entire
cpu_up operation completes. Absolutely no problem in that! So the fact that
you are seeing lock-ups here is another indication that the problem is really
elsewhere!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [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 11:38 AM, Srivatsa S. Bhat wrote:
> On 10/03/2012 09:20 AM, Srivatsa S. Bhat wrote:
>> 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(_mutex);
/*
* the chain is never empty, cache_cache is never destroyed
*/
list_del(>list);
if (__cache_shrink(cachep)) {
slab_error(cachep, "Can't free all objects");
list_add(>list, _caches);
mutex_unlock(_mutex);
put_online_cpus();
return;
}
mutex_unlock(_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
>>> 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()
>>>
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
> On 10/03/2012 01:13 PM, Jiri Kosina wrote:
> > On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
> >
> > CPU 0 CPU 1
> > kmem_cache_destroy()
>
> What about the get_online_cpus() right here at CPU0 before
> calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
> on CPU1?? I still don't get it... :(
>
> (kmem_cache_destroy() uses get/put_online_cpus() around acquiring
> and releasing slab_mutex).
> >>>
> >>> The problem is that there is a CPU-hotplug notifier for slab, which
> >>> establishes hotplug->slab.
> >>
> >> Agreed.
> >>
> >>> Then having kmem_cache_destroy() call
> >>> rcu_barrier() under the lock
> >>
> >> Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
> >> this point in time, because it has invoked get_online_cpus()! It simply
> >> cannot be running past that point in the presence of a running hotplug
> >> notifier! So, kmem_cache_destroy() should have been sleeping on the
> >> hotplug lock, waiting for the notifier to release it, no?
> >
> > Please look carefully at the scenario again. kmem_cache_destroy() calls
> > get_online_cpus() before the hotplug notifier even starts. Hence it has no
> > reason to block there (noone is holding hotplug lock).
> >
>
> Agreed.
>
> > *Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
>
> Ah, that's the problem! The hotplug reader-writer synchronization is not just
> via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
> incremented
> the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
> immediately
> and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a
> hotplug-writer
> (cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
>
>
> Take a look at the hotplug lock acquire function at the writer side:
>
> static void cpu_hotplug_begin(void)
> {
> cpu_hotplug.active_writer = current;
>
> for (;;) {
> mutex_lock(_hotplug.lock);
> if (likely(!cpu_hotplug.refcount)) < This
> one!
> break;
> __set_current_state(TASK_UNINTERRUPTIBLE);
> mutex_unlock(_hotplug.lock);
> schedule();
> }
> }
I acutally just came to the same conclusion (7 hours of sleep later, the
mind indeed seems to be brighter ... what a poet I am).
Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
therefore gets confused by the fact that mutual exclusion is actually
achieved through the refcount instead of mutex (and the same apparently
happened to me).
So right, now I agree that the deadlock scenario I have come up with is
indeed bogus (*), and we just have to annotate this fact to lockdep
somehow.
And I actually believe that moving the slab_mutex around in
kmem_cache_destroy() is a good anotation (maybe worth a separate comment
in the code), as it not only makes the lockdep false positive go away, but
it also reduces the mutex hold time.
(*) I have seen machine locking hard reproducibly, but that was only with
additional Paul's patch, so I guess the lock order there actually was
wrong
Thanks!
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 01:13 PM, Jiri Kosina wrote:
> On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
>
> CPU 0 CPU 1
> kmem_cache_destroy()
What about the get_online_cpus() right here at CPU0 before
calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
on CPU1?? I still don't get it... :(
(kmem_cache_destroy() uses get/put_online_cpus() around acquiring
and releasing slab_mutex).
>>>
>>> The problem is that there is a CPU-hotplug notifier for slab, which
>>> establishes hotplug->slab.
>>
>> Agreed.
>>
>>> Then having kmem_cache_destroy() call
>>> rcu_barrier() under the lock
>>
>> Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
>> this point in time, because it has invoked get_online_cpus()! It simply
>> cannot be running past that point in the presence of a running hotplug
>> notifier! So, kmem_cache_destroy() should have been sleeping on the
>> hotplug lock, waiting for the notifier to release it, no?
>
> Please look carefully at the scenario again. kmem_cache_destroy() calls
> get_online_cpus() before the hotplug notifier even starts. Hence it has no
> reason to block there (noone is holding hotplug lock).
>
Agreed.
> *Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
Ah, that's the problem! The hotplug reader-writer synchronization is not just
via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
incremented
the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
immediately
and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a hotplug-writer
(cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
Take a look at the hotplug lock acquire function at the writer side:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(_hotplug.lock);
if (likely(!cpu_hotplug.refcount)) < This one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(_hotplug.lock);
schedule();
}
}
> kmem_cache_destroy() calls rcu_barrier in the meantime, and blocks itself
> on the hotplug lock there.
>
> Please note that the get_online_cpus() call in kmem_cache_destroy()
> doesn't play *any* role in this scenario.
>
Please consider my thoughts above. You'll see why I'm not convinced.
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: > >>> CPU 0 CPU 1 > >>> kmem_cache_destroy() > >> > >> What about the get_online_cpus() right here at CPU0 before > >> calling mutex_lock(slab_mutex)? How can the cpu_up() proceed > >> on CPU1?? I still don't get it... :( > >> > >> (kmem_cache_destroy() uses get/put_online_cpus() around acquiring > >> and releasing slab_mutex). > > > > The problem is that there is a CPU-hotplug notifier for slab, which > > establishes hotplug->slab. > > Agreed. > > > Then having kmem_cache_destroy() call > > rcu_barrier() under the lock > > Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at > this point in time, because it has invoked get_online_cpus()! It simply > cannot be running past that point in the presence of a running hotplug > notifier! So, kmem_cache_destroy() should have been sleeping on the > hotplug lock, waiting for the notifier to release it, no? Please look carefully at the scenario again. kmem_cache_destroy() calls get_online_cpus() before the hotplug notifier even starts. Hence it has no reason to block there (noone is holding hotplug lock). *Then* hotplug notifier fires up, succeeds obtaining hotplug lock, kmem_cache_destroy() calls rcu_barrier in the meantime, and blocks itself on the hotplug lock there. Please note that the get_online_cpus() call in kmem_cache_destroy() doesn't play *any* role in this scenario. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 09:20 AM, Srivatsa S. Bhat wrote:
> 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(_mutex);
>>> /*
>>> * the chain is never empty, cache_cache is never destroyed
>>> */
>>> list_del(>list);
>>> if (__cache_shrink(cachep)) {
>>> slab_error(cachep, "Can't free all objects");
>>> list_add(>list, _caches);
>>> mutex_unlock(_mutex);
>>> put_online_cpus();
>>> return;
>>> }
>>> mutex_unlock(_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
>> 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)
>>
Re: [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 09:20 AM, Srivatsa S. Bhat wrote:
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 jkos...@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().
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: CPU 0 CPU 1 kmem_cache_destroy() What about the get_online_cpus() right here at CPU0 before calling mutex_lock(slab_mutex)? How can the cpu_up() proceed on CPU1?? I still don't get it... :( (kmem_cache_destroy() uses get/put_online_cpus() around acquiring and releasing slab_mutex). The problem is that there is a CPU-hotplug notifier for slab, which establishes hotplug-slab. Agreed. Then having kmem_cache_destroy() call rcu_barrier() under the lock Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at this point in time, because it has invoked get_online_cpus()! It simply cannot be running past that point in the presence of a running hotplug notifier! So, kmem_cache_destroy() should have been sleeping on the hotplug lock, waiting for the notifier to release it, no? Please look carefully at the scenario again. kmem_cache_destroy() calls get_online_cpus() before the hotplug notifier even starts. Hence it has no reason to block there (noone is holding hotplug lock). *Then* hotplug notifier fires up, succeeds obtaining hotplug lock, kmem_cache_destroy() calls rcu_barrier in the meantime, and blocks itself on the hotplug lock there. Please note that the get_online_cpus() call in kmem_cache_destroy() doesn't play *any* role in this scenario. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 01:13 PM, Jiri Kosina wrote:
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
CPU 0 CPU 1
kmem_cache_destroy()
What about the get_online_cpus() right here at CPU0 before
calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
on CPU1?? I still don't get it... :(
(kmem_cache_destroy() uses get/put_online_cpus() around acquiring
and releasing slab_mutex).
The problem is that there is a CPU-hotplug notifier for slab, which
establishes hotplug-slab.
Agreed.
Then having kmem_cache_destroy() call
rcu_barrier() under the lock
Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
this point in time, because it has invoked get_online_cpus()! It simply
cannot be running past that point in the presence of a running hotplug
notifier! So, kmem_cache_destroy() should have been sleeping on the
hotplug lock, waiting for the notifier to release it, no?
Please look carefully at the scenario again. kmem_cache_destroy() calls
get_online_cpus() before the hotplug notifier even starts. Hence it has no
reason to block there (noone is holding hotplug lock).
Agreed.
*Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
Ah, that's the problem! The hotplug reader-writer synchronization is not just
via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
incremented
the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
immediately
and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a hotplug-writer
(cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
Take a look at the hotplug lock acquire function at the writer side:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount)) This one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(cpu_hotplug.lock);
schedule();
}
}
kmem_cache_destroy() calls rcu_barrier in the meantime, and blocks itself
on the hotplug lock there.
Please note that the get_online_cpus() call in kmem_cache_destroy()
doesn't play *any* role in this scenario.
Please consider my thoughts above. You'll see why I'm not convinced.
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
On 10/03/2012 01:13 PM, Jiri Kosina wrote:
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
CPU 0 CPU 1
kmem_cache_destroy()
What about the get_online_cpus() right here at CPU0 before
calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
on CPU1?? I still don't get it... :(
(kmem_cache_destroy() uses get/put_online_cpus() around acquiring
and releasing slab_mutex).
The problem is that there is a CPU-hotplug notifier for slab, which
establishes hotplug-slab.
Agreed.
Then having kmem_cache_destroy() call
rcu_barrier() under the lock
Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
this point in time, because it has invoked get_online_cpus()! It simply
cannot be running past that point in the presence of a running hotplug
notifier! So, kmem_cache_destroy() should have been sleeping on the
hotplug lock, waiting for the notifier to release it, no?
Please look carefully at the scenario again. kmem_cache_destroy() calls
get_online_cpus() before the hotplug notifier even starts. Hence it has no
reason to block there (noone is holding hotplug lock).
Agreed.
*Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
Ah, that's the problem! The hotplug reader-writer synchronization is not just
via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
incremented
the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
immediately
and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a
hotplug-writer
(cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
Take a look at the hotplug lock acquire function at the writer side:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount)) This
one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(cpu_hotplug.lock);
schedule();
}
}
I acutally just came to the same conclusion (7 hours of sleep later, the
mind indeed seems to be brighter ... what a poet I am).
Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
therefore gets confused by the fact that mutual exclusion is actually
achieved through the refcount instead of mutex (and the same apparently
happened to me).
So right, now I agree that the deadlock scenario I have come up with is
indeed bogus (*), and we just have to annotate this fact to lockdep
somehow.
And I actually believe that moving the slab_mutex around in
kmem_cache_destroy() is a good anotation (maybe worth a separate comment
in the code), as it not only makes the lockdep false positive go away, but
it also reduces the mutex hold time.
(*) I have seen machine locking hard reproducibly, but that was only with
additional Paul's patch, so I guess the lock order there actually was
wrong
Thanks!
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [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 11:38 AM, Srivatsa S. Bhat wrote:
On 10/03/2012 09:20 AM, Srivatsa S. Bhat wrote:
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 jkos...@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
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 01:49 PM, Jiri Kosina wrote:
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
On 10/03/2012 01:13 PM, Jiri Kosina wrote:
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
CPU 0 CPU 1
kmem_cache_destroy()
What about the get_online_cpus() right here at CPU0 before
calling mutex_lock(slab_mutex)? How can the cpu_up() proceed
on CPU1?? I still don't get it... :(
(kmem_cache_destroy() uses get/put_online_cpus() around acquiring
and releasing slab_mutex).
The problem is that there is a CPU-hotplug notifier for slab, which
establishes hotplug-slab.
Agreed.
Then having kmem_cache_destroy() call
rcu_barrier() under the lock
Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at
this point in time, because it has invoked get_online_cpus()! It simply
cannot be running past that point in the presence of a running hotplug
notifier! So, kmem_cache_destroy() should have been sleeping on the
hotplug lock, waiting for the notifier to release it, no?
Please look carefully at the scenario again. kmem_cache_destroy() calls
get_online_cpus() before the hotplug notifier even starts. Hence it has no
reason to block there (noone is holding hotplug lock).
Agreed.
*Then* hotplug notifier fires up, succeeds obtaining hotplug lock,
Ah, that's the problem! The hotplug reader-writer synchronization is not just
via a simple mutex. Its a refcount underneath. If kmem_cache_destroy()
incremented
the refcount, the hotplug-writer (cpu_up) will release the hotplug lock
immediately
and try again. IOW, a hotplug-reader (kmem_cache_destroy()) and a
hotplug-writer
(cpu_up) can *NEVER* run concurrently. If they do, we are totally screwed!
Take a look at the hotplug lock acquire function at the writer side:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount)) This
one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(cpu_hotplug.lock);
schedule();
}
}
I acutally just came to the same conclusion (7 hours of sleep later, the
mind indeed seems to be brighter ... what a poet I am).
Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
therefore gets confused by the fact that mutual exclusion is actually
achieved through the refcount instead of mutex (and the same apparently
happened to me).
No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
our refcounting has messed up somewhere. As a result, we really *are* running
a hotplug-reader and a hotplug-writer at the same time! We really need to fix
*that*! So please try the second debug patch I sent just now (with the BUG_ON()
in put_online_cpus()). We need to know who is calling put_online_cpus() twice
and fix that culprit!
So right, now I agree that the deadlock scenario I have come up with is
indeed bogus (*), and we just have to annotate this fact to lockdep
somehow.
Yes, the deadlock scenario is bogus, but the refcounting leak is for real
and needs fixing.
And I actually believe that moving the slab_mutex around in
kmem_cache_destroy() is a good anotation (maybe worth a separate comment
in the code), as it not only makes the lockdep false positive go away, but
it also reduces the mutex hold time.
I'm fine with this, but the real problem is elsewhere, like I mentioned above.
This one is only a good-to-have, not a fix.
(*) I have seen machine locking hard reproducibly, but that was only with
additional Paul's patch, so I guess the lock order there actually was
wrong
If refcounting was working fine, Paul's patch wouldn't have caused *any* issues.
With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
get_online_cpus() until the slab cpu hotplug notifier as well as the entire
cpu_up operation completes. Absolutely no problem in that! So the fact that
you are seeing lock-ups here is another indication that the problem is really
elsewhere!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount))
This one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(cpu_hotplug.lock);
schedule();
}
}
I acutally just came to the same conclusion (7 hours of sleep later, the
mind indeed seems to be brighter ... what a poet I am).
Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
therefore gets confused by the fact that mutual exclusion is actually
achieved through the refcount instead of mutex (and the same apparently
happened to me).
No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
our refcounting has messed up somewhere. As a result, we really *are* running
a hotplug-reader and a hotplug-writer at the same time! We really need to fix
*that*! So please try the second debug patch I sent just now (with the
BUG_ON()
in put_online_cpus()). We need to know who is calling put_online_cpus() twice
and fix that culprit!
I don't think so.
Lockdep is complaining, because
(a) during system bootup, the smp_init() - cpu_up() - cpuup_callback()
teaches him about hotplug.lock - slab_mutex dependency
(b) many many jiffies later, nf_conntrack_cleanup_net() calls
kmem_cache_destroy(), which introduces slab_mutex - hotplug.lock
dependency
Lockdep rightfully (from his POV) sees this as potential ABBA, and reports
it, it's as simple as that.
It has no way of knowing the fact that the ABBA can actually never happen,
because of special semantics of cpu_hotplug.refcount and it's handling in
cpu_hotplug_begin().
The neither cpu_up() nor cpu_down() will proceed past cpu_hotplug_begin()
until everyone who called get_online_cpus() will call put_online_cpus()
is totally invisible to lockdep.
So right, now I agree that the deadlock scenario I have come up with is
indeed bogus (*), and we just have to annotate this fact to lockdep
somehow.
Yes, the deadlock scenario is bogus, but the refcounting leak is for real
and needs fixing.
With your patch applied, the BUG_ON() in put_online_cpus() didn't trigger
for me at all. Which is what I expected.
I'm fine with this, but the real problem is elsewhere, like I mentioned above.
This one is only a good-to-have, not a fix.
(*) I have seen machine locking hard reproducibly, but that was only with
additional Paul's patch, so I guess the lock order there actually was
wrong
If refcounting was working fine, Paul's patch wouldn't have caused *any*
issues.
With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
get_online_cpus() until the slab cpu hotplug notifier as well as the entire
cpu_up operation completes. Absolutely no problem in that! So the fact that
you are seeing lock-ups here is another indication that the problem is really
elsewhere!
I don't agree. The reason why Paul's patch (1331e7a1bb) started to trigger
this, is that (b) above doesn't exist in pre-1331e7a1bb kernels.
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 02:54 PM, Jiri Kosina wrote:
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote:
static void cpu_hotplug_begin(void)
{
cpu_hotplug.active_writer = current;
for (;;) {
mutex_lock(cpu_hotplug.lock);
if (likely(!cpu_hotplug.refcount))
This one!
break;
__set_current_state(TASK_UNINTERRUPTIBLE);
mutex_unlock(cpu_hotplug.lock);
schedule();
}
}
I acutally just came to the same conclusion (7 hours of sleep later, the
mind indeed seems to be brighter ... what a poet I am).
Lockdep doesn't know about this semantics of cpu_hotplug_begin(), and
therefore gets confused by the fact that mutual exclusion is actually
achieved through the refcount instead of mutex (and the same apparently
happened to me).
No, that's not the problem. Lockdep is fine. The calltrace clearly shows that
our refcounting has messed up somewhere. As a result, we really *are* running
a hotplug-reader and a hotplug-writer at the same time! We really need to fix
*that*! So please try the second debug patch I sent just now (with the
BUG_ON()
in put_online_cpus()). We need to know who is calling put_online_cpus() twice
and fix that culprit!
I don't think so.
Lockdep is complaining, because
(a) during system bootup, the smp_init() - cpu_up() - cpuup_callback()
teaches him about hotplug.lock - slab_mutex dependency
(b) many many jiffies later, nf_conntrack_cleanup_net() calls
kmem_cache_destroy(), which introduces slab_mutex - hotplug.lock
dependency
Lockdep rightfully (from his POV) sees this as potential ABBA, and reports
it, it's as simple as that.
It has no way of knowing the fact that the ABBA can actually never happen,
because of special semantics of cpu_hotplug.refcount and it's handling in
cpu_hotplug_begin().
Hmm, you are right.
The neither cpu_up() nor cpu_down() will proceed past cpu_hotplug_begin()
until everyone who called get_online_cpus() will call put_online_cpus()
is totally invisible to lockdep.
I see your point..
So right, now I agree that the deadlock scenario I have come up with is
indeed bogus (*), and we just have to annotate this fact to lockdep
somehow.
Yes, the deadlock scenario is bogus, but the refcounting leak is for real
and needs fixing.
With your patch applied, the BUG_ON() in put_online_cpus() didn't trigger
for me at all. Which is what I expected.
Oh, ok..
I'm fine with this, but the real problem is elsewhere, like I mentioned
above.
This one is only a good-to-have, not a fix.
(*) I have seen machine locking hard reproducibly, but that was only with
additional Paul's patch, so I guess the lock order there actually was
wrong
If refcounting was working fine, Paul's patch wouldn't have caused *any*
issues.
With that patch in place, the 2 places where rcu_barrier() get invoked (ie.,
kmem_cache_destroy() and deactivate_locked_super()) both start waiting on
get_online_cpus() until the slab cpu hotplug notifier as well as the entire
cpu_up operation completes. Absolutely no problem in that! So the fact that
you are seeing lock-ups here is another indication that the problem is really
elsewhere!
I don't agree. The reason why Paul's patch (1331e7a1bb) started to trigger
this, is that (b) above doesn't exist in pre-1331e7a1bb kernels.
So basically what you are saying is, the calltraces in the lockdep splat are
from
different points in time right? Then I see why its just a false positive and not
a real bug.
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [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 Wed, 3 Oct 2012, Jiri Kosina 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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 09:37 AM, Paul E. McKenney wrote:
> On Wed, Oct 03, 2012 at 09:29:01AM +0530, Srivatsa S. Bhat wrote:
>> On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
>>> On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
On Tue, 2 Oct 2012, Jiri Kosina wrote:
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of
removing
the need for _rcu_barrier() to adopt callbacks: Because
CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney
Signed-off-by: Paul E. McKenney
Reviewed-by: Josh Triplett
==
is causing lockdep to complain (see the full trace below). I haven't
yet
had time to analyze what exactly is happening, and probably will not
have
time to do so until tomorrow, so just sending this as a heads-up in
case
anyone sees the culprit immediately.
>>>
>>> Hmmm... Does the following patch help? It swaps the order in which
>>> rcu_barrier() acquires the hotplug and rcu_barrier locks.
>>
>> It changed the report slightly (see for example the change in possible
>> unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
>> now directly about cpu_hotplug.lock). With the patch applied I get
>>
>>
>>
>> ==
>> [ INFO: possible circular locking dependency detected ]
>> 3.6.0-03888-g3f99f3b #145 Not tainted
>
> And it really seems valid.
>>>
>>> Yep, it sure is. I wasn't getting the full picture earlier, so please
>>> accept my apologies for the bogus patch.
>>>
> kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
> introduces slab_mutex -> cpu_hotplug.lock dependency (through
> rcu_barrier() -> _rcu_barrier() -> get_online_cpus()).
>
> On the other hand, _cpu_up() acquires cpu_hotplug.lock through
> cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
> gets called, which acquires slab_mutex. This gives the reverse
> dependency,
> i.e. deadlock scenario is valid one.
>
> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
> before that, there was no slab_mutex -> cpu_hotplug.lock dependency.
>
> Simply put, the commit causes get_online_cpus() to be called with
> slab_mutex held, which is invalid.
Oh, and it seems to be actually triggering in real.
With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
your patch, changing the order in which rcu_barrier() acquires hotplug and
rcu_barrier locks, the machine hangs 100% reliably during suspend, which
very likely actually is the deadlock described above.
>>>
>>> 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.
>>
>> Why not? IMHO it should have been perfectly fine! See below...
>>
>>> 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...
>>>
>>
>> The thing is, get_online_cpus() is smart: it *knows* when you are calling
>> it in a hotplug-writer, IOW, when you are in a hotplug notifier.
>>
>> The relevant code is:
>>
>> void get_online_cpus(void)
>> {
>> might_sleep();
>> if (cpu_hotplug.active_writer == current)
>> return;
>>
>> }
>>
>> So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
>> notifier
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, Oct 03, 2012 at 09:29:01AM +0530, Srivatsa S. Bhat wrote:
> On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
> > On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
> >> On Tue, 2 Oct 2012, Jiri Kosina wrote:
> >>
> >> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
> >> commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
> >> Author: Paul E. McKenney
> >> Date: Thu Aug 2 17:43:50 2012 -0700
> >>
> >> rcu: Remove _rcu_barrier() dependency on __stop_machine()
> >>
> >> Currently, _rcu_barrier() relies on preempt_disable() to prevent
> >> any CPU from going offline, which in turn depends on CPU hotplug's
> >> use of __stop_machine().
> >>
> >> This patch therefore makes _rcu_barrier() use get_online_cpus() to
> >> block CPU-hotplug operations. This has the added benefit of
> >> removing
> >> the need for _rcu_barrier() to adopt callbacks: Because
> >> CPU-hotplug
> >> operations are excluded, there can be no callbacks to adopt. This
> >> commit simplifies the code accordingly.
> >>
> >> Signed-off-by: Paul E. McKenney
> >> Signed-off-by: Paul E. McKenney
> >> Reviewed-by: Josh Triplett
> >> ==
> >>
> >> is causing lockdep to complain (see the full trace below). I haven't
> >> yet
> >> had time to analyze what exactly is happening, and probably will not
> >> have
> >> time to do so until tomorrow, so just sending this as a heads-up in
> >> case
> >> anyone sees the culprit immediately.
> >
> > Hmmm... Does the following patch help? It swaps the order in which
> > rcu_barrier() acquires the hotplug and rcu_barrier locks.
>
> It changed the report slightly (see for example the change in possible
> unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
> now directly about cpu_hotplug.lock). With the patch applied I get
>
>
>
> ==
> [ INFO: possible circular locking dependency detected ]
> 3.6.0-03888-g3f99f3b #145 Not tainted
> >>>
> >>> And it really seems valid.
> >
> > Yep, it sure is. I wasn't getting the full picture earlier, so please
> > accept my apologies for the bogus patch.
> >
> >>> kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
> >>> introduces slab_mutex -> cpu_hotplug.lock dependency (through
> >>> rcu_barrier() -> _rcu_barrier() -> get_online_cpus()).
> >>>
> >>> On the other hand, _cpu_up() acquires cpu_hotplug.lock through
> >>> cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
> >>> gets called, which acquires slab_mutex. This gives the reverse
> >>> dependency,
> >>> i.e. deadlock scenario is valid one.
> >>>
> >>> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
> >>> before that, there was no slab_mutex -> cpu_hotplug.lock dependency.
> >>>
> >>> Simply put, the commit causes get_online_cpus() to be called with
> >>> slab_mutex held, which is invalid.
> >>
> >> Oh, and it seems to be actually triggering in real.
> >>
> >> With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
> >> your patch, changing the order in which rcu_barrier() acquires hotplug and
> >> rcu_barrier locks, the machine hangs 100% reliably during suspend, which
> >> very likely actually is the deadlock described above.
> >
> > 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.
>
> Why not? IMHO it should have been perfectly fine! See below...
>
> > 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...
> >
>
> The thing is, get_online_cpus() is smart: it *knows* when you are calling
> it in a hotplug-writer, IOW, when you are in a hotplug notifier.
>
> The relevant code is:
>
> void get_online_cpus(void)
> {
> might_sleep();
> if (cpu_hotplug.active_writer == current)
> return;
>
> }
>
> So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
> notifier should pose no problem at all!
Indeed, that was my confusion.
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 09:14 AM, Paul E. McKenney wrote: > On Wed, Oct 03, 2012 at 09:05:31AM +0530, Srivatsa S. Bhat wrote: >> On 10/03/2012 03:47 AM, Jiri Kosina wrote: >>> On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: >>> I don't see how this circular locking dependency can occur.. If you are using SLUB, kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you are using SLAB, kmem_cache_destroy() wraps its whole operation inside get/put_online_cpus(), which means, it cannot run concurrently with a hotplug operation such as cpu_up(). So, I'm rather puzzled at this lockdep splat.. >>> >>> I am using SLAB here. >>> >>> The scenario I think is very well possible: >>> >>> >>> CPU 0 CPU 1 >>> kmem_cache_destroy() >> >> What about the get_online_cpus() right here at CPU0 before >> calling mutex_lock(slab_mutex)? How can the cpu_up() proceed >> on CPU1?? I still don't get it... :( >> >> (kmem_cache_destroy() uses get/put_online_cpus() around acquiring >> and releasing slab_mutex). > > The problem is that there is a CPU-hotplug notifier for slab, which > establishes hotplug->slab. Agreed. > Then having kmem_cache_destroy() call > rcu_barrier() under the lock Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at this point in time, because it has invoked get_online_cpus()! It simply cannot be running past that point in the presence of a running hotplug notifier! So, kmem_cache_destroy() should have been sleeping on the hotplug lock, waiting for the notifier to release it, no? > establishes slab->hotplug, which results > in deadlock. Jiri really did explain this in an earlier email > message, but both of us managed to miss it. ;-) > Maybe I'm just being blind, sorry! ;-) Regards, Srivatsa S. Bhat > Thanx, Paul > >> Regards, >> Srivatsa S. Bhat >> >>> 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) >>> >>> Deadlock. >>> >>> Right? >>> >> >> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
> On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
>> On Tue, 2 Oct 2012, Jiri Kosina wrote:
>>
>> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
>> commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
>> Author: Paul E. McKenney
>> Date: Thu Aug 2 17:43:50 2012 -0700
>>
>> rcu: Remove _rcu_barrier() dependency on __stop_machine()
>>
>> Currently, _rcu_barrier() relies on preempt_disable() to prevent
>> any CPU from going offline, which in turn depends on CPU hotplug's
>> use of __stop_machine().
>>
>> This patch therefore makes _rcu_barrier() use get_online_cpus() to
>> block CPU-hotplug operations. This has the added benefit of removing
>> the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
>> operations are excluded, there can be no callbacks to adopt. This
>> commit simplifies the code accordingly.
>>
>> Signed-off-by: Paul E. McKenney
>> Signed-off-by: Paul E. McKenney
>> Reviewed-by: Josh Triplett
>> ==
>>
>> is causing lockdep to complain (see the full trace below). I haven't yet
>> had time to analyze what exactly is happening, and probably will not
>> have
>> time to do so until tomorrow, so just sending this as a heads-up in case
>> anyone sees the culprit immediately.
>
> Hmmm... Does the following patch help? It swaps the order in which
> rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
>>>
>>> And it really seems valid.
>
> Yep, it sure is. I wasn't getting the full picture earlier, so please
> accept my apologies for the bogus patch.
>
>>> kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
>>> introduces slab_mutex -> cpu_hotplug.lock dependency (through
>>> rcu_barrier() -> _rcu_barrier() -> get_online_cpus()).
>>>
>>> On the other hand, _cpu_up() acquires cpu_hotplug.lock through
>>> cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
>>> gets called, which acquires slab_mutex. This gives the reverse dependency,
>>> i.e. deadlock scenario is valid one.
>>>
>>> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
>>> before that, there was no slab_mutex -> cpu_hotplug.lock dependency.
>>>
>>> Simply put, the commit causes get_online_cpus() to be called with
>>> slab_mutex held, which is invalid.
>>
>> Oh, and it seems to be actually triggering in real.
>>
>> With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
>> your patch, changing the order in which rcu_barrier() acquires hotplug and
>> rcu_barrier locks, the machine hangs 100% reliably during suspend, which
>> very likely actually is the deadlock described above.
>
> 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.
Why not? IMHO it should have been perfectly fine! See below...
> 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...
>
The thing is, get_online_cpus() is smart: it *knows* when you are calling
it in a hotplug-writer, IOW, when you are in a hotplug notifier.
The relevant code is:
void get_online_cpus(void)
{
might_sleep();
if (cpu_hotplug.active_writer == current)
return;
}
So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
notifier should pose no problem at all!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [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(_mutex);
>> /*
>> * the chain is never empty, cache_cache is never destroyed
>> */
>> list_del(>list);
>> if (__cache_shrink(cachep)) {
>> slab_error(cachep, "Can't free all objects");
>> list_add(>list, _caches);
>> mutex_unlock(_mutex);
>> put_online_cpus();
>> return;
>> }
>> mutex_unlock(_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
> 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()
>
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, Oct 03, 2012 at 09:05:31AM +0530, Srivatsa S. Bhat wrote: > On 10/03/2012 03:47 AM, Jiri Kosina wrote: > > On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: > > > >> I don't see how this circular locking dependency can occur.. If you are > >> using SLUB, > >> kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If > >> you are > >> using SLAB, kmem_cache_destroy() wraps its whole operation inside > >> get/put_online_cpus(), > >> which means, it cannot run concurrently with a hotplug operation such as > >> cpu_up(). So, I'm > >> rather puzzled at this lockdep splat.. > > > > I am using SLAB here. > > > > The scenario I think is very well possible: > > > > > > CPU 0 CPU 1 > > kmem_cache_destroy() > > What about the get_online_cpus() right here at CPU0 before > calling mutex_lock(slab_mutex)? How can the cpu_up() proceed > on CPU1?? I still don't get it... :( > > (kmem_cache_destroy() uses get/put_online_cpus() around acquiring > and releasing slab_mutex). The problem is that there is a CPU-hotplug notifier for slab, which establishes hotplug->slab. Then having kmem_cache_destroy() call rcu_barrier() under the lock establishes slab->hotplug, which results in deadlock. Jiri really did explain this in an earlier email message, but both of us managed to miss it. ;-) Thanx, Paul > Regards, > Srivatsa S. Bhat > > > 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) > > > > Deadlock. > > > > Right? > > > > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 Wed, Oct 03, 2012 at 02:45:30AM +0200, 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(_mutex);
> > /*
> > * the chain is never empty, cache_cache is never destroyed
> > */
> > list_del(>list);
> > if (__cache_shrink(cachep)) {
> > slab_error(cachep, "Can't free all objects");
> > list_add(>list, _caches);
> > mutex_unlock(_mutex);
> > put_online_cpus();
> > return;
> > }
> > mutex_unlock(_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).
You can certainly add my Reviewed-by, for whatever that is worth. ;-)
BTW, with this patch, are you able to dispense with my earlier patch,
or is it still needed?
Thanx, Paul
> From: Jiri Kosina
> 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()
>
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/03/2012 03:47 AM, Jiri Kosina wrote: > On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: > >> I don't see how this circular locking dependency can occur.. If you are >> using SLUB, >> kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If >> you are >> using SLAB, kmem_cache_destroy() wraps its whole operation inside >> get/put_online_cpus(), >> which means, it cannot run concurrently with a hotplug operation such as >> cpu_up(). So, I'm >> rather puzzled at this lockdep splat.. > > I am using SLAB here. > > The scenario I think is very well possible: > > > CPU 0 CPU 1 > kmem_cache_destroy() What about the get_online_cpus() right here at CPU0 before calling mutex_lock(slab_mutex)? How can the cpu_up() proceed on CPU1?? I still don't get it... :( (kmem_cache_destroy() uses get/put_online_cpus() around acquiring and releasing slab_mutex). Regards, Srivatsa S. Bhat > 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) > > Deadlock. > > Right? > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[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 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(_mutex);
> /*
>* the chain is never empty, cache_cache is never destroyed
>*/
> list_del(>list);
> if (__cache_shrink(cachep)) {
> slab_error(cachep, "Can't free all objects");
> list_add(>list, _caches);
> mutex_unlock(_mutex);
> put_online_cpus();
> return;
> }
> mutex_unlock(_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
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)
It turns out that slab's kmem_cache_destroy() might release
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
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(_mutex);
/*
* the chain is never empty, cache_cache is never destroyed
*/
list_del(>list);
if (__cache_shrink(cachep)) {
slab_error(cachep, "Can't free all objects");
list_add(>list, _caches);
mutex_unlock(_mutex);
put_online_cpus();
return;
}
mutex_unlock(_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.
Thanx, Paul
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
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? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote: > On Tue, 2 Oct 2012, Jiri Kosina wrote: > > > > > > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit > > > > > commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 > > > > > Author: Paul E. McKenney > > > > > Date: Thu Aug 2 17:43:50 2012 -0700 > > > > > > > > > > rcu: Remove _rcu_barrier() dependency on __stop_machine() > > > > > > > > > > Currently, _rcu_barrier() relies on preempt_disable() to prevent > > > > > any CPU from going offline, which in turn depends on CPU hotplug's > > > > > use of __stop_machine(). > > > > > > > > > > This patch therefore makes _rcu_barrier() use get_online_cpus() to > > > > > block CPU-hotplug operations. This has the added benefit of > > > > > removing > > > > > the need for _rcu_barrier() to adopt callbacks: Because > > > > > CPU-hotplug > > > > > operations are excluded, there can be no callbacks to adopt. This > > > > > commit simplifies the code accordingly. > > > > > > > > > > Signed-off-by: Paul E. McKenney > > > > > Signed-off-by: Paul E. McKenney > > > > > Reviewed-by: Josh Triplett > > > > > == > > > > > > > > > > is causing lockdep to complain (see the full trace below). I haven't > > > > > yet > > > > > had time to analyze what exactly is happening, and probably will not > > > > > have > > > > > time to do so until tomorrow, so just sending this as a heads-up in > > > > > case > > > > > anyone sees the culprit immediately. > > > > > > > > Hmmm... Does the following patch help? It swaps the order in which > > > > rcu_barrier() acquires the hotplug and rcu_barrier locks. > > > > > > It changed the report slightly (see for example the change in possible > > > unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's > > > now directly about cpu_hotplug.lock). With the patch applied I get > > > > > > > > > > > > == > > > [ INFO: possible circular locking dependency detected ] > > > 3.6.0-03888-g3f99f3b #145 Not tainted > > > > And it really seems valid. Yep, it sure is. I wasn't getting the full picture earlier, so please accept my apologies for the bogus patch. > > kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which > > introduces slab_mutex -> cpu_hotplug.lock dependency (through > > rcu_barrier() -> _rcu_barrier() -> get_online_cpus()). > > > > On the other hand, _cpu_up() acquires cpu_hotplug.lock through > > cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier > > gets called, which acquires slab_mutex. This gives the reverse dependency, > > i.e. deadlock scenario is valid one. > > > > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because > > before that, there was no slab_mutex -> cpu_hotplug.lock dependency. > > > > Simply put, the commit causes get_online_cpus() to be called with > > slab_mutex held, which is invalid. > > Oh, and it seems to be actually triggering in real. > > With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c + > your patch, changing the order in which rcu_barrier() acquires hotplug and > rcu_barrier locks, the machine hangs 100% reliably during suspend, which > very likely actually is the deadlock described above. 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... Thanx, Paul -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: > I don't see how this circular locking dependency can occur.. If you are using > SLUB, > kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If > you are > using SLAB, kmem_cache_destroy() wraps its whole operation inside > get/put_online_cpus(), > which means, it cannot run concurrently with a hotplug operation such as > cpu_up(). So, I'm > rather puzzled at this lockdep splat.. I am using SLAB here. The scenario I think is very well possible: 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) Deadlock. Right? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Tue, 2 Oct 2012, Jiri Kosina wrote: > > > > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit > > > > commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 > > > > Author: Paul E. McKenney > > > > Date: Thu Aug 2 17:43:50 2012 -0700 > > > > > > > > rcu: Remove _rcu_barrier() dependency on __stop_machine() > > > > > > > > Currently, _rcu_barrier() relies on preempt_disable() to prevent > > > > any CPU from going offline, which in turn depends on CPU hotplug's > > > > use of __stop_machine(). > > > > > > > > This patch therefore makes _rcu_barrier() use get_online_cpus() to > > > > block CPU-hotplug operations. This has the added benefit of > > > > removing > > > > the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug > > > > operations are excluded, there can be no callbacks to adopt. This > > > > commit simplifies the code accordingly. > > > > > > > > Signed-off-by: Paul E. McKenney > > > > Signed-off-by: Paul E. McKenney > > > > Reviewed-by: Josh Triplett > > > > == > > > > > > > > is causing lockdep to complain (see the full trace below). I haven't > > > > yet > > > > had time to analyze what exactly is happening, and probably will not > > > > have > > > > time to do so until tomorrow, so just sending this as a heads-up in > > > > case > > > > anyone sees the culprit immediately. > > > > > > Hmmm... Does the following patch help? It swaps the order in which > > > rcu_barrier() acquires the hotplug and rcu_barrier locks. > > > > It changed the report slightly (see for example the change in possible > > unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's > > now directly about cpu_hotplug.lock). With the patch applied I get > > > > > > > > == > > [ INFO: possible circular locking dependency detected ] > > 3.6.0-03888-g3f99f3b #145 Not tainted > > And it really seems valid. > > kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which > introduces slab_mutex -> cpu_hotplug.lock dependency (through > rcu_barrier() -> _rcu_barrier() -> get_online_cpus()). > > On the other hand, _cpu_up() acquires cpu_hotplug.lock through > cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier > gets called, which acquires slab_mutex. This gives the reverse dependency, > i.e. deadlock scenario is valid one. > > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because > before that, there was no slab_mutex -> cpu_hotplug.lock dependency. > > Simply put, the commit causes get_online_cpus() to be called with > slab_mutex held, which is invalid. Oh, and it seems to be actually triggering in real. With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c + your patch, changing the order in which rcu_barrier() acquires hotplug and rcu_barrier locks, the machine hangs 100% reliably during suspend, which very likely actually is the deadlock described above. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Tue, 2 Oct 2012, Jiri Kosina wrote: > > > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit > > > commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 > > > Author: Paul E. McKenney > > > Date: Thu Aug 2 17:43:50 2012 -0700 > > > > > > rcu: Remove _rcu_barrier() dependency on __stop_machine() > > > > > > Currently, _rcu_barrier() relies on preempt_disable() to prevent > > > any CPU from going offline, which in turn depends on CPU hotplug's > > > use of __stop_machine(). > > > > > > This patch therefore makes _rcu_barrier() use get_online_cpus() to > > > block CPU-hotplug operations. This has the added benefit of removing > > > the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug > > > operations are excluded, there can be no callbacks to adopt. This > > > commit simplifies the code accordingly. > > > > > > Signed-off-by: Paul E. McKenney > > > Signed-off-by: Paul E. McKenney > > > Reviewed-by: Josh Triplett > > > == > > > > > > is causing lockdep to complain (see the full trace below). I haven't yet > > > had time to analyze what exactly is happening, and probably will not have > > > time to do so until tomorrow, so just sending this as a heads-up in case > > > anyone sees the culprit immediately. > > > > Hmmm... Does the following patch help? It swaps the order in which > > rcu_barrier() acquires the hotplug and rcu_barrier locks. > > It changed the report slightly (see for example the change in possible > unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's > now directly about cpu_hotplug.lock). With the patch applied I get > > > > == > [ INFO: possible circular locking dependency detected ] > 3.6.0-03888-g3f99f3b #145 Not tainted And it really seems valid. kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which introduces slab_mutex -> cpu_hotplug.lock dependency (through rcu_barrier() -> _rcu_barrier() -> get_online_cpus()). On the other hand, _cpu_up() acquires cpu_hotplug.lock through cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier gets called, which acquires slab_mutex. This gives the reverse dependency, i.e. deadlock scenario is valid one. 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because before that, there was no slab_mutex -> cpu_hotplug.lock dependency. Simply put, the commit causes get_online_cpus() to be called with slab_mutex held, which is invalid. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Tue, 2 Oct 2012, Paul E. McKenney wrote:
> > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
> > commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
> > Author: Paul E. McKenney
> > Date: Thu Aug 2 17:43:50 2012 -0700
> >
> > rcu: Remove _rcu_barrier() dependency on __stop_machine()
> >
> > Currently, _rcu_barrier() relies on preempt_disable() to prevent
> > any CPU from going offline, which in turn depends on CPU hotplug's
> > use of __stop_machine().
> >
> > This patch therefore makes _rcu_barrier() use get_online_cpus() to
> > block CPU-hotplug operations. This has the added benefit of removing
> > the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
> > operations are excluded, there can be no callbacks to adopt. This
> > commit simplifies the code accordingly.
> >
> > Signed-off-by: Paul E. McKenney
> > Signed-off-by: Paul E. McKenney
> > Reviewed-by: Josh Triplett
> > ==
> >
> > is causing lockdep to complain (see the full trace below). I haven't yet
> > had time to analyze what exactly is happening, and probably will not have
> > time to do so until tomorrow, so just sending this as a heads-up in case
> > anyone sees the culprit immediately.
>
> Hmmm... Does the following patch help? It swaps the order in which
> rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
---
kworker/u:3/43 is trying to acquire lock:
(cpu_hotplug.lock){+.+.+.}, at: [] get_online_cpus+0x37/0x50
but task is already holding lock:
(slab_mutex){+.+.+.}, at: [] kmem_cache_destroy+0x45/0xe0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #1 (slab_mutex){+.+.+.}:
[] validate_chain+0x632/0x720
[] __lock_acquire+0x359/0x580
[] lock_acquire+0x121/0x190
[] __mutex_lock_common+0x5c/0x450
[] mutex_lock_nested+0x3e/0x50
[] cpuup_callback+0x2f/0xbe
[] notifier_call_chain+0x93/0x140
[] __raw_notifier_call_chain+0x9/0x10
[] _cpu_up+0xc9/0x162
[] cpu_up+0xbc/0x11b
[] smp_init+0x6b/0x9f
[] kernel_init+0x147/0x1dc
[] kernel_thread_helper+0x4/0x10
-> #0 (cpu_hotplug.lock){+.+.+.}:
[] check_prev_add+0x3de/0x440
[] validate_chain+0x632/0x720
[] __lock_acquire+0x359/0x580
[] lock_acquire+0x121/0x190
[] __mutex_lock_common+0x5c/0x450
[] mutex_lock_nested+0x3e/0x50
[] get_online_cpus+0x37/0x50
[] _rcu_barrier+0x22/0x1f0
[] rcu_barrier_sched+0x10/0x20
[] rcu_barrier+0x9/0x10
[] kmem_cache_destroy+0xd1/0xe0
[] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack]
[] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[] ops_exit_list+0x39/0x60
[] cleanup_net+0xfb/0x1b0
[] process_one_work+0x26b/0x4c0
[] worker_thread+0x12e/0x320
[] kthread+0xde/0xf0
[] kernel_thread_helper+0x4/0x10
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(slab_mutex);
lock(cpu_hotplug.lock);
lock(slab_mutex);
lock(cpu_hotplug.lock);
*** DEADLOCK ***
4 locks held by kworker/u:3/43:
#0: (netns){.+.+.+}, at: [] process_one_work+0x1a2/0x4c0
#1: (net_cleanup_work){+.+.+.}, at: []
process_one_work+0x1a2/0x4c0
#2: (net_mutex){+.+.+.}, at: [] cleanup_net+0x80/0x1b0
#3: (slab_mutex){+.+.+.}, at: []
kmem_cache_destroy+0x45/0xe0
stack backtrace:
Pid: 43, comm: kworker/u:3 Not tainted 3.6.0-03888-g3f99f3b #145
Call Trace:
[] print_circular_bug+0x10f/0x120
[] check_prev_add+0x3de/0x440
[] validate_chain+0x632/0x720
[] __lock_acquire+0x359/0x580
[] lock_acquire+0x121/0x190
[] ? get_online_cpus+0x37/0x50
[] __mutex_lock_common+0x5c/0x450
[] ? get_online_cpus+0x37/0x50
[] ? mark_held_locks+0x80/0x120
[] ? get_online_cpus+0x37/0x50
[] mutex_lock_nested+0x3e/0x50
[] get_online_cpus+0x37/0x50
[] _rcu_barrier+0x22/0x1f0
[] rcu_barrier_sched+0x10/0x20
[] rcu_barrier+0x9/0x10
[] kmem_cache_destroy+0xd1/0xe0
[] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack]
[] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[] ops_exit_list+0x39/0x60
[] cleanup_net+0xfb/0x1b0
[] process_one_work+0x26b/0x4c0
[] ? process_one_work+0x1a2/0x4c0
[] ? worker_thread+0x59/0x320
[] ? net_drop_ns+0x40/0x40
[]
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On 10/02/2012 09:44 PM, Jiri Kosina wrote:
> Hi,
>
> this commit:
>
> ==
> 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
> commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
> Author: Paul E. McKenney
> Date: Thu Aug 2 17:43:50 2012 -0700
>
> rcu: Remove _rcu_barrier() dependency on __stop_machine()
>
> Currently, _rcu_barrier() relies on preempt_disable() to prevent
> any CPU from going offline, which in turn depends on CPU hotplug's
> use of __stop_machine().
>
> This patch therefore makes _rcu_barrier() use get_online_cpus() to
> block CPU-hotplug operations. This has the added benefit of removing
> the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
> operations are excluded, there can be no callbacks to adopt. This
> commit simplifies the code accordingly.
>
> Signed-off-by: Paul E. McKenney
> Signed-off-by: Paul E. McKenney
> Reviewed-by: Josh Triplett
> ==
>
> is causing lockdep to complain (see the full trace below). I haven't yet
> had time to analyze what exactly is happening, and probably will not have
> time to do so until tomorrow, so just sending this as a heads-up in case
> anyone sees the culprit immediately.
>
> ==
> [ INFO: possible circular locking dependency detected ]
> 3.6.0-rc5-4-g0d8ee37 #143 Not tainted
> ---
> kworker/u:2/40 is trying to acquire lock:
> (rcu_sched_state.barrier_mutex){+.+...}, at: []
> _rcu_barrier+0x26/0x1e0
>
> but task is already holding lock:
> (slab_mutex){+.+.+.}, at: [] kmem_cache_destroy+0x45/0xe0
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (slab_mutex){+.+.+.}:
> [] validate_chain+0x632/0x720
> [] __lock_acquire+0x309/0x530
> [] lock_acquire+0x121/0x190
> [] __mutex_lock_common+0x5c/0x450
> [] mutex_lock_nested+0x3e/0x50
> [] cpuup_callback+0x2f/0xbe
> [] notifier_call_chain+0x93/0x140
> [] __raw_notifier_call_chain+0x9/0x10
> [] _cpu_up+0xba/0x14e
> [] cpu_up+0xbc/0x117
> [] smp_init+0x6b/0x9f
> [] kernel_init+0x147/0x1dc
> [] kernel_thread_helper+0x4/0x10
>
> -> #1 (cpu_hotplug.lock){+.+.+.}:
> [] validate_chain+0x632/0x720
> [] __lock_acquire+0x309/0x530
> [] lock_acquire+0x121/0x190
> [] __mutex_lock_common+0x5c/0x450
> [] mutex_lock_nested+0x3e/0x50
> [] get_online_cpus+0x37/0x50
> [] _rcu_barrier+0xbb/0x1e0
> [] rcu_barrier_sched+0x10/0x20
> [] rcu_barrier+0x9/0x10
> [] deactivate_locked_super+0x49/0x90
> [] deactivate_super+0x61/0x70
> [] mntput_no_expire+0x127/0x180
> [] sys_umount+0x6e/0xd0
> [] system_call_fastpath+0x16/0x1b
>
> -> #0 (rcu_sched_state.barrier_mutex){+.+...}:
> [] check_prev_add+0x3de/0x440
> [] validate_chain+0x632/0x720
> [] __lock_acquire+0x309/0x530
> [] lock_acquire+0x121/0x190
> [] __mutex_lock_common+0x5c/0x450
> [] mutex_lock_nested+0x3e/0x50
> [] _rcu_barrier+0x26/0x1e0
> [] rcu_barrier_sched+0x10/0x20
> [] rcu_barrier+0x9/0x10
> [] kmem_cache_destroy+0xd1/0xe0
> [] nf_conntrack_cleanup_net+0xe4/0x110
> [nf_conntrack]
> [] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
> [] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
> [] ops_exit_list+0x39/0x60
> [] cleanup_net+0xfb/0x1b0
> [] process_one_work+0x26b/0x4c0
> [] worker_thread+0x12e/0x320
> [] kthread+0x9e/0xb0
> [] kernel_thread_helper+0x4/0x10
>
> other info that might help us debug this:
>
> Chain exists of:
>rcu_sched_state.barrier_mutex --> cpu_hotplug.lock --> slab_mutex
>
> Possible unsafe locking scenario:
>
> CPU0CPU1
>
>lock(slab_mutex);
> lock(cpu_hotplug.lock);
> lock(slab_mutex);
>lock(rcu_sched_state.barrier_mutex);
>
> *** DEADLOCK ***
>
> 4 locks held by kworker/u:2/40:
> #0: (netns){.+.+.+}, at: [] process_one_work+0x1a2/0x4c0
> #1: (net_cleanup_work){+.+.+.}, at: []
> process_one_work+0x1a2/0x4c0
> #2: (net_mutex){+.+.+.}, at: [] cleanup_net+0x80/0x1b0
> #3: (slab_mutex){+.+.+.}, at: []
> kmem_cache_destroy+0x45/0xe0
>
I don't see how this circular locking dependency can occur.. If you are using
SLUB,
kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you
are
using SLAB, kmem_cache_destroy() wraps its whole operation inside
get/put_online_cpus(),
which means, it cannot run concurrently with a hotplug operation such as
cpu_up(). So, I'm
rather puzzled at this lockdep splat..
Regards,
Re: Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
On Tue, Oct 02, 2012 at 06:14:08PM +0200, Jiri Kosina wrote: > Hi, > > this commit: > > == > 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit > commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 > Author: Paul E. McKenney > Date: Thu Aug 2 17:43:50 2012 -0700 > > rcu: Remove _rcu_barrier() dependency on __stop_machine() > > Currently, _rcu_barrier() relies on preempt_disable() to prevent > any CPU from going offline, which in turn depends on CPU hotplug's > use of __stop_machine(). > > This patch therefore makes _rcu_barrier() use get_online_cpus() to > block CPU-hotplug operations. This has the added benefit of removing > the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug > operations are excluded, there can be no callbacks to adopt. This > commit simplifies the code accordingly. > > Signed-off-by: Paul E. McKenney > Signed-off-by: Paul E. McKenney > Reviewed-by: Josh Triplett > == > > is causing lockdep to complain (see the full trace below). I haven't yet > had time to analyze what exactly is happening, and probably will not have > time to do so until tomorrow, so just sending this as a heads-up in case > anyone sees the culprit immediately. Hmmm... Does the following patch help? It swaps the order in which rcu_barrier() acquires the hotplug and rcu_barrier locks. Thanx, Paul diff --git a/kernel/rcutree.c b/kernel/rcutree.c index 2ad9e81..2c71d61 100644 --- a/kernel/rcutree.c +++ b/kernel/rcutree.c @@ -2560,6 +2560,9 @@ static void _rcu_barrier(struct rcu_state *rsp) _rcu_barrier_trace(rsp, "Begin", -1, snap); + /* Exclude CPU-hotplug, ensuring that no offline CPU has callbacks. */ + get_online_cpus(); + /* Take mutex to serialize concurrent rcu_barrier() requests. */ mutex_lock(>barrier_mutex); @@ -2581,6 +2584,7 @@ static void _rcu_barrier(struct rcu_state *rsp) _rcu_barrier_trace(rsp, "EarlyExit", -1, snap_done); smp_mb(); /* caller's subsequent code after above check. */ mutex_unlock(>barrier_mutex); + put_online_cpus(); return; } @@ -2597,8 +2601,7 @@ static void _rcu_barrier(struct rcu_state *rsp) /* * Initialize the count to one rather than to zero in order to * avoid a too-soon return to zero in case of a short grace period -* (or preemption of this task). Exclude CPU-hotplug operations -* to ensure that no offline CPU has callbacks queued. +* (or preemption of this task). */ init_completion(>barrier_completion); atomic_set(>barrier_cpu_count, 1); @@ -2620,7 +2623,6 @@ static void _rcu_barrier(struct rcu_state *rsp) rsp->n_barrier_done); } } - put_online_cpus(); /* * Now that we have an rcu_barrier_callback() callback on each @@ -2639,8 +2641,9 @@ static void _rcu_barrier(struct rcu_state *rsp) /* Wait for all rcu_barrier_callback() callbacks to be invoked. */ wait_for_completion(>barrier_completion); - /* Other rcu_barrier() invocations can now safely proceed. */ + /* Hotplug and rcu_barrier() invocations can now safely proceed. */ mutex_unlock(>barrier_mutex); + put_online_cpus(); } /** -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Lockdep complains about commit 1331e7a1bb ("rcu: Remove _rcu_barrier() dependency on __stop_machine()")
Hi,
this commit:
==
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of removing
the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney
Signed-off-by: Paul E. McKenney
Reviewed-by: Josh Triplett
==
is causing lockdep to complain (see the full trace below). I haven't yet
had time to analyze what exactly is happening, and probably will not have
time to do so until tomorrow, so just sending this as a heads-up in case
anyone sees the culprit immediately.
==
[ INFO: possible circular locking dependency detected ]
3.6.0-rc5-4-g0d8ee37 #143 Not tainted
---
kworker/u:2/40 is trying to acquire lock:
(rcu_sched_state.barrier_mutex){+.+...}, at: []
_rcu_barrier+0x26/0x1e0
but task is already holding lock:
(slab_mutex){+.+.+.}, at: [] kmem_cache_destroy+0x45/0xe0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
-> #2 (slab_mutex){+.+.+.}:
[] validate_chain+0x632/0x720
[] __lock_acquire+0x309/0x530
[] lock_acquire+0x121/0x190
[] __mutex_lock_common+0x5c/0x450
[] mutex_lock_nested+0x3e/0x50
[] cpuup_callback+0x2f/0xbe
[] notifier_call_chain+0x93/0x140
[] __raw_notifier_call_chain+0x9/0x10
[] _cpu_up+0xba/0x14e
[] cpu_up+0xbc/0x117
[] smp_init+0x6b/0x9f
[] kernel_init+0x147/0x1dc
[] kernel_thread_helper+0x4/0x10
-> #1 (cpu_hotplug.lock){+.+.+.}:
[] validate_chain+0x632/0x720
[] __lock_acquire+0x309/0x530
[] lock_acquire+0x121/0x190
[] __mutex_lock_common+0x5c/0x450
[] mutex_lock_nested+0x3e/0x50
[] get_online_cpus+0x37/0x50
[] _rcu_barrier+0xbb/0x1e0
[] rcu_barrier_sched+0x10/0x20
[] rcu_barrier+0x9/0x10
[] deactivate_locked_super+0x49/0x90
[] deactivate_super+0x61/0x70
[] mntput_no_expire+0x127/0x180
[] sys_umount+0x6e/0xd0
[] system_call_fastpath+0x16/0x1b
-> #0 (rcu_sched_state.barrier_mutex){+.+...}:
[] check_prev_add+0x3de/0x440
[] validate_chain+0x632/0x720
[] __lock_acquire+0x309/0x530
[] lock_acquire+0x121/0x190
[] __mutex_lock_common+0x5c/0x450
[] mutex_lock_nested+0x3e/0x50
[] _rcu_barrier+0x26/0x1e0
[] rcu_barrier_sched+0x10/0x20
[] rcu_barrier+0x9/0x10
[] kmem_cache_destroy+0xd1/0xe0
[] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack]
[] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[] ops_exit_list+0x39/0x60
[] cleanup_net+0xfb/0x1b0
[] process_one_work+0x26b/0x4c0
[] worker_thread+0x12e/0x320
[] kthread+0x9e/0xb0
[] kernel_thread_helper+0x4/0x10
other info that might help us debug this:
Chain exists of:
rcu_sched_state.barrier_mutex --> cpu_hotplug.lock --> slab_mutex
Possible unsafe locking scenario:
CPU0CPU1
lock(slab_mutex);
lock(cpu_hotplug.lock);
lock(slab_mutex);
lock(rcu_sched_state.barrier_mutex);
*** DEADLOCK ***
4 locks held by kworker/u:2/40:
#0: (netns){.+.+.+}, at: [] process_one_work+0x1a2/0x4c0
#1: (net_cleanup_work){+.+.+.}, at: []
process_one_work+0x1a2/0x4c0
#2: (net_mutex){+.+.+.}, at: [] cleanup_net+0x80/0x1b0
#3: (slab_mutex){+.+.+.}, at: []
kmem_cache_destroy+0x45/0xe0
stack backtrace:
Pid: 40, comm: kworker/u:2 Not tainted 3.6.0-rc5-4-g0d8ee37 #143
Call Trace:
[] print_circular_bug+0x10f/0x120
[] check_prev_add+0x3de/0x440
[] ? check_prev_add+0xea/0x440
[] ? flat_send_IPI_mask+0x7f/0xc0
[] validate_chain+0x632/0x720
[] __lock_acquire+0x309/0x530
[] lock_acquire+0x121/0x190
[] ? _rcu_barrier+0x26/0x1e0
[] __mutex_lock_common+0x5c/0x450
[] ? _rcu_barrier+0x26/0x1e0
[] ? on_each_cpu+0x65/0xc0
[] ? _rcu_barrier+0x26/0x1e0
[] mutex_lock_nested+0x3e/0x50
[] _rcu_barrier+0x26/0x1e0
[] rcu_barrier_sched+0x10/0x20
[] rcu_barrier+0x9/0x10
[] kmem_cache_destroy+0xd1/0xe0
[]
Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
Hi,
this commit:
==
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of removing
the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't yet
had time to analyze what exactly is happening, and probably will not have
time to do so until tomorrow, so just sending this as a heads-up in case
anyone sees the culprit immediately.
==
[ INFO: possible circular locking dependency detected ]
3.6.0-rc5-4-g0d8ee37 #143 Not tainted
---
kworker/u:2/40 is trying to acquire lock:
(rcu_sched_state.barrier_mutex){+.+...}, at: [810f2126]
_rcu_barrier+0x26/0x1e0
but task is already holding lock:
(slab_mutex){+.+.+.}, at: [81176e15] kmem_cache_destroy+0x45/0xe0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
- #2 (slab_mutex){+.+.+.}:
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[81558cb5] cpuup_callback+0x2f/0xbe
[81564b83] notifier_call_chain+0x93/0x140
[81076f89] __raw_notifier_call_chain+0x9/0x10
[8155719d] _cpu_up+0xba/0x14e
[815572ed] cpu_up+0xbc/0x117
[81ae05e3] smp_init+0x6b/0x9f
[81ac47d6] kernel_init+0x147/0x1dc
[8156ab44] kernel_thread_helper+0x4/0x10
- #1 (cpu_hotplug.lock){+.+.+.}:
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[81049197] get_online_cpus+0x37/0x50
[810f21bb] _rcu_barrier+0xbb/0x1e0
[810f22f0] rcu_barrier_sched+0x10/0x20
[810f2309] rcu_barrier+0x9/0x10
[8118c129] deactivate_locked_super+0x49/0x90
[8118cc01] deactivate_super+0x61/0x70
[8117] mntput_no_expire+0x127/0x180
[811ab49e] sys_umount+0x6e/0xd0
[81569979] system_call_fastpath+0x16/0x1b
- #0 (rcu_sched_state.barrier_mutex){+.+...}:
[810adb4e] check_prev_add+0x3de/0x440
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[810f2126] _rcu_barrier+0x26/0x1e0
[810f22f0] rcu_barrier_sched+0x10/0x20
[810f2309] rcu_barrier+0x9/0x10
[81176ea1] kmem_cache_destroy+0xd1/0xe0
[a04c3154] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack]
[a04c31aa] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[a04c42ce] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[81454b79] ops_exit_list+0x39/0x60
[814551ab] cleanup_net+0xfb/0x1b0
[8106917b] process_one_work+0x26b/0x4c0
[81069f3e] worker_thread+0x12e/0x320
[8106f73e] kthread+0x9e/0xb0
[8156ab44] kernel_thread_helper+0x4/0x10
other info that might help us debug this:
Chain exists of:
rcu_sched_state.barrier_mutex -- cpu_hotplug.lock -- slab_mutex
Possible unsafe locking scenario:
CPU0CPU1
lock(slab_mutex);
lock(cpu_hotplug.lock);
lock(slab_mutex);
lock(rcu_sched_state.barrier_mutex);
*** DEADLOCK ***
4 locks held by kworker/u:2/40:
#0: (netns){.+.+.+}, at: [810690b2] process_one_work+0x1a2/0x4c0
#1:
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Tue, Oct 02, 2012 at 06:14:08PM +0200, Jiri Kosina wrote: Hi, this commit: == 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 Author: Paul E. McKenney paul.mcken...@linaro.org Date: Thu Aug 2 17:43:50 2012 -0700 rcu: Remove _rcu_barrier() dependency on __stop_machine() Currently, _rcu_barrier() relies on preempt_disable() to prevent any CPU from going offline, which in turn depends on CPU hotplug's use of __stop_machine(). This patch therefore makes _rcu_barrier() use get_online_cpus() to block CPU-hotplug operations. This has the added benefit of removing the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug operations are excluded, there can be no callbacks to adopt. This commit simplifies the code accordingly. Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com Reviewed-by: Josh Triplett j...@joshtriplett.org == is causing lockdep to complain (see the full trace below). I haven't yet had time to analyze what exactly is happening, and probably will not have time to do so until tomorrow, so just sending this as a heads-up in case anyone sees the culprit immediately. Hmmm... Does the following patch help? It swaps the order in which rcu_barrier() acquires the hotplug and rcu_barrier locks. Thanx, Paul diff --git a/kernel/rcutree.c b/kernel/rcutree.c index 2ad9e81..2c71d61 100644 --- a/kernel/rcutree.c +++ b/kernel/rcutree.c @@ -2560,6 +2560,9 @@ static void _rcu_barrier(struct rcu_state *rsp) _rcu_barrier_trace(rsp, Begin, -1, snap); + /* Exclude CPU-hotplug, ensuring that no offline CPU has callbacks. */ + get_online_cpus(); + /* Take mutex to serialize concurrent rcu_barrier() requests. */ mutex_lock(rsp-barrier_mutex); @@ -2581,6 +2584,7 @@ static void _rcu_barrier(struct rcu_state *rsp) _rcu_barrier_trace(rsp, EarlyExit, -1, snap_done); smp_mb(); /* caller's subsequent code after above check. */ mutex_unlock(rsp-barrier_mutex); + put_online_cpus(); return; } @@ -2597,8 +2601,7 @@ static void _rcu_barrier(struct rcu_state *rsp) /* * Initialize the count to one rather than to zero in order to * avoid a too-soon return to zero in case of a short grace period -* (or preemption of this task). Exclude CPU-hotplug operations -* to ensure that no offline CPU has callbacks queued. +* (or preemption of this task). */ init_completion(rsp-barrier_completion); atomic_set(rsp-barrier_cpu_count, 1); @@ -2620,7 +2623,6 @@ static void _rcu_barrier(struct rcu_state *rsp) rsp-n_barrier_done); } } - put_online_cpus(); /* * Now that we have an rcu_barrier_callback() callback on each @@ -2639,8 +2641,9 @@ static void _rcu_barrier(struct rcu_state *rsp) /* Wait for all rcu_barrier_callback() callbacks to be invoked. */ wait_for_completion(rsp-barrier_completion); - /* Other rcu_barrier() invocations can now safely proceed. */ + /* Hotplug and rcu_barrier() invocations can now safely proceed. */ mutex_unlock(rsp-barrier_mutex); + put_online_cpus(); } /** -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/02/2012 09:44 PM, Jiri Kosina wrote:
Hi,
this commit:
==
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of removing
the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't yet
had time to analyze what exactly is happening, and probably will not have
time to do so until tomorrow, so just sending this as a heads-up in case
anyone sees the culprit immediately.
==
[ INFO: possible circular locking dependency detected ]
3.6.0-rc5-4-g0d8ee37 #143 Not tainted
---
kworker/u:2/40 is trying to acquire lock:
(rcu_sched_state.barrier_mutex){+.+...}, at: [810f2126]
_rcu_barrier+0x26/0x1e0
but task is already holding lock:
(slab_mutex){+.+.+.}, at: [81176e15] kmem_cache_destroy+0x45/0xe0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
- #2 (slab_mutex){+.+.+.}:
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[81558cb5] cpuup_callback+0x2f/0xbe
[81564b83] notifier_call_chain+0x93/0x140
[81076f89] __raw_notifier_call_chain+0x9/0x10
[8155719d] _cpu_up+0xba/0x14e
[815572ed] cpu_up+0xbc/0x117
[81ae05e3] smp_init+0x6b/0x9f
[81ac47d6] kernel_init+0x147/0x1dc
[8156ab44] kernel_thread_helper+0x4/0x10
- #1 (cpu_hotplug.lock){+.+.+.}:
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[81049197] get_online_cpus+0x37/0x50
[810f21bb] _rcu_barrier+0xbb/0x1e0
[810f22f0] rcu_barrier_sched+0x10/0x20
[810f2309] rcu_barrier+0x9/0x10
[8118c129] deactivate_locked_super+0x49/0x90
[8118cc01] deactivate_super+0x61/0x70
[8117] mntput_no_expire+0x127/0x180
[811ab49e] sys_umount+0x6e/0xd0
[81569979] system_call_fastpath+0x16/0x1b
- #0 (rcu_sched_state.barrier_mutex){+.+...}:
[810adb4e] check_prev_add+0x3de/0x440
[810ae1e2] validate_chain+0x632/0x720
[810ae5d9] __lock_acquire+0x309/0x530
[810ae921] lock_acquire+0x121/0x190
[8155d4cc] __mutex_lock_common+0x5c/0x450
[8155d9ee] mutex_lock_nested+0x3e/0x50
[810f2126] _rcu_barrier+0x26/0x1e0
[810f22f0] rcu_barrier_sched+0x10/0x20
[810f2309] rcu_barrier+0x9/0x10
[81176ea1] kmem_cache_destroy+0xd1/0xe0
[a04c3154] nf_conntrack_cleanup_net+0xe4/0x110
[nf_conntrack]
[a04c31aa] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[a04c42ce] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[81454b79] ops_exit_list+0x39/0x60
[814551ab] cleanup_net+0xfb/0x1b0
[8106917b] process_one_work+0x26b/0x4c0
[81069f3e] worker_thread+0x12e/0x320
[8106f73e] kthread+0x9e/0xb0
[8156ab44] kernel_thread_helper+0x4/0x10
other info that might help us debug this:
Chain exists of:
rcu_sched_state.barrier_mutex -- cpu_hotplug.lock -- slab_mutex
Possible unsafe locking scenario:
CPU0CPU1
lock(slab_mutex);
lock(cpu_hotplug.lock);
lock(slab_mutex);
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Tue, 2 Oct 2012, Paul E. McKenney wrote:
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of removing
the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't yet
had time to analyze what exactly is happening, and probably will not have
time to do so until tomorrow, so just sending this as a heads-up in case
anyone sees the culprit immediately.
Hmmm... Does the following patch help? It swaps the order in which
rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
---
kworker/u:3/43 is trying to acquire lock:
(cpu_hotplug.lock){+.+.+.}, at: [81049287] get_online_cpus+0x37/0x50
but task is already holding lock:
(slab_mutex){+.+.+.}, at: [81178175] kmem_cache_destroy+0x45/0xe0
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
- #1 (slab_mutex){+.+.+.}:
[810aeb22] validate_chain+0x632/0x720
[810aef69] __lock_acquire+0x359/0x580
[810af2b1] lock_acquire+0x121/0x190
[8156130c] __mutex_lock_common+0x5c/0x450
[8156182e] mutex_lock_nested+0x3e/0x50
[8155cafa] cpuup_callback+0x2f/0xbe
[81568bc3] notifier_call_chain+0x93/0x140
[81077289] __raw_notifier_call_chain+0x9/0x10
[8155b1ac] _cpu_up+0xc9/0x162
[8155b301] cpu_up+0xbc/0x11b
[81ae1793] smp_init+0x6b/0x9f
[81ac57d6] kernel_init+0x147/0x1dc
[8156eca4] kernel_thread_helper+0x4/0x10
- #0 (cpu_hotplug.lock){+.+.+.}:
[810ae48e] check_prev_add+0x3de/0x440
[810aeb22] validate_chain+0x632/0x720
[810aef69] __lock_acquire+0x359/0x580
[810af2b1] lock_acquire+0x121/0x190
[8156130c] __mutex_lock_common+0x5c/0x450
[8156182e] mutex_lock_nested+0x3e/0x50
[81049287] get_online_cpus+0x37/0x50
[810f3a92] _rcu_barrier+0x22/0x1f0
[810f3c70] rcu_barrier_sched+0x10/0x20
[810f3c89] rcu_barrier+0x9/0x10
[81178201] kmem_cache_destroy+0xd1/0xe0
[a0488154] nf_conntrack_cleanup_net+0xe4/0x110 [nf_conntrack]
[a04881aa] nf_conntrack_cleanup+0x2a/0x70 [nf_conntrack]
[a04892ce] nf_conntrack_net_exit+0x5e/0x80 [nf_conntrack]
[81458629] ops_exit_list+0x39/0x60
[81458c5b] cleanup_net+0xfb/0x1b0
[810691eb] process_one_work+0x26b/0x4c0
[8106a03e] worker_thread+0x12e/0x320
[8106f86e] kthread+0xde/0xf0
[8156eca4] kernel_thread_helper+0x4/0x10
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0CPU1
lock(slab_mutex);
lock(cpu_hotplug.lock);
lock(slab_mutex);
lock(cpu_hotplug.lock);
*** DEADLOCK ***
4 locks held by kworker/u:3/43:
#0: (netns){.+.+.+}, at: [81069122] process_one_work+0x1a2/0x4c0
#1: (net_cleanup_work){+.+.+.}, at: [81069122]
process_one_work+0x1a2/0x4c0
#2: (net_mutex){+.+.+.}, at: [81458be0] cleanup_net+0x80/0x1b0
#3: (slab_mutex){+.+.+.}, at: [81178175]
kmem_cache_destroy+0x45/0xe0
stack backtrace:
Pid: 43, comm: kworker/u:3 Not tainted 3.6.0-03888-g3f99f3b #145
Call Trace:
[810ac5cf] print_circular_bug+0x10f/0x120
[810ae48e] check_prev_add+0x3de/0x440
[810aeb22] validate_chain+0x632/0x720
[810aef69]
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Tue, 2 Oct 2012, Jiri Kosina wrote: 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 Author: Paul E. McKenney paul.mcken...@linaro.org Date: Thu Aug 2 17:43:50 2012 -0700 rcu: Remove _rcu_barrier() dependency on __stop_machine() Currently, _rcu_barrier() relies on preempt_disable() to prevent any CPU from going offline, which in turn depends on CPU hotplug's use of __stop_machine(). This patch therefore makes _rcu_barrier() use get_online_cpus() to block CPU-hotplug operations. This has the added benefit of removing the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug operations are excluded, there can be no callbacks to adopt. This commit simplifies the code accordingly. Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com Reviewed-by: Josh Triplett j...@joshtriplett.org == is causing lockdep to complain (see the full trace below). I haven't yet had time to analyze what exactly is happening, and probably will not have time to do so until tomorrow, so just sending this as a heads-up in case anyone sees the culprit immediately. Hmmm... Does the following patch help? It swaps the order in which rcu_barrier() acquires the hotplug and rcu_barrier locks. It changed the report slightly (see for example the change in possible unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's now directly about cpu_hotplug.lock). With the patch applied I get == [ INFO: possible circular locking dependency detected ] 3.6.0-03888-g3f99f3b #145 Not tainted And it really seems valid. kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which introduces slab_mutex - cpu_hotplug.lock dependency (through rcu_barrier() - _rcu_barrier() - get_online_cpus()). On the other hand, _cpu_up() acquires cpu_hotplug.lock through cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier gets called, which acquires slab_mutex. This gives the reverse dependency, i.e. deadlock scenario is valid one. 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because before that, there was no slab_mutex - cpu_hotplug.lock dependency. Simply put, the commit causes get_online_cpus() to be called with slab_mutex held, which is invalid. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Tue, 2 Oct 2012, Jiri Kosina wrote: 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 Author: Paul E. McKenney paul.mcken...@linaro.org Date: Thu Aug 2 17:43:50 2012 -0700 rcu: Remove _rcu_barrier() dependency on __stop_machine() Currently, _rcu_barrier() relies on preempt_disable() to prevent any CPU from going offline, which in turn depends on CPU hotplug's use of __stop_machine(). This patch therefore makes _rcu_barrier() use get_online_cpus() to block CPU-hotplug operations. This has the added benefit of removing the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug operations are excluded, there can be no callbacks to adopt. This commit simplifies the code accordingly. Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com Reviewed-by: Josh Triplett j...@joshtriplett.org == is causing lockdep to complain (see the full trace below). I haven't yet had time to analyze what exactly is happening, and probably will not have time to do so until tomorrow, so just sending this as a heads-up in case anyone sees the culprit immediately. Hmmm... Does the following patch help? It swaps the order in which rcu_barrier() acquires the hotplug and rcu_barrier locks. It changed the report slightly (see for example the change in possible unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's now directly about cpu_hotplug.lock). With the patch applied I get == [ INFO: possible circular locking dependency detected ] 3.6.0-03888-g3f99f3b #145 Not tainted And it really seems valid. kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which introduces slab_mutex - cpu_hotplug.lock dependency (through rcu_barrier() - _rcu_barrier() - get_online_cpus()). On the other hand, _cpu_up() acquires cpu_hotplug.lock through cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier gets called, which acquires slab_mutex. This gives the reverse dependency, i.e. deadlock scenario is valid one. 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because before that, there was no slab_mutex - cpu_hotplug.lock dependency. Simply put, the commit causes get_online_cpus() to be called with slab_mutex held, which is invalid. Oh, and it seems to be actually triggering in real. With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c + your patch, changing the order in which rcu_barrier() acquires hotplug and rcu_barrier locks, the machine hangs 100% reliably during suspend, which very likely actually is the deadlock described above. -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: I don't see how this circular locking dependency can occur.. If you are using SLUB, kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you are using SLAB, kmem_cache_destroy() wraps its whole operation inside get/put_online_cpus(), which means, it cannot run concurrently with a hotplug operation such as cpu_up(). So, I'm rather puzzled at this lockdep splat.. I am using SLAB here. The scenario I think is very well possible: 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) Deadlock. Right? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote: On Tue, 2 Oct 2012, Jiri Kosina wrote: 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543 Author: Paul E. McKenney paul.mcken...@linaro.org Date: Thu Aug 2 17:43:50 2012 -0700 rcu: Remove _rcu_barrier() dependency on __stop_machine() Currently, _rcu_barrier() relies on preempt_disable() to prevent any CPU from going offline, which in turn depends on CPU hotplug's use of __stop_machine(). This patch therefore makes _rcu_barrier() use get_online_cpus() to block CPU-hotplug operations. This has the added benefit of removing the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug operations are excluded, there can be no callbacks to adopt. This commit simplifies the code accordingly. Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com Reviewed-by: Josh Triplett j...@joshtriplett.org == is causing lockdep to complain (see the full trace below). I haven't yet had time to analyze what exactly is happening, and probably will not have time to do so until tomorrow, so just sending this as a heads-up in case anyone sees the culprit immediately. Hmmm... Does the following patch help? It swaps the order in which rcu_barrier() acquires the hotplug and rcu_barrier locks. It changed the report slightly (see for example the change in possible unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's now directly about cpu_hotplug.lock). With the patch applied I get == [ INFO: possible circular locking dependency detected ] 3.6.0-03888-g3f99f3b #145 Not tainted And it really seems valid. Yep, it sure is. I wasn't getting the full picture earlier, so please accept my apologies for the bogus patch. kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which introduces slab_mutex - cpu_hotplug.lock dependency (through rcu_barrier() - _rcu_barrier() - get_online_cpus()). On the other hand, _cpu_up() acquires cpu_hotplug.lock through cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier gets called, which acquires slab_mutex. This gives the reverse dependency, i.e. deadlock scenario is valid one. 1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because before that, there was no slab_mutex - cpu_hotplug.lock dependency. Simply put, the commit causes get_online_cpus() to be called with slab_mutex held, which is invalid. Oh, and it seems to be actually triggering in real. With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c + your patch, changing the order in which rcu_barrier() acquires hotplug and rcu_barrier locks, the machine hangs 100% reliably during suspend, which very likely actually is the deadlock described above. 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... Thanx, Paul -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
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? Thanks, -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
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.
Thanx, Paul
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[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 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 jkos...@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)
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
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 03:47 AM, Jiri Kosina wrote: On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: I don't see how this circular locking dependency can occur.. If you are using SLUB, kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you are using SLAB, kmem_cache_destroy() wraps its whole operation inside get/put_online_cpus(), which means, it cannot run concurrently with a hotplug operation such as cpu_up(). So, I'm rather puzzled at this lockdep splat.. I am using SLAB here. The scenario I think is very well possible: CPU 0 CPU 1 kmem_cache_destroy() What about the get_online_cpus() right here at CPU0 before calling mutex_lock(slab_mutex)? How can the cpu_up() proceed on CPU1?? I still don't get it... :( (kmem_cache_destroy() uses get/put_online_cpus() around acquiring and releasing slab_mutex). Regards, Srivatsa S. Bhat 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) Deadlock. Right? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 Wed, Oct 03, 2012 at 02:45:30AM +0200, 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).
You can certainly add my Reviewed-by, for whatever that is worth. ;-)
BTW, with this patch, are you able to dispense with my earlier patch,
or is it still needed?
Thanx, Paul
From: Jiri Kosina jkos...@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)
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, Oct 03, 2012 at 09:05:31AM +0530, Srivatsa S. Bhat wrote: On 10/03/2012 03:47 AM, Jiri Kosina wrote: On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: I don't see how this circular locking dependency can occur.. If you are using SLUB, kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you are using SLAB, kmem_cache_destroy() wraps its whole operation inside get/put_online_cpus(), which means, it cannot run concurrently with a hotplug operation such as cpu_up(). So, I'm rather puzzled at this lockdep splat.. I am using SLAB here. The scenario I think is very well possible: CPU 0 CPU 1 kmem_cache_destroy() What about the get_online_cpus() right here at CPU0 before calling mutex_lock(slab_mutex)? How can the cpu_up() proceed on CPU1?? I still don't get it... :( (kmem_cache_destroy() uses get/put_online_cpus() around acquiring and releasing slab_mutex). The problem is that there is a CPU-hotplug notifier for slab, which establishes hotplug-slab. Then having kmem_cache_destroy() call rcu_barrier() under the lock establishes slab-hotplug, which results in deadlock. Jiri really did explain this in an earlier email message, but both of us managed to miss it. ;-) Thanx, Paul Regards, Srivatsa S. Bhat 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) Deadlock. Right? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [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 jkos...@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
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
On Tue, 2 Oct 2012, Jiri Kosina wrote:
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of removing
the need for _rcu_barrier() to adopt callbacks: Because CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't yet
had time to analyze what exactly is happening, and probably will not
have
time to do so until tomorrow, so just sending this as a heads-up in case
anyone sees the culprit immediately.
Hmmm... Does the following patch help? It swaps the order in which
rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
And it really seems valid.
Yep, it sure is. I wasn't getting the full picture earlier, so please
accept my apologies for the bogus patch.
kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
introduces slab_mutex - cpu_hotplug.lock dependency (through
rcu_barrier() - _rcu_barrier() - get_online_cpus()).
On the other hand, _cpu_up() acquires cpu_hotplug.lock through
cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
gets called, which acquires slab_mutex. This gives the reverse dependency,
i.e. deadlock scenario is valid one.
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
before that, there was no slab_mutex - cpu_hotplug.lock dependency.
Simply put, the commit causes get_online_cpus() to be called with
slab_mutex held, which is invalid.
Oh, and it seems to be actually triggering in real.
With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
your patch, changing the order in which rcu_barrier() acquires hotplug and
rcu_barrier locks, the machine hangs 100% reliably during suspend, which
very likely actually is the deadlock described above.
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.
Why not? IMHO it should have been perfectly fine! See below...
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...
The thing is, get_online_cpus() is smart: it *knows* when you are calling
it in a hotplug-writer, IOW, when you are in a hotplug notifier.
The relevant code is:
void get_online_cpus(void)
{
might_sleep();
if (cpu_hotplug.active_writer == current)
return;
}
So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
notifier should pose no problem at all!
Regards,
Srivatsa S. Bhat
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 09:14 AM, Paul E. McKenney wrote: On Wed, Oct 03, 2012 at 09:05:31AM +0530, Srivatsa S. Bhat wrote: On 10/03/2012 03:47 AM, Jiri Kosina wrote: On Wed, 3 Oct 2012, Srivatsa S. Bhat wrote: I don't see how this circular locking dependency can occur.. If you are using SLUB, kmem_cache_destroy() releases slab_mutex before it calls rcu_barrier(). If you are using SLAB, kmem_cache_destroy() wraps its whole operation inside get/put_online_cpus(), which means, it cannot run concurrently with a hotplug operation such as cpu_up(). So, I'm rather puzzled at this lockdep splat.. I am using SLAB here. The scenario I think is very well possible: CPU 0 CPU 1 kmem_cache_destroy() What about the get_online_cpus() right here at CPU0 before calling mutex_lock(slab_mutex)? How can the cpu_up() proceed on CPU1?? I still don't get it... :( (kmem_cache_destroy() uses get/put_online_cpus() around acquiring and releasing slab_mutex). The problem is that there is a CPU-hotplug notifier for slab, which establishes hotplug-slab. Agreed. Then having kmem_cache_destroy() call rcu_barrier() under the lock Ah, that's where I disagree. kmem_cache_destroy() *cannot* proceed at this point in time, because it has invoked get_online_cpus()! It simply cannot be running past that point in the presence of a running hotplug notifier! So, kmem_cache_destroy() should have been sleeping on the hotplug lock, waiting for the notifier to release it, no? establishes slab-hotplug, which results in deadlock. Jiri really did explain this in an earlier email message, but both of us managed to miss it. ;-) Maybe I'm just being blind, sorry! ;-) Regards, Srivatsa S. Bhat Thanx, Paul Regards, Srivatsa S. Bhat 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) Deadlock. Right? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On Wed, Oct 03, 2012 at 09:29:01AM +0530, Srivatsa S. Bhat wrote:
On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
On Tue, 2 Oct 2012, Jiri Kosina wrote:
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of
removing
the need for _rcu_barrier() to adopt callbacks: Because
CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't
yet
had time to analyze what exactly is happening, and probably will not
have
time to do so until tomorrow, so just sending this as a heads-up in
case
anyone sees the culprit immediately.
Hmmm... Does the following patch help? It swaps the order in which
rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
And it really seems valid.
Yep, it sure is. I wasn't getting the full picture earlier, so please
accept my apologies for the bogus patch.
kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
introduces slab_mutex - cpu_hotplug.lock dependency (through
rcu_barrier() - _rcu_barrier() - get_online_cpus()).
On the other hand, _cpu_up() acquires cpu_hotplug.lock through
cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
gets called, which acquires slab_mutex. This gives the reverse
dependency,
i.e. deadlock scenario is valid one.
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
before that, there was no slab_mutex - cpu_hotplug.lock dependency.
Simply put, the commit causes get_online_cpus() to be called with
slab_mutex held, which is invalid.
Oh, and it seems to be actually triggering in real.
With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
your patch, changing the order in which rcu_barrier() acquires hotplug and
rcu_barrier locks, the machine hangs 100% reliably during suspend, which
very likely actually is the deadlock described above.
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.
Why not? IMHO it should have been perfectly fine! See below...
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...
The thing is, get_online_cpus() is smart: it *knows* when you are calling
it in a hotplug-writer, IOW, when you are in a hotplug notifier.
The relevant code is:
void get_online_cpus(void)
{
might_sleep();
if (cpu_hotplug.active_writer == current)
return;
}
So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
notifier should pose no problem at all!
Indeed, that was my confusion. The deadlock can happen with
the slab CPU-hotplug notifier (without calling rcu_barrier()!), which
establishes hotplug-slab. The some other unrelated thread calls
kmem_cache_destroy(), which acquires slab and then calls rcu_barrier(),
which acquires hotplug. So the deadlock can happen independently of
rcu_barrier() being called from a
Re: Lockdep complains about commit 1331e7a1bb (rcu: Remove _rcu_barrier() dependency on __stop_machine())
On 10/03/2012 09:37 AM, Paul E. McKenney wrote:
On Wed, Oct 03, 2012 at 09:29:01AM +0530, Srivatsa S. Bhat wrote:
On 10/03/2012 05:01 AM, Paul E. McKenney wrote:
On Tue, Oct 02, 2012 at 11:58:36PM +0200, Jiri Kosina wrote:
On Tue, 2 Oct 2012, Jiri Kosina wrote:
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is the first bad commit
commit 1331e7a1bbe1f11b19c4327ba0853bee2a606543
Author: Paul E. McKenney paul.mcken...@linaro.org
Date: Thu Aug 2 17:43:50 2012 -0700
rcu: Remove _rcu_barrier() dependency on __stop_machine()
Currently, _rcu_barrier() relies on preempt_disable() to prevent
any CPU from going offline, which in turn depends on CPU hotplug's
use of __stop_machine().
This patch therefore makes _rcu_barrier() use get_online_cpus() to
block CPU-hotplug operations. This has the added benefit of
removing
the need for _rcu_barrier() to adopt callbacks: Because
CPU-hotplug
operations are excluded, there can be no callbacks to adopt. This
commit simplifies the code accordingly.
Signed-off-by: Paul E. McKenney paul.mcken...@linaro.org
Signed-off-by: Paul E. McKenney paul...@linux.vnet.ibm.com
Reviewed-by: Josh Triplett j...@joshtriplett.org
==
is causing lockdep to complain (see the full trace below). I haven't
yet
had time to analyze what exactly is happening, and probably will not
have
time to do so until tomorrow, so just sending this as a heads-up in
case
anyone sees the culprit immediately.
Hmmm... Does the following patch help? It swaps the order in which
rcu_barrier() acquires the hotplug and rcu_barrier locks.
It changed the report slightly (see for example the change in possible
unsafe locking scenario, rcu_sched_state.barrier_mutex vanished and it's
now directly about cpu_hotplug.lock). With the patch applied I get
==
[ INFO: possible circular locking dependency detected ]
3.6.0-03888-g3f99f3b #145 Not tainted
And it really seems valid.
Yep, it sure is. I wasn't getting the full picture earlier, so please
accept my apologies for the bogus patch.
kmem_cache_destroy() calls rcu_barrier() with slab_mutex locked, which
introduces slab_mutex - cpu_hotplug.lock dependency (through
rcu_barrier() - _rcu_barrier() - get_online_cpus()).
On the other hand, _cpu_up() acquires cpu_hotplug.lock through
cpu_hotplug_begin(), and with this lock held cpuup_callback() notifier
gets called, which acquires slab_mutex. This gives the reverse
dependency,
i.e. deadlock scenario is valid one.
1331e7a1bbe1f11b19c4327ba0853bee2a606543 is triggering this, because
before that, there was no slab_mutex - cpu_hotplug.lock dependency.
Simply put, the commit causes get_online_cpus() to be called with
slab_mutex held, which is invalid.
Oh, and it seems to be actually triggering in real.
With HEAD being 974a847e00c, machine suspends nicely. With 974a847e00c +
your patch, changing the order in which rcu_barrier() acquires hotplug and
rcu_barrier locks, the machine hangs 100% reliably during suspend, which
very likely actually is the deadlock described above.
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.
Why not? IMHO it should have been perfectly fine! See below...
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...
The thing is, get_online_cpus() is smart: it *knows* when you are calling
it in a hotplug-writer, IOW, when you are in a hotplug notifier.
The relevant code is:
void get_online_cpus(void)
{
might_sleep();
if (cpu_hotplug.active_writer == current)
return;
}
So calling rcu_barrier() (and hence get_online_cpus()) from within a hotplug
notifier should pose no problem at all!
Indeed, that was my confusion. The deadlock can happen with
the slab CPU-hotplug notifier (without calling rcu_barrier()!), which
establishes hotplug-slab. The some other unrelated thread calls
kmem_cache_destroy(), which acquires slab and then calls rcu_barrier(),
which acquires hotplug. So the deadlock can happen independently of
rcu_barrier() being called from a CPU-hotplug notifier.
Right, this is exactly what I
