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

[Peter Zijlstra]

On Tue, Jun 03, 2014 at 07:24:38PM +0800, Lai Jiangshan wrote:
> 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.

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(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 240aa83e73f5..0708ee21632f 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -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);

Attachment: pgpP745F0OSQr.pgp
Description: PGP signature