workqueue: WARN at at kernel/workqueue.c:2176

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel I've stumbled on the following spew:

[ 1297.886670] WARNING: CPU: 0 PID: 190 at kernel/workqueue.c:2176 process_one_work+0xb5/0x6f0()
[ 1297.889216] Modules linked in:
[ 1297.890306] CPU: 0 PID: 190 Comm: kworker/3:0 Not tainted 3.15.0-rc5-next-20140512-sasha-00019-ga20bc00-dirty #456
[ 1297.893258]  0000000000000009 ffff88010c5d7ce8 ffffffffb153e1ec 0000000000000002
[ 1297.893258]  0000000000000000 ffff88010c5d7d28 ffffffffae15fd6c ffff88010cdd6c98
[ 1297.893258]  ffff8806285d4000 ffffffffb3cd09e0 ffff88010cdde000 0000000000000000
[ 1297.893258] Call Trace:
[ 1297.893258] dump_stack (lib/dump_stack.c:52)
[ 1297.893258] warn_slowpath_common (kernel/panic.c:430)
[ 1297.893258] warn_slowpath_null (kernel/panic.c:465)
[ 1297.893258] process_one_work (kernel/workqueue.c:2174 (discriminator 38))
[ 1297.893258] worker_thread (kernel/workqueue.c:2354)
[ 1297.893258] kthread (kernel/kthread.c:210)
[ 1297.893258] ret_from_fork (arch/x86/kernel/entry_64.S:553)

Quoted 5 lines · Sasha Levin

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel I've stumbled on the following spew:

Quoted 6 lines

[ 1297.886670] WARNING: CPU: 0 PID: 190 at kernel/workqueue.c:2176 process_one_work+0xb5/0x6f0()
[ 1297.889216] Modules linked in:
[ 1297.890306] CPU: 0 PID: 190 Comm: kworker/3:0 Not tainted 3.15.0-rc5-next-20140512-sasha-00019-ga20bc00-dirty #456
[ 1297.893258]  0000000000000009 ffff88010c5d7ce8 ffffffffb153e1ec 0000000000000002
[ 1297.893258]  0000000000000000 ffff88010c5d7d28 ffffffffae15fd6c ffff88010cdd6c98
[ 1297.893258]  ffff8806285d4000 ffffffffb3cd09e0 ffff88010cdde000 0000000000000000

Quoted 8 lines

[ 1297.893258] Call Trace:
[ 1297.893258] dump_stack (lib/dump_stack.c:52)
[ 1297.893258] warn_slowpath_common (kernel/panic.c:430)
[ 1297.893258] warn_slowpath_null (kernel/panic.c:465)
[ 1297.893258] process_one_work (kernel/workqueue.c:2174 (discriminator 38))
[ 1297.893258] worker_thread (kernel/workqueue.c:2354)
[ 1297.893258] kthread (kernel/kthread.c:210)
[ 1297.893258] ret_from_fork (arch/x86/kernel/entry_64.S:553)

Hmm, this is "percpu worker on the wrong CPU while the current workqueue state indicates it should be on the CPU it's bound to" warning. We had a similar and more reproducible report a couple months back.

http://lkml.kernel.org/g/[email protected]

We added some debug code back then and it looked like the worker was setting the right cpus_allowed mask and the cpu was up but still ending up on the wrong CPU. Peter was looking into it and, ooh, I missed his last message and it fell through the crack. We probably should follow up on that thread.

Thanks.

Quoted 6 lines · Tejun Heo

Quoted 6 lines · Sasha Levin

Quoted 5 lines

Quoted 5 lines

Hi all,

While fuzzing with trinity inside a KVM tools guest running the latest -next kernel I've stumbled on the following spew:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

[ 1297.886670] WARNING: CPU: 0 PID: 190 at kernel/workqueue.c:2176 process_one_work+0xb5/0x6f0()
[ 1297.889216] Modules linked in:
[ 1297.890306] CPU: 0 PID: 190 Comm: kworker/3:0 Not tainted 3.15.0-rc5-next-20140512-sasha-00019-ga20bc00-dirty #456
[ 1297.893258]  0000000000000009 ffff88010c5d7ce8 ffffffffb153e1ec 0000000000000002
[ 1297.893258]  0000000000000000 ffff88010c5d7d28 ffffffffae15fd6c ffff88010cdd6c98
[ 1297.893258]  ffff8806285d4000 ffffffffb3cd09e0 ffff88010cdde000 0000000000000000

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

[ 1297.893258] Call Trace:
[ 1297.893258] dump_stack (lib/dump_stack.c:52)
[ 1297.893258] warn_slowpath_common (kernel/panic.c:430)
[ 1297.893258] warn_slowpath_null (kernel/panic.c:465)
[ 1297.893258] process_one_work (kernel/workqueue.c:2174 (discriminator 38))
[ 1297.893258] worker_thread (kernel/workqueue.c:2354)
[ 1297.893258] kthread (kernel/kthread.c:210)
[ 1297.893258] ret_from_fork (arch/x86/kernel/entry_64.S:553)

Hi,

I have been trying to address this bug.
Buy I can't reproduce this bug. Is your testing arch X86? if yes, could you find out how to reproduce the bug?

I don't have an easy way to reproduce it as I only saw the bug once, but it happened when I started pressuring CPU hotplug paths by adding and removing CPUs often. Maybe it has anything to do with that?

Quoted 3 lines · Sasha Levin

I don't have an easy way to reproduce it as I only saw the bug once, but it happened when I started pressuring CPU hotplug paths by adding and removing CPUs often. Maybe it has anything to do with that?

