[RFC] [PATCH -rt 0/5] patchset focusing on cpu hotplug

[RFC] [PATCH -rt 0/5] patchset focusing on cpu hotplug

[Yong Zhang]

When tring cpu hotplug, I get some warning. And these patches
are tring to fix them up.

BTW, I have marked some of them as RFC:
  patch#3: printk: don't call printk_tick in printk_needs_cpu()
    I don't think we need to call printk_tick() even in mainline
    kernel, fix me if I'm wrong.
  patch#5: cpufreq: get rid of get_online_cpus()/put_online_cpus
    IMHO, it's about lockdep false positive, but it's a little
    annoying. Maybe there is other way to fix it up.
    And the side effect is we introduce more latency than before
    though it's rare path.

More details in eatch patch :)

Thanks,
Yong

---
Yong Zhang (5):
  hotplug: sync_unplug: No '\n' in task name
  hotplug: Call cpu_unplug_begin() a little early
  printk: don't call printk_tick in printk_needs_cpu()
  workqueue: hotplug fix
  cpufreq: get rid of get_online_cpus()/put_online_cpus

 drivers/cpufreq/cpufreq.c |   20 +++++++-------------
 kernel/cpu.c              |   18 ++++++++----------
 kernel/printk.c           |    4 ++--
 kernel/workqueue.c        |   12 ++++++++----
 4 files changed, 25 insertions(+), 29 deletions(-)

[PATCH -rt 1/5] hotplug: sync_unplug: No '\n' in task name

[Yong Zhang]

Otherwise the output will look a little odd.

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 kernel/cpu.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 5f2382a..83fb084 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -139,7 +139,7 @@ static int cpu_unplug_begin(unsigned int cpu)
 	struct task_struct *tsk;
 
 	init_completion(&hp->synced);
-	tsk = kthread_create(sync_unplug_thread, hp, "sync_unplug/%d\n", cpu);
+	tsk = kthread_create(sync_unplug_thread, hp, "sync_unplug/%d", cpu);
 	if (IS_ERR(tsk))
 		return (PTR_ERR(tsk));
 	kthread_bind(tsk, cpu);
-- 
1.7.1

[PATCH -rt 2/5] hotplug: Call cpu_unplug_begin() a little early

[Yong Zhang]

cpu_unplug_begin() should be called before CPU_DOWN_PREPARE,
because at CPU_DOWN_PREPARE cpu_active is cleared and sched_domain
is rebuilt, it will finally lead to thread 'sync_unplug' will
be running on the cpu on which it's created.

I found by an incorrect warning on smp_processor_id() called by
sync_unplug/1, and trace shows below:
(echo 1 > /sys/device/system/cpu/cpu1/online)
  bash-1664  [000]    83.136620: _cpu_down: Bind sync_unplug to cpu 1
  bash-1664  [000]    83.136623: sched_wait_task: comm=sync_unplug/1 pid=1724 prio=120
  bash-1664  [000]    83.136624: _cpu_down: Wake sync_unplug
  bash-1664  [000]    83.136629: sched_wakeup: comm=sync_unplug/1 pid=1724 prio=120 success=1 target_cpu=000

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 kernel/cpu.c |   16 +++++++---------
 1 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/kernel/cpu.c b/kernel/cpu.c
index 83fb084..13066a3 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -337,22 +337,20 @@ static int __ref _cpu_down(unsigned int cpu, int tasks_frozen)
 		return -EBUSY;
 	}
 
-	err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
+	cpu_hotplug_begin();
+	err = cpu_unplug_begin(cpu);
 	if (err) {
-		nr_calls--;
-		__cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
-		printk("%s: attempt to take down CPU %u failed\n",
-				__func__, cpu);
+		printk("cpu_unplug_begin(%d) failed\n", cpu);
 		goto out_cancel;
 	}
 
-	cpu_hotplug_begin();
-	err = cpu_unplug_begin(cpu);
+	err = __cpu_notify(CPU_DOWN_PREPARE | mod, hcpu, -1, &nr_calls);
 	if (err) {
 		nr_calls--;
 		__cpu_notify(CPU_DOWN_FAILED | mod, hcpu, nr_calls, NULL);
-		printk("cpu_unplug_begin(%d) failed\n", cpu);
-		goto out_cancel;
+		printk("%s: attempt to take down CPU %u failed\n",
+				__func__, cpu);
+		goto out_release;
 	}
 
 	err = __stop_machine(take_cpu_down, &tcd_param, cpumask_of(cpu));
