On (06/28/13 10:13), Viresh Kumar wrote: > On 26 June 2013 02:45, Sergey Senozhatsky <[email protected]> > wrote: > > > > [ 60.277396] ====================================================== > > [ 60.277400] [ INFO: possible circular locking dependency detected ] > > [ 60.277407] 3.10.0-rc7-dbg-01385-g241fd04-dirty #1744 Not tainted > > [ 60.277411] ------------------------------------------------------- > > [ 60.277417] bash/2225 is trying to acquire lock: > > [ 60.277422] ((&(&j_cdbs->work)->work)){+.+...}, at: > > [<ffffffff810621b5>] flush_work+0x5/0x280 > > [ 60.277444] > > but task is already holding lock: > > [ 60.277449] (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff81042d8b>] > > cpu_hotplug_begin+0x2b/0x60 > > [ 60.277465] > > which lock already depends on the new lock. > > Hi Sergey, > > Can you try reverting this patch? > > commit 2f7021a815f20f3481c10884fe9735ce2a56db35 > Author: Michael Wang <[email protected]> > Date: Wed Jun 5 08:49:37 2013 +0000 > > cpufreq: protect 'policy->cpus' from offlining during __gov_queue_work() >
Hello, Yes, this helps, of course, but at the same time it returns the previous problem -- preventing cpu_hotplug in some places. I have a bit different (perhaps naive) RFC patch and would like to hear comments. The idead is to brake existing lock dependency chain by not holding cpu_hotplug lock mutex across the calls. In order to detect active refcount readers or active writer, refcount now may have the following values: -1: active writer -- only one writer may be active, readers are blocked 0: no readers/writer >0: active readers -- many readers may be active, writer is blocked "blocked" reader or writer goes to wait_queue. as soon as writer finishes (refcount becomes 0), it wakeups all existing processes in a wait_queue. reader perform wakeup call only when it sees that pending writer is present (active_writer is not NULL). cpu_hotplug lock now only required to protect refcount cmp, inc, dec operations so it can be changed to spinlock. The patch has survived the initial beating: echo 0 > /sys/devices/system/cpu/cpu2/online echo 0 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu2/online echo 0 > /sys/devices/system/cpu/cpu1/online echo 0 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu1/online echo 1 > /sys/devices/system/cpu/cpu3/online echo 0 > /sys/devices/system/cpu/cpu2/online echo 0 > /sys/devices/system/cpu/cpu1/online echo 0 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu1/online echo 0 > /sys/devices/system/cpu/cpu1/online echo 1 > /sys/devices/system/cpu/cpu2/online echo 1 > /sys/devices/system/cpu/cpu1/online echo 0 > /sys/devices/system/cpu/cpu3/online echo 1 > /sys/devices/system/cpu/cpu3/online Signed-off-by: Sergey Senozhatsky <[email protected]> --- kernel/cpu.c | 75 ++++++++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 55 insertions(+), 20 deletions(-) diff --git a/kernel/cpu.c b/kernel/cpu.c index 198a388..7fa7b0f 100644 --- a/kernel/cpu.c +++ b/kernel/cpu.c @@ -50,17 +50,25 @@ static int cpu_hotplug_disabled; #ifdef CONFIG_HOTPLUG_CPU static struct { - struct task_struct *active_writer; - struct mutex lock; /* Synchronizes accesses to refcount, */ - /* - * Also blocks the new readers during - * an ongoing cpu hotplug operation. + /* Synchronizes accesses to refcount, also blocks the new readers + * during an ongoing cpu hotplug operation. + */ + spinlock_t lock; + /* -1: active cpu hotplug process + * 0: unlocked + * >0: active fercount readers */ int refcount; + struct task_struct *active_writer; + /* Wait queue for new refcount readers during an ongoing + * cpu hotplug operation. + */ + wait_queue_head_t wait; } cpu_hotplug = { - .active_writer = NULL, - .lock = __MUTEX_INITIALIZER(cpu_hotplug.lock), + .lock = __SPIN_LOCK_INITIALIZER(cpu_hotplug.lock), .refcount = 0, + .active_writer = NULL, + .wait = __WAIT_QUEUE_HEAD_INITIALIZER(cpu_hotplug.wait), }; void get_online_cpus(void) @@ -68,10 +76,24 @@ void get_online_cpus(void) might_sleep(); if (cpu_hotplug.active_writer == current) return; - mutex_lock(&cpu_hotplug.lock); - cpu_hotplug.refcount++; - mutex_unlock(&cpu_hotplug.lock); + for (;;) { + DECLARE_WAITQUEUE(wait, current); + + spin_lock(&cpu_hotplug.lock); + if (++cpu_hotplug.refcount > 0) { + spin_unlock(&cpu_hotplug.lock); + break; + } + /* Ongoing cpu hotplug process */ + cpu_hotplug.refcount--; + add_wait_queue(&cpu_hotplug.wait, &wait); + __set_current_state(TASK_UNINTERRUPTIBLE); + + spin_unlock(&cpu_hotplug.lock); + schedule(); + remove_wait_queue(&cpu_hotplug.wait, &wait); + } } EXPORT_SYMBOL_GPL(get_online_cpus); @@ -79,15 +101,15 @@ void put_online_cpus(void) { if (cpu_hotplug.active_writer == current) return; - mutex_lock(&cpu_hotplug.lock); + spin_lock(&cpu_hotplug.lock); - if (WARN_ON(!cpu_hotplug.refcount)) + if (WARN_ON(cpu_hotplug.refcount == 0)) cpu_hotplug.refcount++; /* try to fix things up */ + cpu_hotplug.refcount--; - if (!--cpu_hotplug.refcount && unlikely(cpu_hotplug.active_writer)) - wake_up_process(cpu_hotplug.active_writer); - mutex_unlock(&cpu_hotplug.lock); - + if (unlikely(cpu_hotplug.active_writer)) + wake_up(&cpu_hotplug.wait); + spin_unlock(&cpu_hotplug.lock); } EXPORT_SYMBOL_GPL(put_online_cpus); @@ -118,19 +140,32 @@ static void cpu_hotplug_begin(void) cpu_hotplug.active_writer = current; for (;;) { - mutex_lock(&cpu_hotplug.lock); - if (likely(!cpu_hotplug.refcount)) + DECLARE_WAITQUEUE(wait, current); + + spin_lock(&cpu_hotplug.lock); + if (likely(--cpu_hotplug.refcount == -1)) { + spin_unlock(&cpu_hotplug.lock); break; + } + /* Refcount readers present */ + cpu_hotplug.refcount++; + add_wait_queue(&cpu_hotplug.wait, &wait); __set_current_state(TASK_UNINTERRUPTIBLE); - mutex_unlock(&cpu_hotplug.lock); + + spin_unlock(&cpu_hotplug.lock); schedule(); + remove_wait_queue(&cpu_hotplug.wait, &wait); } } static void cpu_hotplug_done(void) { + spin_lock(&cpu_hotplug.lock); cpu_hotplug.active_writer = NULL; - mutex_unlock(&cpu_hotplug.lock); + cpu_hotplug.refcount++; + spin_unlock(&cpu_hotplug.lock); + + wake_up(&cpu_hotplug.wait); } /* -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