As per the original report (http://article.gmane.org/gmane.linux.kernel/1643027)
I am able to reproduce the problem.

The workload is (on S390 architecture):
2 processes onlining random cpus in a tight loop by using 'echo 1 > /sys/bus/cpu.../online'
2 processes offlining random cpus in a tight loop by using 'echo 0 > /sys/bus/cpu.../online'
Otherwise, fairly idle system. load average: 5.82, 6.27, 6.27

The machine has 10 processors.
The warning message some times hits within a few minutes on starting the workload. Other times it takes several hours.

-- Jason J. Herne ([email protected])

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

The following patch is just a workaround. After it is applied, the above WARNING is gone, but I can't hit the wq problem that you found.

---

kernel/cpu.c +3 -2

index a9e710e..253a129 100644
@@ -726,9 +726,10 @@ void set_cpu_present(unsigned int cpu, bool present)
 
 void set_cpu_online(unsigned int cpu, bool online)
 {
-	if (online)
+	if (online) {
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
-	else
+		cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+	} else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
 }
 

kernel/sched/core.c +0 -1

index 268a45e..c1a712d 100644
@@ -5043,7 +5043,6 @@ static int sched_cpu_active(struct notifier_block *nfb,
 				      unsigned long action, void *hcpu)
 {
 	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_STARTING:
 	case CPU_DOWN_FAILED:
 		set_cpu_active((long)hcpu, true);
 		return NOTIFY_OK;

Quoted 4 lines · Lai Jiangshan

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

Good find, and yes it does that.

Quoted 2 lines

The following patch is just a workaround. After it is applied, the above WARNING is gone, but I can't hit the wq problem that you found.

Seeing how the entirety of hotplug is basically duct tape and twigs, the below isn't that bad.

Quoted 31 lines

---

kernel/cpu.c +3 -2

index a9e710e..253a129 100644
@@ -726,9 +726,10 @@ void set_cpu_present(unsigned int cpu, bool present)
 
 void set_cpu_online(unsigned int cpu, bool online)
 {
-	if (online)
+	if (online) {
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
-	else
+		cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+	} else
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
 }
 

kernel/sched/core.c +0 -1

index 268a45e..c1a712d 100644
@@ -5043,7 +5043,6 @@ static int sched_cpu_active(struct notifier_block *nfb,
 				      unsigned long action, void *hcpu)
 {
 	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_STARTING:
 	case CPU_DOWN_FAILED:
 		set_cpu_active((long)hcpu, true);
 		return NOTIFY_OK;

Quoted 8 lines · Peter Zijlstra

Quoted 4 lines · Lai Jiangshan

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

Good find, and yes it does that.

Quoted 5 lines

Quoted 2 lines

The following patch is just a workaround. After it is applied, the above WARNING is gone, but I can't hit the wq problem that you found.

Seeing how the entirety of hotplug is basically duct tape and twigs, the below isn't that bad.

I think we need to find a more grace solution...

Quoted 4 lines · Lai Jiangshan

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

Good find, and yes it does that.

Quoted 2 lines

The following patch is just a workaround. After it is applied, the above WARNING is gone, but I can't hit the wq problem that you found.

Seeing how the entirety of hotplug is basically duct tape and twigs, the below isn't that bad.

kernel/sched/core.c static int sched_cpu_active(struct notif · line 5131

Quoted 10 lines · Peter Zijlstra

kernel/sched/core.c +0 -1

@@ -5126,7 +5126,6 @@ static int sched_cpu_active(struct notif
 				      unsigned long action, void *hcpu)
 {
 	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_STARTING:
 	case CPU_DOWN_FAILED:
 		set_cpu_active((long)hcpu, true);
 		return NOTIFY_OK;

In fact, I dropped this part as it conflicts with another patch I have, and setting it again isn't a big problem.

Hi, Peter and other scheduler Gurus:

When I was trying to test wq-VS-hotplug, I always hit a problem in scheduler with the following WARNING:

[   74.765519] WARNING: CPU: 1 PID: 13 at arch/x86/kernel/smp.c:124 native_smp_send_reschedule+0x2d/0x4b()
[   74.765520] Modules linked in: wq_hotplug(O) fuse cpufreq_ondemand ipv6 kvm_intel kvm uinput snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi e1000e snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer ptp iTCO_wdt iTCO_vendor_support lpc_ich snd mfd_core pps_core soundcore acpi_cpufreq i2c_i801 microcode wmi radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core
[   74.765545] CPU: 1 PID: 13 Comm: migration/1 Tainted: G           O  3.15.0-rc3+ #153
[   74.765546] Hardware name: LENOVO ThinkCentre M8200T/  , BIOS 5JKT51AUS 11/02/2010
[   74.765547]  000000000000007c ffff880236199c88 ffffffff814d7d2c 0000000000000000
[   74.765550]  0000000000000000 ffff880236199cc8 ffffffff8103add4 ffff880236199cb8
[   74.765552]  ffffffff81023e1b ffff8802361861c0 0000000000000001 ffff88023fd92b40
[   74.765555] Call Trace:
[   74.765559]  [<ffffffff814d7d2c>] dump_stack+0x51/0x75
[   74.765562]  [<ffffffff8103add4>] warn_slowpath_common+0x81/0x9b
[   74.765564]  [<ffffffff81023e1b>] ? native_smp_send_reschedule+0x2d/0x4b
[   74.765566]  [<ffffffff8103ae08>] warn_slowpath_null+0x1a/0x1c
[   74.765568]  [<ffffffff81023e1b>] native_smp_send_reschedule+0x2d/0x4b
[   74.765571]  [<ffffffff8105c2ea>] smp_send_reschedule+0xa/0xc
[   74.765574]  [<ffffffff8105fe46>] resched_task+0x5e/0x62
[   74.765576]  [<ffffffff81060238>] check_preempt_curr+0x43/0x77
[   74.765578]  [<ffffffff81060680>] __migrate_task+0xda/0x100
[   74.765580]  [<ffffffff810606a6>] ? __migrate_task+0x100/0x100
[   74.765582]  [<ffffffff810606c3>] migration_cpu_stop+0x1d/0x22
[   74.765585]  [<ffffffff810a33c6>] cpu_stopper_thread+0x84/0x116
[   74.765587]  [<ffffffff814d8642>] ? __schedule+0x559/0x581
[   74.765590]  [<ffffffff814dae3c>] ? _raw_spin_lock_irqsave+0x12/0x3c
[   74.765592]  [<ffffffff8105bd75>] ? __smpboot_create_thread+0x109/0x109
[   74.765594]  [<ffffffff8105bf46>] smpboot_thread_fn+0x1d1/0x1d6
[   74.765598]  [<ffffffff81056665>] kthread+0xad/0xb5
[   74.765600]  [<ffffffff810565b8>] ? kthread_freezable_should_stop+0x41/0x41
[   74.765603]  [<ffffffff814e0e2c>] ret_from_fork+0x7c/0xb0
[   74.765605]  [<ffffffff810565b8>] ? kthread_freezable_should_stop+0x41/0x41
[   74.765607] ---[ end trace 662efb362b4e8ed0 ]---

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

This of course leaves the question how the workqueue code manages to call set_cpu_allowed_ptr() on a cpu _before_ its online.

That too sounds fishy.. with the proposed patch the set_cpus_allowed_ptr() will 'gracefully' fail, but calling it in the first place is of course dubious too.

Quoted 6 lines · Peter Zijlstra

This of course leaves the question how the workqueue code manages to call set_cpu_allowed_ptr() on a cpu _before_ its online.

That too sounds fishy.. with the proposed patch the set_cpus_allowed_ptr() will 'gracefully' fail, but calling it in the first place is of course dubious too.

Right after being created, a workqueue worker invokes set_cpus_allowed_ptr() to the target cpumask without checking whether the cpu[s] are online or not and it's allowed to fail. The guarantee there is that the worker is already registered by that point and if a CPU comes online after the registration, CPU_ONLINE notification will update the cpumask accordingly, so either way the worker is guaranteed to be on the right cpumask.

Quoted 8 lines · Tejun Heo

Quoted 6 lines · Peter Zijlstra

This of course leaves the question how the workqueue code manages to call set_cpu_allowed_ptr() on a cpu _before_ its online.

That too sounds fishy.. with the proposed patch the set_cpus_allowed_ptr() will 'gracefully' fail, but calling it in the first place is of course dubious too.

Quoted 7 lines

Right after being created, a workqueue worker invokes set_cpus_allowed_ptr() to the target cpumask without checking whether the cpu[s] are online or not and it's allowed to fail. The guarantee there is that the worker is already registered by that point and if a CPU comes online after the registration, CPU_ONLINE notification will update the cpumask accordingly, so either way the worker is guaranteed to be on the right cpumask.

That's what the kthread_create_on_cpu/kthread_park/kthread_unpark infrastructure is for.

Quoted 35 lines · Lai Jiangshan

Hi, Peter and other scheduler Gurus:

When I was trying to test wq-VS-hotplug, I always hit a problem in scheduler with the following WARNING:

[   74.765519] WARNING: CPU: 1 PID: 13 at arch/x86/kernel/smp.c:124 native_smp_send_reschedule+0x2d/0x4b()
[   74.765520] Modules linked in: wq_hotplug(O) fuse cpufreq_ondemand ipv6 kvm_intel kvm uinput snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec_hdmi e1000e snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_timer ptp iTCO_wdt iTCO_vendor_support lpc_ich snd mfd_core pps_core soundcore acpi_cpufreq i2c_i801 microcode wmi radeon ttm drm_kms_helper drm i2c_algo_bit i2c_core
[   74.765545] CPU: 1 PID: 13 Comm: migration/1 Tainted: G           O  3.15.0-rc3+ #153
[   74.765546] Hardware name: LENOVO ThinkCentre M8200T/  , BIOS 5JKT51AUS 11/02/2010
[   74.765547]  000000000000007c ffff880236199c88 ffffffff814d7d2c 0000000000000000
[   74.765550]  0000000000000000 ffff880236199cc8 ffffffff8103add4 ffff880236199cb8
[   74.765552]  ffffffff81023e1b ffff8802361861c0 0000000000000001 ffff88023fd92b40
[   74.765555] Call Trace:
[   74.765559]  [<ffffffff814d7d2c>] dump_stack+0x51/0x75
[   74.765562]  [<ffffffff8103add4>] warn_slowpath_common+0x81/0x9b
[   74.765564]  [<ffffffff81023e1b>] ? native_smp_send_reschedule+0x2d/0x4b
[   74.765566]  [<ffffffff8103ae08>] warn_slowpath_null+0x1a/0x1c
[   74.765568]  [<ffffffff81023e1b>] native_smp_send_reschedule+0x2d/0x4b
[   74.765571]  [<ffffffff8105c2ea>] smp_send_reschedule+0xa/0xc
[   74.765574]  [<ffffffff8105fe46>] resched_task+0x5e/0x62
[   74.765576]  [<ffffffff81060238>] check_preempt_curr+0x43/0x77
[   74.765578]  [<ffffffff81060680>] __migrate_task+0xda/0x100
[   74.765580]  [<ffffffff810606a6>] ? __migrate_task+0x100/0x100
[   74.765582]  [<ffffffff810606c3>] migration_cpu_stop+0x1d/0x22
[   74.765585]  [<ffffffff810a33c6>] cpu_stopper_thread+0x84/0x116
[   74.765587]  [<ffffffff814d8642>] ? __schedule+0x559/0x581
[   74.765590]  [<ffffffff814dae3c>] ? _raw_spin_lock_irqsave+0x12/0x3c
[   74.765592]  [<ffffffff8105bd75>] ? __smpboot_create_thread+0x109/0x109
[   74.765594]  [<ffffffff8105bf46>] smpboot_thread_fn+0x1d1/0x1d6
[   74.765598]  [<ffffffff81056665>] kthread+0xad/0xb5
[   74.765600]  [<ffffffff810565b8>] ? kthread_freezable_should_stop+0x41/0x41
[   74.765603]  [<ffffffff814e0e2c>] ret_from_fork+0x7c/0xb0
[   74.765605]  [<ffffffff810565b8>] ? kthread_freezable_should_stop+0x41/0x41
[   74.765607] ---[ end trace 662efb362b4e8ed0 ]---

Quoted 5 lines

After debugging, I found the hotlug-in cpu is atctive but !online in this case. the problem was introduced by 5fbd036b.
Some code assumes that any cpu in cpu_active_mask is also online, but 5fbd036b breaks this assumption, so the corresponding code with this assumption should be changed too.

This of course leaves the question how the workqueue code manages to call set_cpu_allowed_ptr() on a cpu _before_ its online.

That too sounds fishy.. with the proposed patch the set_cpus_allowed_ptr() will 'gracefully' fail, but calling it in the first place is of course dubious too.

so the scheduler/set_cpus_allowed_ptr()/cpu_active_mask should be the first place to fix.

Quoted 2 lines · Lai Jiangshan

so the scheduler/set_cpus_allowed_ptr()/cpu_active_mask should be the first place to fix.

I'm not arguing about that, not to mention that this is userspace exposed and nobody protects that.

But I was expecting kernel stuff that calls it on hotplug to be serialized thusly, but apparently not so.

Quoted 7 lines · Peter Zijlstra

Quoted 2 lines · Lai Jiangshan

so the scheduler/set_cpus_allowed_ptr()/cpu_active_mask should be the first place to fix.

I'm not arguing about that, not to mention that this is userspace exposed and nobody protects that.

Quoted 3 lines

But I was expecting kernel stuff that calls it on hotplug to be serialized thusly, but apparently not so.

Was a final patch posted for this issue? The discussion made it sound like there were still a few things to figure out before we could resolve this bug. I can recreate this as needed and I'm happy to test any patches.

Quoted 12 lines · Jason J. Herne

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Lai Jiangshan

so the scheduler/set_cpus_allowed_ptr()/cpu_active_mask should be the first place to fix.

I'm not arguing about that, not to mention that this is userspace exposed and nobody protects that.

But I was expecting kernel stuff that calls it on hotplug to be serialized thusly, but apparently not so.

Quoted 3 lines

Was a final patch posted for this issue? The discussion made it sound like there were still a few things to figure out before we could resolve this bug. I can recreate this as needed and I'm happy to test any patches.

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=sched/urgent&id=6acbfb96976fc3350e30d964acb1dbbdf876d55e

which should make its way to Linus soonish I suppose.

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Jason J. Herne

Quoted 10 lines · Peter Zijlstra

Quoted 2 lines · Lai Jiangshan

so the scheduler/set_cpus_allowed_ptr()/cpu_active_mask should be the first place to fix.

I'm not arguing about that, not to mention that this is userspace exposed and nobody protects that.

But I was expecting kernel stuff that calls it on hotplug to be serialized thusly, but apparently not so.

Quoted 9 lines

Quoted 3 lines

Was a final patch posted for this issue? The discussion made it sound like there were still a few things to figure out before we could resolve this bug. I can recreate this as needed and I'm happy to test any patches.

https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/?h=sched/urgent&id=6acbfb96976fc3350e30d964acb1dbbdf876d55e

which should make its way to Linus soonish I suppose.

I applied the patch on top of c7208164e66f63e3ec1759b98087849286410741 and I am still hitting the problem. Should I have applied to a different branch/commit to pick up any other needed changes?

Patch applied:

kernel/cpu.c +4 -2

index a9e710e..247979a 100644
@@ -726,10 +726,12 @@ void set_cpu_present(unsigned int cpu, bool present)
 void set_cpu_online(unsigned int cpu, bool online)
 {
- if (online)
+ if (online) {
 cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
- else
+ cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+ } else {
 cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+ }
 }
 void set_cpu_active(unsigned int cpu, bool active)

kernel/sched/core.c +0 -1

index 44e00ab..86f3890 100644
@@ -5076,7 +5076,6 @@ static int sched_cpu_active(struct notifier_block *nfb,
 unsigned long action, void *hcpu)
 {
 switch (action & ~CPU_TASKS_FROZEN) {
- case CPU_STARTING:

case CPU_DOWN_FAILED:
set_cpu_active((long)hcpu, true);
return NOTIFY_OK;

Here is the output from the recreation using this patch:

[ 3634.146233] ------------[ cut here ]------------
[ 3634.146238] WARNING: at kernel/workqueue.c:2176
[ 3634.146239] Modules linked in: ipt_MASQUERADE iptable_nat nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack xt_CHECKSUM iptable_mangle bridge stp llc ip6table_filter ip6_tables ebtable_nat ebtables iscsi_tcp libiscsi_tcp libiscsi scsi_transport_iscsi qeth_l2 tape_3590 tape tape_class vhost_net tun vhost macvtap macvlan eadm_sch qeth ccwgroup zfcp scsi_transport_fc scsi_tgt qdio dasd_eckd_mod dasd_mod dm_multipath [last unloaded: kvm]
[ 3634.146260] CPU: 6 PID: 28009 Comm: kworker/7:0 Not tainted 3.15.0-rc7 #1
[ 3634.146263] Workqueue: \xffffff80           (null)
[ 3634.146264] task: 000000025def32e0 ti: 000000026dca0000 task.ti: 000000026dca0000
[ 3634.146266] Krnl PSW : 0404c00180000000 000000000015ad1a (process_one_work+0x2e6/0x4c0)
[ 3634.146272]            R:0 T:1 IO:0 EX:0 Key:0 M:1 W:0 P:0 AS:3 CC:0 PM:0 EA:3
Krnl GPRS: 0000000000000000 0000000000bc649a 00000002764b0980 0000000000b94f40
[ 3634.146275]            0000000000b94f40 0000000000000000 0000000000000000 0000000000bc6496
[ 3634.146277]            0000000000000000 000000008b65b600 000000008b657000 000000008b657018
[ 3634.146278]            00000002764b0980 0000000000b94f40 000000026dca3dd0 000000026dca3d70
[ 3634.146287] Krnl Code: 000000000015ad0e: 95001000        cli    0(%r1),0
           000000000015ad12: a774fece        brc    7,15aaae
          #000000000015ad16: a7f40001        brc    15,15ad18
          >000000000015ad1a: 92011000        mvi    0(%r1),1
           000000000015ad1e: a7f4fec8        brc    15,15aaae
           000000000015ad22: e31003180004    lg    %r1,792
           000000000015ad28: 58301024        l    %r3,36(%r1)
           000000000015ad2c: a73a0001        ahi    %r3,1
[ 3634.146299] Call Trace:
[ 3634.146301] ([<000000000015ace8>] process_one_work+0x2b4/0x4c0)
[ 3634.146303]  [<000000000015c100>] worker_thread+0x178/0x39c
[ 3634.146305]  [<0000000000164ba6>] kthread+0x10e/0x128
[ 3634.146310]  [<000000000072d026>] kernel_thread_starter+0x6/0xc
[ 3634.146312]  [<000000000072d020>] kernel_thread_starter+0x0/0xc
[ 3634.146313] Last Breaking-Event-Address:
[ 3634.146315]  [<000000000015ad16>] process_one_work+0x2e2/0x4c0
[ 3634.146316] ---[ end trace 03f51c9126c24171 ]---

I don't think this output provides anything new. Please let me know if I can gather any more data.

Hi, Peter,

I rewrote the analyse. (scheduler_ipi() must be called before stopper-task, so the part for workqueue of the old analyse maybe be wrong.)

---

kernel/sched/core.c +0 -0

index 268a45e..1a198a5 100644

Quoted 5 lines · Lai Jiangshan

Hi, Peter,

I rewrote the analyse. (scheduler_ipi() must be called before stopper-task, so the part for workqueue of the old analyse maybe be wrong.)

But I don't think there is any guarantee we'll do the wakeup before running the stop work.

Suppose the initial task gets queued, and the thing gets send the interrupt, meanwhile we'll do the stopper work wakeup !queueing, the set_cpus_allowed_ptr() isn't crossing llc boundaries.

Now, the remote cpu preempts/schedules before the interrupt hits and runs the stop task.

At which point we'll run __migrate_task() while the task is still queued on the wake list.

Quoted 6 lines

---

kernel/sched/core.c +0 -0

index 268a45e..1a198a5 100644

kernel/sched/core.c int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask) · line 4536

Quoted 8 lines

kernel/sched/core.c +1 -1

@@ -4530,7 +4530,7 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 
 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
-	if (p->on_rq) {
+	if (p->on_rq || p->state == TASK_WAKING) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */
 		task_rq_unlock(rq, p, &flags);

So while this will close the window somewhat, I don't think its entirely closed.

Hi, Jason

Could you test again after the following command has done.
(if Peter hasn't asked you test with this command before nor he doesn't stop you now)

echo NO_TTWU_QUEUE > /sys/kernel/debug/sched_features

Thanks a lot.

Hi, Peter,

I found something strange by review (just by review, no test yet)

__migrate_task()
{
...

	/*
	 * If we're not on a rq, the next wake-up will ensure we're
	 * placed properly.
	 */
	if (p->on_rq) {
		dequeue_task(rq_src, p, 0);
		set_task_cpu(p, dest_cpu);
		enqueue_task(rq_dest, p, 0);
		check_preempt_curr(rq_dest, p, 0);
	}

...
}

The comment is incorrect if TTWU_QUEUE is enabled. The task is waken-up even p->on_rq==0 in this case:

	p->wake_entry is added to the rq,
	p->state is TASK_WAKING
	p->on_rq is 0

In this case __migrate_task() fails to migrate the task!!!.

Go back to workqueue for higher level analysing.

task1 cpu#4 task3
workqueue_cpu_up_callback()
wake_up_process(worker1)
ttwu_queue_remote() #queue worker1 to cpu#4
set_cpus_allowed_ptr()
set worker's cpuallowed to
cpumask_of(5)

				#stopper_task
				__migrate_task()
				finds p->on_rq is 0,
				do nothing return

set_cpus_allowed_ptr() return 0

In this case, the WARN_ON() in process_one_work() hit.

Quoted 52 lines · Lai Jiangshan

Hi, Jason

Could you test again after the following command has done.
(if Peter hasn't asked you test with this command before nor he doesn't stop you now)

echo NO_TTWU_QUEUE > /sys/kernel/debug/sched_features

Thanks a lot.

Hi, Peter,

I found something strange by review (just by review, no test yet)

__migrate_task()
{
...

	/*
	 * If we're not on a rq, the next wake-up will ensure we're
	 * placed properly.
	 */
	if (p->on_rq) {
		dequeue_task(rq_src, p, 0);
		set_task_cpu(p, dest_cpu);
		enqueue_task(rq_dest, p, 0);
		check_preempt_curr(rq_dest, p, 0);
	}

...
}

The comment is incorrect if TTWU_QUEUE is enabled. The task is waken-up even p->on_rq==0 in this case:

	p->wake_entry is added to the rq,
	p->state is TASK_WAKING
	p->on_rq is 0

In this case __migrate_task() fails to migrate the task!!!.

Go back to workqueue for higher level analysing.

task1 cpu#4 task3
workqueue_cpu_up_callback()
wake_up_process(worker1)
ttwu_queue_remote() #queue worker1 to cpu#4
set_cpus_allowed_ptr()
set worker's cpuallowed to
cpumask_of(5)

				#stopper_task
				__migrate_task()
				finds p->on_rq is 0,
				do nothing return

set_cpus_allowed_ptr() return 0

Quoted 1 line

In this case, the WARN_ON() in process_one_work() hit.

Hmm, yes I think you're right. A queued wakeup can miss an affinity change like that.

Something like the below ought to cure that I suppose..

---
 kernel/sched/core.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

kernel/sched/core.c +15 -6

index 240aa83e73f5..0708ee21632f 100644
@@ -1521,17 +1521,32 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
+static void ttwu_queue_remote(struct task_struct *p, int cpu)
+{
+	if (llist_add(&p->wake_entry, &cpu_rq(cpu)->wake_list))
+		smp_send_reschedule(cpu);
+}
+
 static void sched_ttwu_pending(void)
 {
 	struct rq *rq = this_rq();
 	struct llist_node *llist = llist_del_all(&rq->wake_list);
 	struct task_struct *p;
+	int cpu;
 
 	raw_spin_lock(&rq->lock);
 
 	while (llist) {
 		p = llist_entry(llist, struct task_struct, wake_entry);
 		llist = llist_next(llist);
+
+		if (unlikely(!cpumask_test_cpu(rq->cpu, tsk_cpus_allowed(p)))) {
+			cpu = select_fallback_rq(rq->cpu, p);
+			set_task_cpu(p, cpu);
+			ttwu_queue_remote(p, cpu);
+			continue;
+		}
+
 		ttwu_do_activate(rq, p, 0);
 	}
 

@@ -1579,12 +1594,6 @@ void scheduler_ipi(void)
 	irq_exit();
 }
 
-static void ttwu_queue_remote(struct task_struct *p, int cpu)
-{
-	if (llist_add(&p->wake_entry, &cpu_rq(cpu)->wake_list))
-		smp_send_reschedule(cpu);
-}
-
 bool cpus_share_cache(int this_cpu, int that_cpu)
 {
 	return per_cpu(sd_llc_id, this_cpu) == per_cpu(sd_llc_id, that_cpu);

Quoted 4 lines

Hmm, yes I think you're right. A queued wakeup can miss an affinity change like that.

Something like the below ought to cure that I suppose..

As a non-scheduler developer, I can't find anything wrong with the patch (I searched all on_rq in kernel/sched).

but I think __migrate_task() is slow path comparing to sched_ttwu_pending(). So I prefer to change set_cpus_allowed_ptr() and __migrate_task() rather than to sched_ttwu_pending().

Quoted 7 lines · Lai Jiangshan

Quoted 4 lines

Hmm, yes I think you're right. A queued wakeup can miss an affinity change like that.

Something like the below ought to cure that I suppose..

As a non-scheduler developer, I can't find anything wrong with the patch (I searched all on_rq in kernel/sched).

You did fine finding that hole, so who knows, you might become one real soon now ;-)

Quoted 3 lines

but I think __migrate_task() is slow path comparing to sched_ttwu_pending(). So I prefer to change set_cpus_allowed_ptr() and __migrate_task() rather than to sched_ttwu_pending().

Yes, I agree, something there would be better, but I couldn't find anything without holes in.

I think the following code works. (inspirited from the sched_ttwu_pending() in migration_call().)

if p->on_rq == 0 && p->state == TASK_WAKING in __migrate_task() after this patch, it means the cpuallowed is changed before __migrate_task() along with other scheduler movements happens between sched_ttwu_pending() and __migrate_task().

kernel/sched/core.c +1 -1

index 268a45e..277f3bc 100644
@@ -4530,7 +4530,7 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 
 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
-	if (p->on_rq) {
+	if (p->on_rq || p->state == TASK_WAKING) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */
 		task_rq_unlock(rq, p, &flags);

kernel/sched/core.c +1 -0

+	sched_ttwu_pending();
 	__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu);
 	local_irq_enable();
 	return 0;

The patch is not tested by Jason, I don't know whether the patch fix the problem. The changlog including the "Reported-by:" and "Tested-by:" need to be updated after it is proved.

The patch is not tested by Jason, I don't know whether the patch fix the problem. The changlog including the "Reported-by:" and "Tested-by:" need to be updated after it is proved.

------------

Subject: [PATCH] sched: migrate the waking tasks

Current code skips to migrate the waking task silently when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not on queue (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it due to it is not on queue.

This behavior is incorrect, because the task had been already waken-up, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to make the waking tasks on-queue (transfer the waking tasks to running state) before migrate them.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/sched/core.c +17 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
  }
 
  #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
  {
-	struct rq *rq = this_rq();
  	struct llist_node *llist = llist_del_all(&rq->wake_list);
  	struct task_struct *p;
 
-	raw_spin_lock(&rq->lock);
-
  	while (llist) {
  		p = llist_entry(llist, struct task_struct, wake_entry);
  		llist = llist_next(llist);
  		ttwu_do_activate(rq, p, 0);
  	}
+}
 
+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+
+	raw_spin_lock(&rq->lock);
+	sched_ttwu_pending_locked(rq);
  	raw_spin_unlock(&rq->lock);
  }
 

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
  		goto out;
 
  	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq);
+
  	if (p->on_rq) {
  		struct migration_arg arg = { p, dest_cpu };
  		/* Need help from migration thread: drop lock and wait. */

@@ -4576,6 +4585,10 @@ static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu)
  	if (!cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)))
  		goto fail;
 
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq_src);
+
  	/*
  	 * If we're not on a rq, the next wake-up will ensure we're
  	 * placed properly.

kernel/sched/core.c +1 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
 {
-	struct rq *rq = this_rq();
 	struct llist_node *llist = llist_del_all(&rq->wake_list);
 	struct task_struct *p;
 
-	raw_spin_lock(&rq->lock);
-
 	while (llist) {
 		p = llist_entry(llist, struct task_struct, wake_entry);
 		llist = llist_next(llist);
 		ttwu_do_activate(rq, p, 0);
 	}

kernel/sched/core.c +1 -0

+
 	if (p->on_rq) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */

Quoted 24 lines · Lai Jiangshan

kernel/sched/core.c +2 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
 {
-	struct rq *rq = this_rq();
 	struct llist_node *llist = llist_del_all(&rq->wake_list);
 	struct task_struct *p;
 
-	raw_spin_lock(&rq->lock);
-
 	while (llist) {
 		p = llist_entry(llist, struct task_struct, wake_entry);
 		llist = llist_next(llist);
 		ttwu_do_activate(rq, p, 0);
 	}
+}
 

kernel/sched/core.c static int ttwu_remote(struct task_struct *p, int wake_flags) · line 1496

Quoted 8 lines

kernel/sched/core.c +6 -0

+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+
+	raw_spin_lock(&rq->lock);
+	sched_ttwu_pending_locked(rq);
 	raw_spin_unlock(&rq->lock);
 }

OK, so this won't apply to a recent kernel.

Quoted 4 lines

kernel/sched/core.c +0 -0

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 

kernel/sched/core.c int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask) · line 4540

