Re: BUG: HANG_DETECT waiting for migration_cpu_stop() complete

[Hillf Danton <hdanton@sina.com>]

On 22 Sep 2022 13:40:47 +0800 Jing-Ting Wu <jing-ting.wu@mediatek.com> wrote
> 
> Because Peter have some concern for patch by Waiman.
> We add Hillf's patch to our stability test.
> But there are side effects after patched.
> The warning appear once < two weeks. 

Thanks for your test. Any other effects observed?

> 
> Backtrace as follows:
> [name:panic&]WARNING: CPU: 6 PID: 32583 at affine_move_task
> pc : affine_move_task
> lr : __set_cpus_allowed_ptr_locked
> Call trace:
> affine_move_task
> __set_cpus_allowed_ptr_locked
> migrate_enable
> __cgroup_bpf_run_filter_skb
> ip_finish_output
> ip_output
> 
> 
> The root cause is when is_migration_disabled(p) is true，the patched
> version will set p->migration_pending to NULL by migration_cpu_stop.
> And in affine_move_task will raise a WARN_ON_ONCE(!pending).
> 
> Kernel-5.15/kernel/sched/core.c:
> static int affine_move_task(struct rq *rq, struct task_struct *p,
> struct rq_flags *rf, int dest_cpu, unsigned int flags) {
> ...
> 	If (WARN_ON_ONCE(!pending)) {
>  	  Task_rq_unlock(rq,p,fr);
>   	  return -EINVAL;
> 	}
> ...
> }
> 
> But the tasks have not been migrated to the new affinity CPU, so there
> should be pending tasks to be processed, so p->migration_pending should
> not be NULL.
> 
> 
> 
> Without patch:
> When is_migration_disabled is true, then goto out and not set p-
> >migration_pending to NULL.
> 
> static int migration_cpu_stop(void *data) {
> ...
> 	If (task_rq(p) == rq) {
>         	     if (is_migration_disabled(p))
>                 	           goto out;
> 	}
> ...
> }
> 
> 
> With patch:
> When is_migration_disabled is true and pending is true, goto else if
> flow. Because p->cpus_ptr not updated when migrate_disable, so this
> condition is always true and p->migration_pending will set to NULL.
> 
> static int migration_cpu_stop(void *data) {
> ...
> 	If (task_rq(p) == rq && !is_migration_disabled(p) ) {
>  	  ...
> 	} else if (pending) {
> 	  ...
		/*
		 * The task moved before the stopper got to run. We're holding
		 * ->pi_lock, so the allowed mask is stable - if it got
		 * somewhere allowed, we're done.
		 */
> 	  If (cpumask_test_cpu(task_cpu(p), p-> cpus_ ptr)) { 
>         	p->migration_pending = NULL;
>       		 complete = true;
>       		 goto out;
> 	}
> ...
> }

Given p->migration_pending reset in case of job done, the warning you saw
is benign without other negative effects observed.

It should be fixed (example by simply cutting it) if you have a reproducer
on top of the mainline tree.

Hillf