WARNING at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()

hi,
not sure you'd be interested nor if I can reproduce it, but got another workqueue warning

jirka

[ 4324.514324] ------------[ cut here ]------------
[ 4324.514547] WARNING: CPU: 0 PID: 12 at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()
[ 4324.514933] Modules linked in:
[ 4324.515076] CPU: 0 PID: 12 Comm: migration/0 Tainted: G        W    3.13.0+ #213
[ 4324.515411] Hardware name: Intel Corporation Montevina platform/To be filled by O.E.M., BIOS AMVACRB1.86C.0066.B00.0805070703 05/07/2008
[ 4324.515966]  0000000000000009 ffff8800001c9b78 ffffffff81643b6a 0000000000000004
[ 4324.516320]  0000000000000000 ffff8800001c9bb8 ffffffff81045a7c ffff88007a9d35c0
[ 4324.516674]  0000000000000001 ffff88007a9d35c0 ffff8800751828f8 0000000000000086
[ 4324.517027] Call Trace:
[ 4324.517141]  [<ffffffff81643b6a>] dump_stack+0x4f/0x7c
[ 4324.517374]  [<ffffffff81045a7c>] warn_slowpath_common+0x8c/0xc0
[ 4324.517647]  [<ffffffff81045aca>] warn_slowpath_null+0x1a/0x20
[ 4324.517912]  [<ffffffff810663f3>] wq_worker_waking_up+0x53/0x70
[ 4324.518181]  [<ffffffff81079f09>] ttwu_do_activate.constprop.98+0x59/0x70
[ 4324.518489]  [<ffffffff8107d99f>] try_to_wake_up+0x1cf/0x2e0
[ 4324.518745]  [<ffffffff8107dac2>] default_wake_function+0x12/0x20
[ 4324.519022]  [<ffffffff8108cad5>] __wake_up_common+0x55/0x90
[ 4324.519279]  [<ffffffff8107a3f0>] ? __migrate_task+0x1a0/0x1a0
[ 4324.519543]  [<ffffffff8108cb23>] __wake_up_locked+0x13/0x20
[ 4324.519799]  [<ffffffff8108d4d2>] complete+0x42/0x60
[ 4324.520024]  [<ffffffff8107a424>] ? migration_cpu_stop+0x34/0x40
[ 4324.520297]  [<ffffffff810d207d>] cpu_stop_signal_done+0x2d/0x30
[ 4324.520570]  [<ffffffff810d226f>] cpu_stopper_thread+0xaf/0x130
[ 4324.520838]  [<ffffffff81092f0e>] ? put_lock_stats.isra.18+0xe/0x30
[ 4324.521122]  [<ffffffff8164dded>] ? _raw_spin_unlock_irqrestore+0x6d/0x80
[ 4324.521430]  [<ffffffff8107af61>] ? get_parent_ip+0x11/0x50

[ 4324.521684]  [<ffffffff81074e91>] smpboot_thread_fn+0x1a1/0x2b0
[ 4324.521952]  [<ffffffff81074cf0>] ? SyS_setgroups+0x150/0x150
[ 4324.522213]  [<ffffffff8106db04>] kthread+0xe4/0x100
[ 4324.522439]  [<ffffffff816498a8>] ? wait_for_common+0xd8/0x160
[ 4324.522703]  [<ffffffff8106da20>] ? __init_kthread_worker+0x70/0x70
[ 4324.522988]  [<ffffffff8165682c>] ret_from_fork+0x7c/0xb0
[ 4324.523233]  [<ffffffff8106da20>] ? __init_kthread_worker+0x70/0x70
[ 4324.523517] ---[ end trace 8659853860e530bc ]---

acpi_processor_set_throttling() plays with set_cpus_allowed_ptr(current), this is obviously wrong, and the worker is bound.

Hello,

Quoted 2 lines · Oleg Nesterov

acpi_processor_set_throttling() plays with set_cpus_allowed_ptr(current), this is obviously wrong, and the worker is bound.