Quoted 5 lines

kernel/sched/core.c +4 -0

 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq);

So I would really rather like to avoid this if possible, its doing full remote queueing, exactly what we tried to avoid.

Quoted 6 lines

kernel/sched/core.c +1 -0

+
 	if (p->on_rq) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */

kernel/sched/core.c static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu) · line 4594

Quoted 10 lines

kernel/sched/core.c +4 -0

@@ -4576,6 +4585,10 @@ static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu)
 	if (!cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)))
 		goto fail;
 
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq_src);
+
 	/*
 	 * If we're not on a rq, the next wake-up will ensure we're
 	 * placed properly.

Oh man, another variant.. why did you change it again? And without explanation for why you changed it.

I don't see a reason to call sched_ttwu_pending() with rq->lock held, seeing as how we append to that list without it held.

I'm still thinking the previous version is good, can you explain why you changed it?

.

Quoted 22 lines · Lai Jiangshan

kernel/sched/core.c +1 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
 }
 
 #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
 {
-	struct rq *rq = this_rq();
 	struct llist_node *llist = llist_del_all(&rq->wake_list);
 	struct task_struct *p;
 
-	raw_spin_lock(&rq->lock);
-
 	while (llist) {
 		p = llist_entry(llist, struct task_struct, wake_entry);
 		llist = llist_next(llist);
 		ttwu_do_activate(rq, p, 0);
 	}

kernel/sched/core.c static int ttwu_remote(struct task_struct *p, int wake_flags) · line 1496

Quoted 10 lines

kernel/sched/core.c +7 -0

+}
 
+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+
+	raw_spin_lock(&rq->lock);
+	sched_ttwu_pending_locked(rq);
 	raw_spin_unlock(&rq->lock);
 }

OK, so this won't apply to a recent kernel.

kernel/sched/core.c int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask) · line 4540

Quoted 8 lines

kernel/sched/core.c +4 -0

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 
 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq);

So I would really rather like to avoid this if possible, its doing full remote queueing, exactly what we tried to avoid.

Quoted 5 lines

kernel/sched/core.c +1 -0

+
 	if (p->on_rq) {
 		struct migration_arg arg = { p, dest_cpu };
 		/* Need help from migration thread: drop lock and wait. */