-- 
1.7.1

[RFC] [PATCH -rt 3/5] printk: don't call printk_tick in printk_needs_cpu()

[Yong Zhang]

printk_tick() can't be called in atomic context, otherwise below
warning will show:

[  117.597095] BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
[  117.597102] in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: kworker/0:0
[  117.597111] Pid: 0, comm: kworker/0:0 Not tainted 3.0.6-rt17-00284-gb76d419-dirty #7
[  117.597116] Call Trace:
[  117.597131]  [<c06e3b61>] ? printk+0x1d/0x24
[  117.597142]  [<c01390b6>] __might_sleep+0xe6/0x110
[  117.597151]  [<c06e634c>] rt_spin_lock+0x1c/0x30
[  117.597158]  [<c0142f26>] __wake_up+0x26/0x60
[  117.597166]  [<c014c78e>] printk_tick+0x3e/0x40
[  117.597173]  [<c014c7b4>] printk_needs_cpu+0x24/0x30
[  117.597181]  [<c017ecc8>] tick_nohz_stop_sched_tick+0x2e8/0x410
[  117.597191]  [<c017305a>] ? sched_clock_idle_wakeup_event+0x1a/0x20
[  117.597201]  [<c010182a>] cpu_idle+0x4a/0xb0
[  117.597209]  [<c06e0b97>] start_secondary+0xd3/0xd7

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 kernel/printk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 0c9a0ab..a50af4e 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1282,8 +1282,8 @@ void printk_tick(void)
 
 int printk_needs_cpu(int cpu)
 {
-	if (cpu_is_offline(cpu))
-		printk_tick();
+	if (unlikely(cpu_is_offline(cpu)))
+		__this_cpu_write(printk_pending, 0);
 	return __this_cpu_read(printk_pending);
 }
 
-- 
1.7.1

[PATCH -rt 4/5] workqueue: hotplug fix

[Yong Zhang]

BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
in_atomic(): 1, irqs_disabled(): 0, pid: 1739, name: bash
Pid: 1739, comm: bash Not tainted 3.0.6-rt17-00284-gb76d419 #3
Call Trace:
 [<c06e3b5d>] ? printk+0x1d/0x20
 [<c01390b6>] __might_sleep+0xe6/0x110
 [<c06e633c>] rt_spin_lock+0x1c/0x30
 [<c01655a6>] flush_gcwq+0x236/0x320
 [<c021c651>] ? kfree+0xe1/0x1a0
 [<c05b7178>] ? __cpufreq_remove_dev+0xf8/0x260
 [<c0183fad>] ? rt_down_write+0xd/0x10
 [<c06cd91e>] workqueue_cpu_down_callback+0x26/0x2d
 [<c06e9d65>] notifier_call_chain+0x45/0x60
 [<c0171cfe>] __raw_notifier_call_chain+0x1e/0x30
 [<c014c9b4>] __cpu_notify+0x24/0x40
 [<c06cbc6f>] _cpu_down+0xdf/0x330
 [<c06cbef0>] cpu_down+0x30/0x50
 [<c06cd6b0>] store_online+0x50/0xa7
 [<c06cd660>] ? acpi_os_map_memory+0xec/0xec
 [<c04f2faa>] sysdev_store+0x2a/0x40
 [<c02887a4>] sysfs_write_file+0xa4/0x100
 [<c0229ab2>] vfs_write+0xa2/0x170
 [<c0288700>] ? sysfs_poll+0x90/0x90
 [<c0229d92>] sys_write+0x42/0x70
 [<c06ecedf>] sysenter_do_call+0x12/0x2d
