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

[Lai Jiangshan <laijs@cn.fujitsu.com>]

On 06/03/2014 10:16 PM, Peter Zijlstra wrote:
> 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..

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

Any way, I agree on the change.
I hope Jason test it in few days.

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