kernel/sched/core.c static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu) · line 4594

Quoted 10 lines

kernel/sched/core.c +4 -0

@@ -4576,6 +4585,10 @@ static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu)
 	if (!cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)))
 		goto fail;
 
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq_src);
+
 	/*
 	 * If we're not on a rq, the next wake-up will ensure we're
 	 * placed properly.

Oh man, another variant.. why did you change it again? And without explanation for why you changed it.

I don't see a reason to call sched_ttwu_pending() with rq->lock held, seeing as how we append to that list without it held.

I'm still thinking the previous version is good, can you explain why you changed it?

.

Hi, Peter

Could you make a patch for it, please? Jason J. Herne's test showed we addressed the bug. But the fix is not in kernel yet. Some new highly related reports are come up again.

I don't want to argue any more, no matter how the patch will be, I will accept. And please add the following tags in your patch:

Quoted 6 lines · Lai Jiangshan

Hi, Peter

Could you make a patch for it, please? Jason J. Herne's test showed we addressed the bug. But the fix is not in kernel yet. Some new highly related reports are come up again.

Quoted 2 lines

I don't want to argue any more, no matter how the patch will be, I will accept. And please add the following tags in your patch:

Well; I said http://marc.info/?l=linux-kernel&m=140187009016179 was a good patch and only asked you to add a comment and make it a nice patch which I could apply: http://marc.info/?l=linux-kernel&m=140187477317886&w=2

Instead you posted an entirely different patch again.

So how about the below?

---
Subject: sched: Migrate waking tasks
From: Lai Jiangshan <[email protected]>
Date: Wed, 4 Jun 2014 16:25:15 +0800

Current code can fail to migrate a waking task (silently) when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not queued (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it because its invisible to them.

This behavior is incorrect, because the task has been already woken, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to finish the wakeup (so they appear on the runqueue) before we migrate them.

Reported-by Sasha Levin <[email protected]>

Reported-by Jason J. Herne <[email protected]>

Tested-by Jason J. Herne <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

---
 kernel/sched/core.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

kernel/sched/core.c int set_cpus_allowed_ptr(struct task_str · line 4645

Quoted 3 lines

kernel/sched/core.c +1 -1

 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
-	if (task_on_rq_queued(p)) {
+	if (task_on_rq_queued(p) || p->state == TASK_WAKING) {

unrelated question: why we have to stop the cpu even the task is not running?

------------

Subject: [PATCH] sched: migrate the waking tasks

Current code skips to migrate the waking task silently when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not on queue (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it due to it is not on queue.

This behavior is incorrect, because the task had been already waken-up, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to make the waking tasks on-queue (transfer the waking tasks to running state) before migrate them.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/sched/core.c +13 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
  }
 
  #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
  {
-	struct rq *rq = this_rq();
  	struct llist_node *llist = llist_del_all(&rq->wake_list);
  	struct task_struct *p;
 
-	raw_spin_lock(&rq->lock);
-
  	while (llist) {
  		p = llist_entry(llist, struct task_struct, wake_entry);
  		llist = llist_next(llist);
  		ttwu_do_activate(rq, p, 0);
  	}
+}
 
+static void sched_ttwu_pending(void)
+{
+	struct rq *rq = this_rq();
+
+	raw_spin_lock(&rq->lock);
+	sched_ttwu_pending_locked(rq);
  	raw_spin_unlock(&rq->lock);
  }
 

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
  		goto out;
 
  	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+	/* Ensure it is on rq for migration if it is waking */