CPU 1 is now offline
SMP alternatives: switching to UP code
SMP alternatives: switching to SMP code
Booting Node 0 Processor 1 APIC 0x1
smpboot cpu 1: start_ip = 9b000
Initializing CPU#1
BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: kworker/0:0
Pid: 0, comm: kworker/0:0 Not tainted 3.0.6-rt17-00284-gb76d419 #3
Call Trace:
 [<c06e3b5d>] ? printk+0x1d/0x20
 [<c01390b6>] __might_sleep+0xe6/0x110
 [<c06e633c>] rt_spin_lock+0x1c/0x30
 [<c06cd85b>] workqueue_cpu_up_callback+0x56/0xf3
 [<c06e9d65>] notifier_call_chain+0x45/0x60
 [<c0171cfe>] __raw_notifier_call_chain+0x1e/0x30
 [<c014c9b4>] __cpu_notify+0x24/0x40
 [<c014c9ec>] cpu_notify+0x1c/0x20
 [<c06e1d43>] notify_cpu_starting+0x1e/0x20
 [<c06e0aad>] smp_callin+0xfb/0x10e
 [<c06e0ad9>] start_secondary+0x19/0xd7
NMI watchdog enabled, takes one hw-pmu counter.
Switched to NOHz mode on CPU #1

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 kernel/workqueue.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index e262f6b..ddbf89c 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -3195,30 +3195,34 @@ static int __devinit workqueue_cpu_up_callback(struct notifier_block *nfb,
 	}
 
 	/* some are called w/ irq disabled, don't disturb irq status */
-	spin_lock_irqsave(&gcwq->lock, flags);
 
 	switch (action) {
 	case CPU_UP_PREPARE:
+		spin_lock_irqsave(&gcwq->lock, flags);
 		BUG_ON(gcwq->first_idle);
 		gcwq->first_idle = new_worker;
+		spin_unlock_irqrestore(&gcwq->lock, flags);
 		break;
 
 	case CPU_UP_CANCELED:
+		spin_lock_irqsave(&gcwq->lock, flags);
 		destroy_worker(gcwq->first_idle);
 		gcwq->first_idle = NULL;
+		spin_unlock_irqrestore(&gcwq->lock, flags);
 		break;
 
 	case CPU_ONLINE:
+		spin_lock_irqsave(&gcwq->lock, flags);
 		spin_unlock_irq(&gcwq->lock);
 		kthread_bind(gcwq->first_idle->task, cpu);
 		spin_lock_irq(&gcwq->lock);
 		gcwq->flags |= GCWQ_MANAGE_WORKERS;
 		start_worker(gcwq->first_idle);
 		gcwq->first_idle = NULL;
+		spin_unlock_irqrestore(&gcwq->lock, flags);
 		break;
 	}
 
-	spin_unlock_irqrestore(&gcwq->lock, flags);
 
 	return notifier_from_errno(0);
 }
@@ -3274,14 +3278,14 @@ static void flush_gcwq(struct global_cwq *gcwq)
 
 	spin_unlock_irq(&gcwq->lock);
 
-	gcwq = get_gcwq(get_cpu());
+	gcwq = get_gcwq(get_cpu_light());
 	spin_lock_irq(&gcwq->lock);
 	list_for_each_entry_safe(work, nw, &non_affine_works, entry) {
 		list_del_init(&work->entry);
 		___queue_work(get_work_cwq(work)->wq, gcwq, work);
 	}
 	spin_unlock_irq(&gcwq->lock);
-	put_cpu();
+	put_cpu_light();
 }
 
 static int __devinit workqueue_cpu_down_callback(struct notifier_block *nfb,
-- 
1.7.1

Re: [PATCH -rt 4/5] workqueue: hotplug fix

[Thomas Gleixner]

On Sun, 16 Oct 2011, Yong Zhang wrote:
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -3195,30 +3195,34 @@ static int __devinit workqueue_cpu_up_callback(struct notifier_block *nfb,
>  	}
>  
>  	/* some are called w/ irq disabled, don't disturb irq status */
> -	spin_lock_irqsave(&gcwq->lock, flags);
>  
>  	switch (action) {
>  	case CPU_UP_PREPARE:
> +		spin_lock_irqsave(&gcwq->lock, flags);
>  		BUG_ON(gcwq->first_idle);
>  		gcwq->first_idle = new_worker;
> +		spin_unlock_irqrestore(&gcwq->lock, flags);
>  		break;
>  
>  	case CPU_UP_CANCELED:
> +		spin_lock_irqsave(&gcwq->lock, flags);
>  		destroy_worker(gcwq->first_idle);
>  		gcwq->first_idle = NULL;
> +		spin_unlock_irqrestore(&gcwq->lock, flags);
>  		break;
>  
>  	case CPU_ONLINE:
> +		spin_lock_irqsave(&gcwq->lock, flags);
>  		spin_unlock_irq(&gcwq->lock);
>  		kthread_bind(gcwq->first_idle->task, cpu);
>  		spin_lock_irq(&gcwq->lock);
>  		gcwq->flags |= GCWQ_MANAGE_WORKERS;
>  		start_worker(gcwq->first_idle);
>  		gcwq->first_idle = NULL;
> +		spin_unlock_irqrestore(&gcwq->lock, flags);
>  		break;
>  	}
>  
> -	spin_unlock_irqrestore(&gcwq->lock, flags);

This part of the patch is pretty pointless.
  
>  	return notifier_from_errno(0);
>  }
> @@ -3274,14 +3278,14 @@ static void flush_gcwq(struct global_cwq *gcwq)
>  
>  	spin_unlock_irq(&gcwq->lock);
>  
> -	gcwq = get_gcwq(get_cpu());
> +	gcwq = get_gcwq(get_cpu_light());