Umm... yeah, anything running on workqueues shouldn't be diddling with cpu affinity. The function even has /* FIXME: use work_on_cpu() */ in it. I suppose it's about time to actually implement that?

Thanks.

Quoted 10 lines · Tejun Heo

Hello,

Quoted 2 lines · Oleg Nesterov

acpi_processor_set_throttling() plays with set_cpus_allowed_ptr(current), this is obviously wrong, and the worker is bound.

Umm... yeah, anything running on workqueues shouldn't be diddling with cpu affinity. The function even has /* FIXME: use work_on_cpu() */ in it. I suppose it's about time to actually implement that?

Quoted 2 lines

Thanks.

Hi Jiri:
Could you try this patch which reworks ACPI processor throttling
with work_on_cpu()?

Quoted 11 lines · Lan Tianyu

Quoted 10 lines · Tejun Heo

Hello,

Quoted 2 lines · Oleg Nesterov

acpi_processor_set_throttling() plays with set_cpus_allowed_ptr(current), this is obviously wrong, and the worker is bound.

Umm... yeah, anything running on workqueues shouldn't be diddling with cpu affinity. The function even has /* FIXME: use work_on_cpu() */ in it. I suppose it's about time to actually implement that?

Quoted 6 lines

Quoted 2 lines

Thanks.

Hi Jiri:
Could you try this patch which reworks ACPI processor throttling
with work_on_cpu()?

hum, I've got difficulties to apply it..

[jolsa@krava2 linux-perf]$ git am /tmp/wq/
Applying: WARNING at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()
fatal: corrupt patch at line 8
Patch failed at 0001 WARNING at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()

jirka

From 058dc817636186d1e9ccadfa7a2409aaeb1458f4 Mon Sep 17 00:00:00 2001

Quoted 21 lines · Jiri Olsa

Quoted 11 lines · Lan Tianyu

Quoted 10 lines · Tejun Heo

Hello,

Quoted 2 lines · Oleg Nesterov

acpi_processor_set_throttling() plays with set_cpus_allowed_ptr(current), this is obviously wrong, and the worker is bound.

Umm... yeah, anything running on workqueues shouldn't be diddling with cpu affinity. The function even has /* FIXME: use work_on_cpu() */ in it. I suppose it's about time to actually implement that?

Quoted 6 lines

Quoted 2 lines

Thanks.

Hi Jiri:
Could you try this patch which reworks ACPI processor throttling
with work_on_cpu()?

hum, I've got difficulties to apply it..

Quoted 7 lines

[jolsa@krava2 linux-perf]$ git am /tmp/wq/
Applying: WARNING at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()
fatal: corrupt patch at line 8
Patch failed at 0001 WARNING at kernel/workqueue.c:829 wq_worker_waking_up+0x53/0x70()

jirka

Sorry. Please try the attachment again.

drivers/acpi/processor_throttling.c int acpi_processor_set_throttling(struct acpi_processor *pr, · line 1099

Quoted 6 lines · Lan Tianyu

drivers/acpi/processor_throttling.c +0 -1

@@ -1096,7 +1112,6 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 		return -ENODEV;
 	}
 
-	cpumask_copy(saved_mask, &current->cpus_allowed);

Yes, but the patch forgets to kill alloc_cpumask_var(&saved_mask) and saved_mask itself ;)

Quoted 7 lines · Oleg Nesterov

Quoted 5 lines · Lan Tianyu

drivers/acpi/processor_throttling.c +0 -0

@@ -1096,7 +1112,6 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
  		return -ENODEV;
  	}
 

Quoted 4 lines

drivers/acpi/processor_throttling.c int acpi_processor_set_throttling(struct acpi_processor *pr, · line 1099

Quoted 1 line

drivers/acpi/processor_throttling.c +0 -1

-	cpumask_copy(saved_mask, &current->cpus_allowed);

Yes, but the patch forgets to kill alloc_cpumask_var(&saved_mask) and saved_mask itself ;

And it seems that acpi_processor_set_throttling() doesn't need online_throttling_cpus at all. It could use for_each_cpu_and(cpu_online_mask, p_throttling->shared_cpu_map).

Oleg.

acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make sure struct acpi_processor->acpi_processor_set_throttling() callback run on associated cpu. But the function maybe called in a worker which has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr() with work_on_cpu().

Quoted 5 lines · Lan Tianyu

acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make sure struct acpi_processor->acpi_processor_set_throttling() callback run on associated cpu. But the function maybe called in a worker which has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr() with work_on_cpu().

testing the new patch.. so far so good ;-)