+	if (p->state == TASK_WAKING)
+		sched_ttwu_pending_locked(rq);
+
  	if (p->on_rq) {
  		struct migration_arg arg = { p, dest_cpu };
  		/* Need help from migration thread: drop lock and wait. */

Quoted 68 lines · Lai Jiangshan

------------

Subject: [PATCH] sched: migrate the waking tasks

Current code skips to migrate the waking task silently when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not on queue (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it due to it is not on queue.

This behavior is incorrect, because the task had been already waken-up, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to make the waking tasks on-queue (transfer the waking tasks to running state) before migrate them.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/sched/core.c +13 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
  }
 
  #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
  {
-    struct rq *rq = this_rq();
      struct llist_node *llist = llist_del_all(&rq->wake_list);
      struct task_struct *p;
 
-    raw_spin_lock(&rq->lock);
-
      while (llist) {
          p = llist_entry(llist, struct task_struct, wake_entry);
          llist = llist_next(llist);
          ttwu_do_activate(rq, p, 0);
      }
+}
 
+static void sched_ttwu_pending(void)
+{
+    struct rq *rq = this_rq();
+
+    raw_spin_lock(&rq->lock);
+    sched_ttwu_pending_locked(rq);
      raw_spin_unlock(&rq->lock);
  }
 

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
          goto out;
 
      dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+    /* Ensure it is on rq for migration if it is waking */
+    if (p->state == TASK_WAKING)
+        sched_ttwu_pending_locked(rq);
+
      if (p->on_rq) {
          struct migration_arg arg = { p, dest_cpu };
          /* Need help from migration thread: drop lock and wait. */

@@ -4576,6 +4585,10 @@ static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu)
      if (!cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)))
          goto fail;
 