Right.

>  	spin_lock_irq(&gcwq->lock);
>  	list_for_each_entry_safe(work, nw, &non_affine_works, entry) {
>  		list_del_init(&work->entry);
>  		___queue_work(get_work_cwq(work)->wq, gcwq, work);
>  	}
>  	spin_unlock_irq(&gcwq->lock);
> -	put_cpu();
> +	put_cpu_light();
>  }
>  
>  static int __devinit workqueue_cpu_down_callback(struct notifier_block *nfb,
> -- 
> 1.7.1
> 
> 

Re: [PATCH -rt 4/5] workqueue: hotplug fix

[Yong Zhang]

On Wed, Oct 19, 2011 at 09:14:28AM +0200, Thomas Gleixner wrote:
> On Sun, 16 Oct 2011, Yong Zhang wrote:
> > --- a/kernel/workqueue.c
> > +++ b/kernel/workqueue.c
> > @@ -3195,30 +3195,34 @@ static int __devinit workqueue_cpu_up_callback(struct notifier_block *nfb,
> >  	}
> >  
> >  	/* some are called w/ irq disabled, don't disturb irq status */
> > -	spin_lock_irqsave(&gcwq->lock, flags);
> >  
> >  	switch (action) {
> >  	case CPU_UP_PREPARE:
> > +		spin_lock_irqsave(&gcwq->lock, flags);
> >  		BUG_ON(gcwq->first_idle);
> >  		gcwq->first_idle = new_worker;
> > +		spin_unlock_irqrestore(&gcwq->lock, flags);
> >  		break;
> >  
> >  	case CPU_UP_CANCELED:
> > +		spin_lock_irqsave(&gcwq->lock, flags);
> >  		destroy_worker(gcwq->first_idle);
> >  		gcwq->first_idle = NULL;
> > +		spin_unlock_irqrestore(&gcwq->lock, flags);
> >  		break;
> >  
> >  	case CPU_ONLINE:
> > +		spin_lock_irqsave(&gcwq->lock, flags);
> >  		spin_unlock_irq(&gcwq->lock);
> >  		kthread_bind(gcwq->first_idle->task, cpu);
> >  		spin_lock_irq(&gcwq->lock);
> >  		gcwq->flags |= GCWQ_MANAGE_WORKERS;
> >  		start_worker(gcwq->first_idle);
> >  		gcwq->first_idle = NULL;
> > +		spin_unlock_irqrestore(&gcwq->lock, flags);
> >  		break;
> >  	}
> >  
> > -	spin_unlock_irqrestore(&gcwq->lock, flags);
> 
> This part of the patch is pretty pointless.

But CPU_STARTING is called with irq disabled, and it will take
the lock unconditionally. Thus below warning is triggered:
BUG: sleeping function called from invalid context at kernel/rtmutex.c:645
in_atomic(): 1, irqs_disabled(): 1, pid: 0, name: kworker/0:0
Pid: 0, comm: kworker/0:0 Not tainted 3.0.6-rt17-00284-gb76d419 #3
Call Trace:
 [<c06e3b5d>] ? printk+0x1d/0x20
 [<c01390b6>] __might_sleep+0xe6/0x110
 [<c06e633c>] rt_spin_lock+0x1c/0x30
 [<c06cd85b>] workqueue_cpu_up_callback+0x56/0xf3
 [<c06e9d65>] notifier_call_chain+0x45/0x60
 [<c0171cfe>] __raw_notifier_call_chain+0x1e/0x30
 [<c014c9b4>] __cpu_notify+0x24/0x40
 [<c014c9ec>] cpu_notify+0x1c/0x20
 [<c06e1d43>] notify_cpu_starting+0x1e/0x20
 [<c06e0aad>] smp_callin+0xfb/0x10e
 [<c06e0ad9>] start_secondary+0x19/0xd7

Thanks,
Yong

Re: [PATCH -rt 4/5] workqueue: hotplug fix

[Thomas Gleixner]

On Mon, 24 Oct 2011, Yong Zhang wrote:
> On Wed, Oct 19, 2011 at 09:14:28AM +0200, Thomas Gleixner wrote:
> > > -	spin_unlock_irqrestore(&gcwq->lock, flags);
> > 
> > This part of the patch is pretty pointless.
> 
> But CPU_STARTING is called with irq disabled, and it will take
> the lock unconditionally. Thus below warning is triggered:

Aarg, yes.

Thanks,

	tglx

[RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Yong Zhang]

Fix below false positive (seems this is not a real deadlock scenario)
lockdep warning:

=======================================================
[ INFO: possible circular locking dependency detected ]
3.0.6-rt18-00293-g6f698ae-dirty #24
-------------------------------------------------------
bash/1709 is trying to acquire lock:
 (s_active#103){++++.+}, at: [<c02ab4a3>] sysfs_addrm_finish+0x33/0x60

but task is already holding lock:
 (cpu_hotplug.lock){+.+.+.}, at: [<c0150530>] cpu_hotplug_begin+0x20/0x50

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #2 (cpu_hotplug.lock){+.+.+.}:
       [<c018ba31>] validate_chain.clone.20+0x701/0x810
       [<c018e1c4>] __lock_acquire+0x3b4/0x8d0
       [<c018eda8>] lock_acquire+0x88/0x1b0
       [<c0736b48>] _mutex_lock+0x38/0x50
       [<c01506a1>] get_online_cpus+0x31/0x50
       [<c05ea533>] cpufreq_add_dev_interface+0x103/0x270
       [<c05ea9ac>] cpufreq_add_dev+0x30c/0x410
       [<c0524aa1>] sysdev_driver_register+0xb1/0x140
       [<c05e988e>] cpufreq_register_driver+0x8e/0x150
       [<c0a25e92>] acpi_cpufreq_init+0x78/0x8b
       [<c0101225>] do_one_initcall+0x35/0x170
       [<c09f5852>] kernel_init+0xb8/0x14e
       [<c073e742>] kernel_thread_helper+0x6/0x10

-> #1 (&(*({ do { const void *__vpp_verify = (typeof((&(cpu_policy_rwsem))))((void *)0); (void)__vpp_verify; } while (0); ({ unsigned long __ptr; __asm__ ("" : "=r"(__ptr) : "0"((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))); (typeof((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))) (__ptr + (((__per_cpu_offset[cpu])))); }); }))){+++++.}:
       [<c018ba31>] validate_chain.clone.20+0x701/0x810
       [<c018e1c4>] __lock_acquire+0x3b4/0x8d0
       [<c018eda8>] lock_acquire+0x88/0x1b0
       [<c0195f66>] __rt_down_read+0x36/0x60
       [<c0195faf>] rt_down_read+0xf/0x20
       [<c05e975f>] lock_policy_rwsem_read+0x3f/0x80
       [<c05ea3f4>] show+0x34/0x70
       [<c02a9fb7>] sysfs_read_file+0x87/0x130
       [<c024522f>] vfs_read+0x9f/0x170
       [<c0245348>] sys_read+0x48/0x80
       [<c073e15f>] sysenter_do_call+0x12/0x38

-> #0 (s_active#103){++++.+}:
       [<c018b2fa>] check_prev_add+0x5fa/0x630
       [<c018ba31>] validate_chain.clone.20+0x701/0x810
       [<c018e1c4>] __lock_acquire+0x3b4/0x8d0
       [<c018eda8>] lock_acquire+0x88/0x1b0
       [<c02aac85>] sysfs_deactivate+0xc5/0x120
       [<c02ab4a3>] sysfs_addrm_finish+0x33/0x60
       [<c02aba0a>] sysfs_remove_dir+0x6a/0x80
       [<c03bab3f>] kobject_del+0xf/0x30
       [<c03babbf>] kobject_release+0x5f/0x80
       [<c03bbf3d>] kref_put+0x2d/0x60
       [<c03baa7d>] kobject_put+0x1d/0x50
       [<c05eab6e>] __cpufreq_remove_dev+0xbe/0x260
       [<c07339f3>] cpufreq_cpu_callback+0x70/0x83
       [<c073ac5a>] notifier_call_chain+0x7a/0xa0
       [<c017a3fe>] __raw_notifier_call_chain+0x1e/0x30
       [<c0150494>] __cpu_notify+0x24/0x40
       [<c071b7c1>] _cpu_down+0x151/0x350
       [<c071b9f0>] cpu_down+0x30/0x50
       [<c071d550>] store_online+0x50/0xa7
       [<c052466a>] sysdev_store+0x2a/0x40
       [<c02a9ed4>] sysfs_write_file+0xa4/0x100
       [<c02450c2>] vfs_write+0xa2/0x170
       [<c02453c8>] sys_write+0x48/0x80
       [<c073e15f>] sysenter_do_call+0x12/0x38

other info that might help us debug this:

Chain exists of:
  s_active --> &(*({ do { const void *__vpp_verify = (typeof((&(cpu_policy_rwsem))))((void *)0); (void)__vpp_verify; } while (0); ({ unsigned long __ptr; __asm__ ("" : "=r"(__ptr) : "0"((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))); (typeof((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))) (__ptr + (((__per_cpu_offset[cpu])))); }); })) --> cpu_hotplug.lock

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(cpu_hotplug.lock);
                               lock(&(*({ do { const void *__vpp_verify = (typeof((&(cpu_policy_rwsem))))((void *)0); (void)__vpp_verify; } while (0); ({ unsigned long __ptr; __asm__ ("" : "=r"(__ptr) : "0"((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))); (typeof((typeof(*(&(cpu_policy_rwsem))) *)(&(cpu_policy_rwsem)))) (__ptr + (((__per_cpu_offset[cpu])))); }); })));
                               lock(cpu_hotplug.lock);
  lock(s_active);

 *** DEADLOCK ***

5 locks held by bash/1709:
 #0:  (&buffer->mutex){+.+.+.}, at: [<c02a9e57>] sysfs_write_file+0x27/0x100
 #1:  (s_active#119){.+.+.+}, at: [<c02a9eb9>] sysfs_write_file+0x89/0x100
 #2:  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<c011dc42>] cpu_hotplug_driver_lock+0x12/0x20
 #3:  (cpu_add_remove_lock){+.+.+.}, at: [<c01506d2>] cpu_maps_update_begin+0x12/0x20
 #4:  (cpu_hotplug.lock){+.+.+.}, at: [<c0150530>] cpu_hotplug_begin+0x20/0x50

stack backtrace:
Pid: 1709, comm: bash Not tainted 3.0.6-rt18-00293-g6f698ae-dirty #24
Call Trace:
 [<c0733d94>] ? printk+0x1d/0x21
 [<c018a28f>] print_circular_bug+0xcf/0xe0
 [<c018b2fa>] check_prev_add+0x5fa/0x630
 [<c018bfd9>] ? mark_lock_irq+0xc9/0x270
 [<c018ba31>] validate_chain.clone.20+0x701/0x810
 [<c018e1c4>] __lock_acquire+0x3b4/0x8d0
 [<c018ef24>] ? lockdep_init_map+0x54/0x4e0
 [<c018eda8>] lock_acquire+0x88/0x1b0
 [<c02ab4a3>] ? sysfs_addrm_finish+0x33/0x60
 [<c02aac85>] sysfs_deactivate+0xc5/0x120
 [<c02ab4a3>] ? sysfs_addrm_finish+0x33/0x60
 [<c0234382>] ? unlock_slab_and_free_delayed.clone.25+0x92/0xc0
 [<c0234382>] ? unlock_slab_and_free_delayed.clone.25+0x92/0xc0
 [<c02ab4a3>] sysfs_addrm_finish+0x33/0x60
 [<c02aba0a>] sysfs_remove_dir+0x6a/0x80
 [<c03bab3f>] kobject_del+0xf/0x30
 [<c03babbf>] kobject_release+0x5f/0x80
 [<c03bab60>] ? kobject_del+0x30/0x30
 [<c03bbf3d>] kref_put+0x2d/0x60
 [<c03baa7d>] kobject_put+0x1d/0x50
 [<c073620d>] ? rt_mutex_unlock+0xd/0x10
 [<c01961f2>] ? rt_up_write+0x22/0x30
 [<c05e918d>] ? unlock_policy_rwsem_write+0x2d/0x40
 [<c05eab6e>] __cpufreq_remove_dev+0xbe/0x260
 [<c019614b>] ? rt_down_write_trylock+0x4b/0x60
 [<c07339f3>] cpufreq_cpu_callback+0x70/0x83
 [<c073ac5a>] notifier_call_chain+0x7a/0xa0
 [<c017a3fe>] __raw_notifier_call_chain+0x1e/0x30
 [<c0150494>] __cpu_notify+0x24/0x40
 [<c071b7c1>] _cpu_down+0x151/0x350
 [<c071b9f0>] cpu_down+0x30/0x50
 [<c071d550>] store_online+0x50/0xa7
 [<c071d500>] ? acpi_os_map_memory+0xf2/0xf2
 [<c052466a>] sysdev_store+0x2a/0x40
 [<c02a9ed4>] sysfs_write_file+0xa4/0x100
 [<c02450c2>] vfs_write+0xa2/0x170
 [<c02a9e30>] ? sysfs_poll+0x90/0x90
 [<c02453c8>] sys_write+0x48/0x80
 [<c073e15f>] sysenter_do_call+0x12/0x38

Signed-off-by: Yong Zhang <yong.zhang0@gmail.com>
---
 drivers/cpufreq/cpufreq.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7741f0f..8561182 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -821,16 +821,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 			goto err_out_kobj_put;
 	}
 
-	get_online_cpus();
+	raw_spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_cpu(j, policy->cpus) {
 		if (!cpu_online(j))
 			continue;
-		raw_spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		per_cpu(cpufreq_cpu_data, j) = policy;
 		per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
-		raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
-	put_online_cpus();
+	raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	ret = cpufreq_add_dev_symlink(cpu, policy);
 	if (ret)
@@ -972,13 +970,11 @@ static int cpufreq_add_dev(struct sys_device *sys_dev)
 
 
 err_out_unregister:
-	get_online_cpus();
+	raw_spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_cpu(j, policy->cpus) {
-		raw_spin_lock_irqsave(&cpufreq_driver_lock, flags);
 		per_cpu(cpufreq_cpu_data, j) = NULL;
-		raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
-	put_online_cpus();
+	raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	kobject_put(&policy->kobj);
 	wait_for_completion(&policy->kobj_unregister);
@@ -1045,7 +1041,6 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
 	}
 #endif
 
-	raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 #ifdef CONFIG_SMP
 
 #ifdef CONFIG_HOTPLUG_CPU
@@ -1058,17 +1053,14 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
 	 * per_cpu(cpufreq_cpu_data) while holding the lock, and remove
 	 * the sysfs links afterwards.
 	 */
-	get_online_cpus();
 	if (unlikely(cpumask_weight(data->cpus) > 1)) {
 		for_each_cpu(j, data->cpus) {
 			if (j == cpu)
 				continue;
-			raw_spin_lock_irqsave(&cpufreq_driver_lock, flags);
 			per_cpu(cpufreq_cpu_data, j) = NULL;
-			raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 		}
 	}
-	put_online_cpus();
+	raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	if (unlikely(cpumask_weight(data->cpus) > 1)) {
 		for_each_cpu(j, data->cpus) {
@@ -1087,6 +1079,8 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
 			cpufreq_cpu_put(data);
 		}
 	}
+#else
+	raw_spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
 #endif
 
 	if (cpufreq_driver->target)
-- 
1.7.1

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Thomas Gleixner]

On Sun, 16 Oct 2011, Yong Zhang wrote:

> Fix below false positive (seems this is not a real deadlock scenario)
> lockdep warning:

This looks like you caused it with patch 1. Both need a bit more
thought, but thanks for catching that.

Thanks,

	tglx

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Yong Zhang]

