lkml.org 
[lkml]   [2006]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
From
Date
SubjectRe: SPAM: Re: _cpu_down deadlock [was Re: 2.6.19-rc1-mm1]
On Thursday October 12, arjan@infradead.org wrote:
> On Thu, 2006-10-12 at 09:46 +1000, Neil Brown wrote:
> > On Wednesday October 11, akpm@osdl.org wrote:
> > > >
> > > > So A waits on B and C, C waits on B, B waits on A.
> > > > Deadlock.
> > >
> > > Except the entire operation is serialised by the the two top-level callers
> > > (cpu_up() and cpu_down()) taking mutex_lock(&cpu_add_remove_lock). Can
> > > lockdep be taught about that?
> >
> > So you are saying that even though we have locking sequences
> > A -> B and B -> A,
> > that cannot - in this case - cause a deadlock as both sequences only
> > ever happen under a third exclusive lock C,
> > So when lockdep records a lock-dependency A -> B, it should also
> > record a list of locks that are *always* held when that dependency
> > occurs.
>
> in that case... why are A and B there *at all* ?
>

:-)

Obviously because someone out-side of C might want to interact with
the data protected by A or B.

But wait... what are the implications of that.

The data managed by B (where B == cpu_chain.rwsem) is a list of
notifiers. Each notifier is called with CPU_DOWN_PREPARE and then
will be called with either CPU_DOWN_FAILED or CPU_DEAD.

Now because we release and reclaim B it is possible for someone to add
or remove a notifier. Either of these event means that the relevant
notifier will get called with one of these but not the other. It
seems likely that a notifier will be written to assume this
bracketing.
In fact workqueue_cpu_callback (which is such a notifier) does
mutex_lock(&workqueue_mutex);
in CPU_DOWN_PREPARE and
mutex_unlock(&workqueue_mutex);
in CPU_DOWN_FAILED and CPU_DEAD.

If it got registered in the middle of _cpu_down, then it would unlock
a mutex that wasn't locked. Now I suspect it cannot be registered
while _cpu_down is active as it is only registered once, very early.
But it certainly does raise the question of why all this locking
is needed....
I think I'm in favour of the following. It should clean up the
lockdep warning and seems to make sense.

NeilBrown

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
./kernel/cpu.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff .prev/kernel/cpu.c ./kernel/cpu.c
--- .prev/kernel/cpu.c 2006-10-12 17:46:37.000000000 +1000
+++ ./kernel/cpu.c 2006-10-12 17:51:50.000000000 +1000
@@ -126,9 +126,11 @@ static int _cpu_down(unsigned int cpu)
if (!cpu_online(cpu))
return -EINVAL;

- err = blocking_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE,
+ down_read(&cpu_chain.rwsem);
+ err = raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE,
(void *)(long)cpu);
if (err == NOTIFY_BAD) {
+ up_read(&cpu_chain.rwsem);
printk("%s: attempt to take down CPU %u failed\n",
__FUNCTION__, cpu);
return -EINVAL;
@@ -146,11 +148,11 @@ static int _cpu_down(unsigned int cpu)

if (IS_ERR(p)) {
/* CPU didn't die: tell everyone. Can't complain. */
- if (blocking_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED,
+ if (raw_notifier_call_chain(&cpu_chain, CPU_DOWN_FAILED,
(void *)(long)cpu) == NOTIFY_BAD)
BUG();

- err = PTR_ERR(p);
+ err = PTR_ERR(p);
goto out_allowed;
}

@@ -169,7 +171,7 @@ static int _cpu_down(unsigned int cpu)
put_cpu();

/* CPU is completely dead: tell everyone. Too late to complain. */
- if (blocking_notifier_call_chain(&cpu_chain, CPU_DEAD,
+ if (raw_notifier_call_chain(&cpu_chain, CPU_DEAD,
(void *)(long)cpu) == NOTIFY_BAD)
BUG();

@@ -178,6 +180,7 @@ static int _cpu_down(unsigned int cpu)
out_thread:
err = kthread_stop(p);
out_allowed:
+ up_read(&cpu_chain.rwsem);
set_cpus_allowed(current, old_allowed);
return err;
}
@@ -206,7 +209,8 @@ static int __devinit _cpu_up(unsigned in
if (cpu_online(cpu) || !cpu_present(cpu))
return -EINVAL;

- ret = blocking_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu);
+ down_read(&cpu_chain.rwsem);
+ ret = raw_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu);
if (ret == NOTIFY_BAD) {
printk("%s: attempt to bring up CPU %u failed\n",
__FUNCTION__, cpu);
@@ -223,13 +227,13 @@ static int __devinit _cpu_up(unsigned in
BUG_ON(!cpu_online(cpu));

/* Now call notifier in preparation. */
- blocking_notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);
+ raw_notifier_call_chain(&cpu_chain, CPU_ONLINE, hcpu);

out_notify:
if (ret != 0)
- blocking_notifier_call_chain(&cpu_chain,
+ raw_notifier_call_chain(&cpu_chain,
CPU_UP_CANCELED, hcpu);
-
+ up_read(&cpu_chain.rwsem);
return ret;
}

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
\
 
 \ /
  Last update: 2006-10-12 09:55    [from the cache]
©2003-2011 Jasper Spaans