kernel/sched/core.c static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu) · line 4594

Quoted 8 lines

kernel/sched/core.c +4 -0

+    /* Ensure it is on rq for migration if it is waking */
+    if (p->state == TASK_WAKING)
+        sched_ttwu_pending_locked(rq_src);
+
      /*
       * If we're not on a rq, the next wake-up will ensure we're
       * placed properly.

FYI, this patch appears to fix the problem. I was able to run for 3 days without hitting the warning.

I see that you guys are still discussing the details of the fix. When you decide on a final solution I'm happy to retest. Just be sure to ask :). It is hard to tell what to test with so many patches and code snippets flying around all the time.

Happy coding.

Quoted 69 lines · Jason J. Herne

Quoted 68 lines · Lai Jiangshan

------------

Subject: [PATCH] sched: migrate the waking tasks

Current code skips to migrate the waking task silently when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not on queue (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it due to it is not on queue.

This behavior is incorrect, because the task had been already waken-up, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to make the waking tasks on-queue (transfer the waking tasks to running state) before migrate them.

Signed-off-by Lai Jiangshan <[email protected]>

---

kernel/sched/core.c +13 -4

index 268a45e..d05a5a1 100644
@@ -1474,20 +1474,24 @@ static int ttwu_remote(struct task_struct *p, int wake_flags)
  }
 
  #ifdef CONFIG_SMP
-static void sched_ttwu_pending(void)
+static void sched_ttwu_pending_locked(struct rq *rq)
  {
-    struct rq *rq = this_rq();
      struct llist_node *llist = llist_del_all(&rq->wake_list);
      struct task_struct *p;
 
-    raw_spin_lock(&rq->lock);
-
      while (llist) {
          p = llist_entry(llist, struct task_struct, wake_entry);
          llist = llist_next(llist);
          ttwu_do_activate(rq, p, 0);
      }
+}
 
+static void sched_ttwu_pending(void)
+{
+    struct rq *rq = this_rq();
+
+    raw_spin_lock(&rq->lock);
+    sched_ttwu_pending_locked(rq);
      raw_spin_unlock(&rq->lock);
  }
 

@@ -4530,6 +4534,11 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
          goto out;
 
      dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
+
+    /* Ensure it is on rq for migration if it is waking */
+    if (p->state == TASK_WAKING)
+        sched_ttwu_pending_locked(rq);
+
      if (p->on_rq) {
          struct migration_arg arg = { p, dest_cpu };
          /* Need help from migration thread: drop lock and wait. */

@@ -4576,6 +4585,10 @@ static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu)
      if (!cpumask_test_cpu(dest_cpu, tsk_cpus_allowed(p)))
          goto fail;
 