On Wed, Oct 19, 2011 at 11:12:20AM +0200, Thomas Gleixner wrote:
> On Sun, 16 Oct 2011, Yong Zhang wrote:
> 
> > Fix below false positive (seems this is not a real deadlock scenario)
> > lockdep warning:
> 
> This looks like you caused it with patch 1.

Hmmm, yup.
CPU_DOWN_PREPARE will be under cpu_hotplug.lock.

> Both need a bit more
> thought, but thanks for catching that.

No sure whether it's big issue or not.
Mind showing more about your concern?

Thanks,
Yong

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Thomas Gleixner]

On Mon, 24 Oct 2011, Yong Zhang wrote:

> On Wed, Oct 19, 2011 at 11:12:20AM +0200, Thomas Gleixner wrote:
> > On Sun, 16 Oct 2011, Yong Zhang wrote:
> > 
> > > Fix below false positive (seems this is not a real deadlock scenario)
> > > lockdep warning:
> > 
> > This looks like you caused it with patch 1.
> 
> Hmmm, yup.
> CPU_DOWN_PREPARE will be under cpu_hotplug.lock.
> 
> > Both need a bit more
> > thought, but thanks for catching that.
> 
> No sure whether it's big issue or not.
> Mind showing more about your concern?

This is probably not the only place which will run into that issue and
I have not much interest to patch all those sites.

