Messages in this thread | | | Date | Tue, 29 Apr 2008 18:33:50 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH 5/8] cpu: cpu-hotplug deadlock |
| |
On 04/29, Gautham R Shenoy wrote: > > cpu_hotplug.mutex is basically a lock-internal lock; but by keeping it locked > over the 'write' section (cpu_hotplug_begin/done) a lock inversion happens when > some of the write side code calls into code that would otherwise take a > read lock. > > And it so happens that read-in-write recursion is expressly permitted. > > Fix this by turning cpu_hotplug into a proper stand alone unfair reader/writer > lock that allows reader-in-reader and reader-in-writer recursion.
While the patch itself is very clean and understandable, I can't understand the changelog ;)
Could you explain what is the semantics difference? The current code allows read-in-write recursion too.
The only difference I can see is that now cpu_hotplug_begin() doesn't rely on cpu_add_remove_lock any longer (currently the caller must hold this lock), but this (good) change is not documented.
> static void cpu_hotplug_done(void) > { > + spin_lock(&cpu_hotplug.lock); > cpu_hotplug.active_writer = NULL; > - mutex_unlock(&cpu_hotplug.lock); > + if (!list_empty(&cpu_hotplug.writer_queue.task_list))
waitqueue_active() ?
> + wake_up(&cpu_hotplug.writer_queue); > + else > + wake_up_all(&cpu_hotplug.reader_queue);
Please note that wake_up() and wake_up_all() doesn't differ here, because cpu_hotplug_begin() use prepare_to_wait(), not prepare_to_wait_exclusive(). I'd suggest to change cpu_hotplug_begin(), and use wake_up() for both cases.
(actually, since write-locks should be very rare, perhaps we don't need 2 wait_queues ?)
Oleg.
| |