Quoted 10 lines

kernel/sched/core.c static int __migrate_task(struct task_struct *p, int src_cpu, int dest_cpu) · line 4594

Quoted 8 lines

kernel/sched/core.c +4 -0

+    /* Ensure it is on rq for migration if it is waking */
+    if (p->state == TASK_WAKING)
+        sched_ttwu_pending_locked(rq_src);
+
      /*
       * If we're not on a rq, the next wake-up will ensure we're
       * placed properly.

FYI, this patch appears to fix the problem. I was able to run for 3 days without hitting the warning.

Thank you for the test. It proves that we found the root cause.
Your tests are the most important, coding takes the second place, let it go forward step by step.

Thanks,
Lai

Quoted 5 lines

I see that you guys are still discussing the details of the fix. When you decide on a final solution I'm happy to retest. Just be sure to ask :). It is hard to tell what to test with so many patches and code snippets flying around all the time.

Happy coding.

--
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/
.

Commit-ID:  5cd038f53ed9ec7a17ab7d536a727363080f4210
Gitweb:     http://git.kernel.org/tip/5cd038f53ed9ec7a17ab7d536a727363080f4210
Author:     Lai Jiangshan <[email protected]>
AuthorDate: Wed, 4 Jun 2014 16:25:15 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Tue, 9 Sep 2014 06:47:27 +0200

sched: Migrate waking tasks

Current code can fail to migrate a waking task (silently) when TTWU_QUEUE is enabled.

When a task is waking, it is pending on the wake_list of the rq, but it is not queued (task->on_rq == 0). In this case, set_cpus_allowed_ptr() and __migrate_task() will not migrate it because its invisible to them.

This behavior is incorrect, because the task has been already woken, it will be running on the wrong CPU without correct placement until the next wake-up or update for cpus_allowed.

To fix this problem, we need to finish the wakeup (so they appear on the runqueue) before we migrate them.

Reported-by Sasha Levin <[email protected]>

Reported-by Jason J. Herne <[email protected]>

Tested-by Jason J. Herne <[email protected]>

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Cc Linus Torvalds <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/sched/core.c | 8 +++++++-

 1 file changed, 7 insertions(+), 1 deletion(-)

kernel/sched/core.c +0 -0

index a814b3c..78e5c83 100644

09.09.2014, 18:54, "tip-bot for Lai Jiangshan" <[email protected]>:

kernel/sched/core.c int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask) · line 4672

Quoted 13 lines

kernel/sched/core.c +1 -1

index a814b3c..78e5c83 100644
@@ -4666,7 +4666,7 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
 		goto out;
 
 	dest_cpu = cpumask_any_and(cpu_active_mask, new_mask);
-	if (task_on_rq_queued(p)) {
+	if (task_on_rq_queued(p) || p->state == TASK_WAKING) {
 		struct migration_arg arg = { p, dest_cpu };
  		/* Need help from migration thread: drop lock and wait. */
  		task_rq_unlock(rq, p, &flags);