Thanks,

	tglx

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Yong Zhang]

On Mon, Oct 24, 2011 at 11:24:51AM +0200, Thomas Gleixner wrote:
> 
> 
> On Mon, 24 Oct 2011, Yong Zhang wrote:
> 
> > On Wed, Oct 19, 2011 at 11:12:20AM +0200, Thomas Gleixner wrote:
> > > On Sun, 16 Oct 2011, Yong Zhang wrote:
> > > 
> > > > Fix below false positive (seems this is not a real deadlock scenario)
> > > > lockdep warning:
> > > 
> > > This looks like you caused it with patch 1.
> > 
> > Hmmm, yup.
> > CPU_DOWN_PREPARE will be under cpu_hotplug.lock.
> > 
> > > Both need a bit more
> > > thought, but thanks for catching that.
> > 
> > No sure whether it's big issue or not.
> > Mind showing more about your concern?
> 
> This is probably not the only place which will run into that issue and
> I have not much interest to patch all those sites.

If so, I think that kind of violation has been caught in mainline,
because mainline has had CPU_DOWN_PREPARE called under
cpu_hotplug.lock, right?

Or am I missing something?

Thanks,
Yong

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Thomas Gleixner]

On Mon, 24 Oct 2011, Yong Zhang wrote:
> On Mon, Oct 24, 2011 at 11:24:51AM +0200, Thomas Gleixner wrote:
> > > No sure whether it's big issue or not.
> > > Mind showing more about your concern?
> > 
> > This is probably not the only place which will run into that issue and
> > I have not much interest to patch all those sites.
> 
> If so, I think that kind of violation has been caught in mainline,
> because mainline has had CPU_DOWN_PREPARE called under
> cpu_hotplug.lock, right?
> 
> Or am I missing something?