Quoted 7 lines · Jiri Olsa

Quoted 5 lines · Lan Tianyu

acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make sure struct acpi_processor->acpi_processor_set_throttling() callback run on associated cpu. But the function maybe called in a worker which has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr() with work_on_cpu().

Quoted 1 line

testing the new patch.. so far so good ;-)

ook.. survived whole day under the test workload without the warning ;-)

jirka

acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make sure struct acpi_processor->acpi_processor_set_throttling() callback run on associated cpu. But the function maybe called in a worker which has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr() with work_on_cpu().

Signed-off-by Lan Tianyu <[email protected]>

---
 drivers/acpi/processor_throttling.c | 70 +++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

drivers/acpi/processor_throttling.c +6 -0

index 28baa05..2db105a 100644
@@ -56,6 +56,12 @@ struct throttling_tstate {
 	int target_state;		/* target T-state */
 };
 
+struct acpi_processor_throttling_arg {
+	struct acpi_processor *pr;
+	int target_state;
+	bool force;
+};
+
 #define THROTTLING_PRECHANGE       (1)
 #define THROTTLING_POSTCHANGE      (2)
 

@@ -1060,16 +1066,25 @@ static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr,
 	return 0;

drivers/acpi/processor_throttling.c +19 -37

+}
+
 int acpi_processor_set_throttling(struct acpi_processor *pr,
 						int state, bool force)
 {
-	cpumask_var_t saved_mask;
 	int ret = 0;
 	unsigned int i;
 	struct acpi_processor *match_pr;
 	struct acpi_processor_throttling *p_throttling;
+	struct acpi_processor_throttling_arg arg;
 	struct throttling_tstate t_state;
-	cpumask_var_t online_throttling_cpus;
 
 	if (!pr)
 		return -EINVAL;

@@ -1080,14 +1095,6 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	if ((state < 0) || (state > (pr->throttling.state_count - 1)))
 		return -EINVAL;
 
-	if (!alloc_cpumask_var(&saved_mask, GFP_KERNEL))
-		return -ENOMEM;
-
-	if (!alloc_cpumask_var(&online_throttling_cpus, GFP_KERNEL)) {
-		free_cpumask_var(saved_mask);
-		return -ENOMEM;
-	}
-
 	if (cpu_is_offline(pr->id)) {
 		/*
 		 * the cpu pointed by pr->id is offline. Unnecessary to change

@@ -1096,17 +1103,15 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 		return -ENODEV;
 	}
 
-	cpumask_copy(saved_mask, &current->cpus_allowed);
 	t_state.target_state = state;
 	p_throttling = &(pr->throttling);
-	cpumask_and(online_throttling_cpus, cpu_online_mask,
-		    p_throttling->shared_cpu_map);
+
 	/*
 	 * The throttling notifier will be called for every
 	 * affected cpu in order to get one proper T-state.
 	 * The notifier event is THROTTLING_PRECHANGE.
 	 */
-	for_each_cpu(i, online_throttling_cpus) {
+	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
 		t_state.cpu = i;
 		acpi_processor_throttling_notifier(THROTTLING_PRECHANGE,
 							&t_state);

@@ -1118,21 +1123,18 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	 * it can be called only for the cpu pointed by pr.
 	 */
 	if (p_throttling->shared_type == DOMAIN_COORD_TYPE_SW_ANY) {
-		/* FIXME: use work_on_cpu() */
-		if (set_cpus_allowed_ptr(current, cpumask_of(pr->id))) {
-			/* Can't migrate to the pr->id CPU. Exit */
-			ret = -ENODEV;
-			goto exit;
-		}
-		ret = p_throttling->acpi_processor_set_throttling(pr,
-						t_state.target_state, force);
+		arg.pr = pr;
+		arg.target_state = state;
+		arg.force = force;
+		ret = work_on_cpu(pr->id, acpi_processor_throttling_fn, &arg);
 	} else {
 		/*
 		 * When the T-state coordination is SW_ALL or HW_ALL,
 		 * it is necessary to set T-state for every affected
 		 * cpus.
 		 */
-		for_each_cpu(i, online_throttling_cpus) {
+		for_each_cpu_and(i, cpu_online_mask,
+		    p_throttling->shared_cpu_map) {
 			match_pr = per_cpu(processors, i);
 			/*
 			 * If the pointer is invalid, we will report the

@@ -1153,13 +1155,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 					"on CPU %d\n", i));
 				continue;
 			}
-			t_state.cpu = i;
-			/* FIXME: use work_on_cpu() */
-			if (set_cpus_allowed_ptr(current, cpumask_of(i)))
-				continue;
-			ret = match_pr->throttling.
-				acpi_processor_set_throttling(
-				match_pr, t_state.target_state, force);
+
+			arg.pr = match_pr;
+			arg.target_state = state;
+			arg.force = force;
+			ret = work_on_cpu(pr->id, acpi_processor_throttling_fn,
+				&arg);
 		}
 	}
 	/*

@@ -1168,17 +1169,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	 * affected cpu to update the T-states.
 	 * The notifier event is THROTTLING_POSTCHANGE
 	 */
-	for_each_cpu(i, online_throttling_cpus) {
+	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
 		t_state.cpu = i;
 		acpi_processor_throttling_notifier(THROTTLING_POSTCHANGE,
 							&t_state);
 	}
-	/* restore the previous state */
-	/* FIXME: use work_on_cpu() */
-	set_cpus_allowed_ptr(current, saved_mask);
-exit:
-	free_cpumask_var(online_throttling_cpus);
-	free_cpumask_var(saved_mask);
+
 	return ret;
 }
 

Quoted 30 lines · Lan Tianyu

acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make sure struct acpi_processor->acpi_processor_set_throttling() callback run on associated cpu. But the function maybe called in a worker which has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr() with work_on_cpu().

Signed-off-by Lan Tianyu <[email protected]>

---
 drivers/acpi/processor_throttling.c | 70 +++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 37 deletions(-)

drivers/acpi/processor_throttling.c +6 -0

index 28baa05..2db105a 100644
@@ -56,6 +56,12 @@ struct throttling_tstate {
 	int target_state;		/* target T-state */
 };
 
+struct acpi_processor_throttling_arg {
+	struct acpi_processor *pr;
+	int target_state;
+	bool force;
+};
+
 #define THROTTLING_PRECHANGE       (1)
 #define THROTTLING_POSTCHANGE      (2)
 

@@ -1060,16 +1066,25 @@ static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr,
 	return 0;

drivers/acpi/processor_throttling.c static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr, · line 1076

Quoted 10 lines

drivers/acpi/processor_throttling.c +8 -0

 }
 
+static long acpi_processor_throttling_fn(void *data)
+{
+	struct acpi_processor_throttling_arg *arg = data;
+	struct acpi_processor *pr = arg->pr;
+	struct acpi_processor_throttling *p_throttling = &pr->throttling;
+
+	return p_throttling->acpi_processor_set_throttling(pr,
+			arg->target_state, arg->force);

What about doing

	return pr->throttling.acpi_processor_set_throttling(...);

directly without using the extra p_throttling pointer?

Quoted 121 lines

drivers/acpi/processor_throttling.c +19 -37

+}
+
 int acpi_processor_set_throttling(struct acpi_processor *pr,
 						int state, bool force)
 {
-	cpumask_var_t saved_mask;
 	int ret = 0;
 	unsigned int i;
 	struct acpi_processor *match_pr;
 	struct acpi_processor_throttling *p_throttling;
+	struct acpi_processor_throttling_arg arg;
 	struct throttling_tstate t_state;
-	cpumask_var_t online_throttling_cpus;
 
 	if (!pr)
 		return -EINVAL;

@@ -1080,14 +1095,6 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	if ((state < 0) || (state > (pr->throttling.state_count - 1)))
 		return -EINVAL;
 
-	if (!alloc_cpumask_var(&saved_mask, GFP_KERNEL))
-		return -ENOMEM;
-
-	if (!alloc_cpumask_var(&online_throttling_cpus, GFP_KERNEL)) {
-		free_cpumask_var(saved_mask);
-		return -ENOMEM;
-	}
-
 	if (cpu_is_offline(pr->id)) {
 		/*
 		 * the cpu pointed by pr->id is offline. Unnecessary to change

@@ -1096,17 +1103,15 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 		return -ENODEV;
 	}
 
-	cpumask_copy(saved_mask, &current->cpus_allowed);
 	t_state.target_state = state;
 	p_throttling = &(pr->throttling);
-	cpumask_and(online_throttling_cpus, cpu_online_mask,
-		    p_throttling->shared_cpu_map);
+
 	/*
 	 * The throttling notifier will be called for every
 	 * affected cpu in order to get one proper T-state.
 	 * The notifier event is THROTTLING_PRECHANGE.
 	 */
-	for_each_cpu(i, online_throttling_cpus) {
+	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
 		t_state.cpu = i;
 		acpi_processor_throttling_notifier(THROTTLING_PRECHANGE,
 							&t_state);

@@ -1118,21 +1123,18 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	 * it can be called only for the cpu pointed by pr.
 	 */
 	if (p_throttling->shared_type == DOMAIN_COORD_TYPE_SW_ANY) {
-		/* FIXME: use work_on_cpu() */
-		if (set_cpus_allowed_ptr(current, cpumask_of(pr->id))) {
-			/* Can't migrate to the pr->id CPU. Exit */
-			ret = -ENODEV;
-			goto exit;
-		}
-		ret = p_throttling->acpi_processor_set_throttling(pr,
-						t_state.target_state, force);
+		arg.pr = pr;
+		arg.target_state = state;
+		arg.force = force;
+		ret = work_on_cpu(pr->id, acpi_processor_throttling_fn, &arg);
 	} else {
 		/*
 		 * When the T-state coordination is SW_ALL or HW_ALL,
 		 * it is necessary to set T-state for every affected
 		 * cpus.
 		 */
-		for_each_cpu(i, online_throttling_cpus) {
+		for_each_cpu_and(i, cpu_online_mask,
+		    p_throttling->shared_cpu_map) {
 			match_pr = per_cpu(processors, i);
 			/*
 			 * If the pointer is invalid, we will report the

@@ -1153,13 +1155,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 					"on CPU %d\n", i));
 				continue;
 			}
-			t_state.cpu = i;
-			/* FIXME: use work_on_cpu() */
-			if (set_cpus_allowed_ptr(current, cpumask_of(i)))
-				continue;
-			ret = match_pr->throttling.
-				acpi_processor_set_throttling(
-				match_pr, t_state.target_state, force);
+
+			arg.pr = match_pr;
+			arg.target_state = state;
+			arg.force = force;
+			ret = work_on_cpu(pr->id, acpi_processor_throttling_fn,
+				&arg);
 		}
 	}
 	/*

@@ -1168,17 +1169,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	 * affected cpu to update the T-states.
 	 * The notifier event is THROTTLING_POSTCHANGE
 	 */
-	for_each_cpu(i, online_throttling_cpus) {
+	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
 		t_state.cpu = i;
 		acpi_processor_throttling_notifier(THROTTLING_POSTCHANGE,
 							&t_state);
 	}
-	/* restore the previous state */
-	/* FIXME: use work_on_cpu() */
-	set_cpus_allowed_ptr(current, saved_mask);
-exit:
-	free_cpumask_var(online_throttling_cpus);
-	free_cpumask_var(saved_mask);
+
 	return ret;
 }