About migration_cpu_stop():

kernel/sched/core.c static int migration_cpu_stop(void *data) · line 4810

Quoted 13 lines

kernel/sched/core.c +6 -0

@@ -4799,6 +4799,12 @@ static int migration_cpu_stop(void *data)
 	* be on another cpu but it doesn't matter.
 	*/
 	local_irq_disable();
+	/*
+	* We need to explicitly wake pending tasks before running
+	* __migrate_task() such that we will not miss enforcing cpus_allowed
+	* during wakeups, see set_cpus_allowed_ptr()'s TASK_WAKING test.
+	*/
+	sched_ttwu_pending();
 	__migrate_task(arg->task, raw_smp_processor_id(), arg->dest_cpu);
 	local_irq_enable();
 	return 0;

It looks like we do not need this hunk, because IPI happens earlier then stop class begins migration_cpu_stop() execution.

In the first hunk the check "p->state == TASK_WAKING" is under pi_lock, so if the task is really waking then the IPI is already set.

So, the first hunk is enough here, the second is not need.

Hi, Jason

Could you test again after the following command has done.
(if Peter hasn't asked you test with this command before nor he doesn't stop you now)

echo NO_TTWU_QUEUE > /sys/kernel/debug/sched_features

Quoted 5 lines · Lai Jiangshan

Hi, Jason

Could you test again after the following command has done.
(if Peter hasn't asked you test with this command before nor he doesn't stop you now)

Quoted 1 line

echo NO_TTWU_QUEUE > /sys/kernel/debug/sched_features

Off-topic!

Why sched_features is in debugfs? I thought it is in procfs or sysfs.

Commit-ID:  6acbfb96976fc3350e30d964acb1dbbdf876d55e
Gitweb:     http://git.kernel.org/tip/6acbfb96976fc3350e30d964acb1dbbdf876d55e
Author:     Lai Jiangshan <[email protected]>
AuthorDate: Fri, 16 May 2014 11:50:42 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 22 May 2014 10:21:31 +0200

sched: Fix hotplug vs. set_cpus_allowed_ptr()

Lai found that:

  WARNING: CPU: 1 PID: 13 at arch/x86/kernel/smp.c:124 native_smp_send_reschedule+0x2d/0x4b()
  ...
  migration_cpu_stop+0x1d/0x22

was caused by set_cpus_allowed_ptr() assuming that cpu_active_mask is always a sub-set of cpu_online_mask.

This isn't true since 5fbd036b552f ("sched: Cleanup cpu_active madness").

So set active and online at the same time to avoid this particular problem.

Fixes 5fbd036b552f ("sched: Cleanup cpu_active madness")

Signed-off-by Lai Jiangshan <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Gautham R. Shenoy <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Michael wang <[email protected]>

Cc Paul Gortmaker <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Srivatsa S. Bhat <[email protected]>

Cc Toshi Kani <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 kernel/cpu.c        | 6 ++++--
 kernel/sched/core.c | 1 -
 2 files changed, 4 insertions(+), 3 deletions(-)

kernel/cpu.c +4 -2

index a9e710e..247979a 100644
@@ -726,10 +726,12 @@ void set_cpu_present(unsigned int cpu, bool present)
 
 void set_cpu_online(unsigned int cpu, bool online)
 {
-	if (online)
+	if (online) {
 		cpumask_set_cpu(cpu, to_cpumask(cpu_online_bits));
-	else
+		cpumask_set_cpu(cpu, to_cpumask(cpu_active_bits));
+	} else {
 		cpumask_clear_cpu(cpu, to_cpumask(cpu_online_bits));
+	}
 }
 
 void set_cpu_active(unsigned int cpu, bool active)

kernel/sched/core.c +0 -1

index 44e00ab..86f3890 100644
@@ -5076,7 +5076,6 @@ static int sched_cpu_active(struct notifier_block *nfb,
 				      unsigned long action, void *hcpu)
 {
 	switch (action & ~CPU_TASKS_FROZEN) {
-	case CPU_STARTING:
 	case CPU_DOWN_FAILED:
 		set_cpu_active((long)hcpu, true);
 		return NOTIFY_OK;