Hmm, no. I obviously forgot that I moved it in RT :(

So yes, it should be safe. Though the question is why cpufreq does not
suffer from that that problem in mainline. I'll stare at that code
some more.

Thanks,

	tglx

Re: [RFC] [PATCH -rt 5/5] cpufreq: get rid of get_online_cpus()/put_online_cpus()

[Yong Zhang]

On Mon, Oct 24, 2011 at 01:45:16PM +0200, Thomas Gleixner wrote:
> On Mon, 24 Oct 2011, Yong Zhang wrote:
> > On Mon, Oct 24, 2011 at 11:24:51AM +0200, Thomas Gleixner wrote:
> > > > No sure whether it's big issue or not.
> > > > Mind showing more about your concern?
> > > 
> > > This is probably not the only place which will run into that issue and
> > > I have not much interest to patch all those sites.
> > 
> > If so, I think that kind of violation has been caught in mainline,
> > because mainline has had CPU_DOWN_PREPARE called under
> > cpu_hotplug.lock, right?
> > 
> > Or am I missing something?
> 
> Hmm, no. I obviously forgot that I moved it in RT :(
> 
> So yes, it should be safe. Though the question is why cpufreq does not
> suffer from that that problem in mainline.

By this one: peter_zijlstra-fix-cpufreq.patch

Thanks,
Yong