Re: [PATCH] mm: memcontrol: don't miss processes whose leader has exited in OOM scans

[Michal Hocko]

On Mon 26-10-26 20:02:48, Zhan Xusheng wrote:
> From: Zhan Xusheng <zhanxusheng@xxxxxxxxxx>
> 
> mem_cgroup_scan_tasks() walks with CSS_TASK_ITER_PROCS, so it sees only
> thread group leaders, plus a dying leader that still has live threads.
> Moving a process whose leader has exited, by writing its PID to
> cgroup.procs, moves the live threads but leaves the leader behind, as
> cgroup_migrate_add_task() skips exiting tasks.

Is this a desirable/intended behavior? Shouldn't we move the dead leader
along with the rest?

> The destination's OOM killer then cannot see the process.  If it is
> alone there, every attempt ends with "Out of memory and no killable
> processes..." and the charge is retried indefinitely, and
> memory.oom.group kills nothing; otherwise another process is killed in
> its place.  The source's OOM killer still finds the leader, reaches the
> moved threads' mm through find_lock_task_mm(), and kills a process that
> has left the memcg.

While you patch works around this seems like something to address in the
cgroup core. Should CSS_TASK_ITER_PROCS iteration see all processes
regardless leader being alive after or prior the migration? AFAIU the
thread group belongs to the new cgroup in both cases.

> 
> @@ -1452,9 +1482,10 @@ void mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
>  		struct css_task_iter it;
>  		struct task_struct *task;
>  
> -		css_task_iter_start(&iter->css, CSS_TASK_ITER_PROCS, &it);
> +		css_task_iter_start(&iter->css, 0, &it);

Shouldn't this be CSS_TASK_ITER_THREADED instead of 0?

>  		while (!ret && (task = css_task_iter_next(&it))) {
> -			ret = fn(task, arg);
> +			if (!mem_cgroup_scan_skip(task))
> +				ret = fn(task, arg);
>  			/* Avoid potential softlockup warning */
>  			cond_resched();
>  		}
> 
> base-commit: 6b4b543fa18757297ac756a6fe6e64f506b7844a
> -- 
> 2.43.0

-- 
Michal Hocko
SUSE Labs