Re: [patch] oom: prevent unnecessary oom kills or kernel panics

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

Hi

> This patch revents unnecessary oom kills or kernel panics by reverting
> two commits:
> 
> 	495789a5 (oom: make oom_score to per-process value)
> 	cef1d352 (oom: multi threaded process coredump don't make deadlock)
> 
> First, 495789a5 (oom: make oom_score to per-process value) ignores the
> fact that all threads in a thread group do not necessarily exit at the
> same time.
> 
> It is imperative that select_bad_process() detect threads that are in the
> exit path, specifically those with PF_EXITING set, to prevent needlessly
> killing additional tasks.  

to prevent? No, it is not a reason of PF_EXITING exist.


> If a process is oom killed and the thread
> group leader exits, select_bad_process() cannot detect the other threads
> that are PF_EXITING by iterating over only processes.  Thus, it currently
> chooses another task unnecessarily for oom kill or panics the machine
> when nothing else is eligible.
> 
> By iterating over threads instead, it is possible to detect threads that
> are exiting and nominate them for oom kill so they get access to memory
> reserves.

In fact, PF_EXITING is a sing of *THREAD* exiting, not process. Therefore
PF_EXITING is not a sign of memory freeing in nearly future. If other
CPUs don't try to free memory, prevent oom and waiting makes deadlock.

Thus, I suggest to remove PF_EXITING check completely.

> 
> Second, cef1d352 (oom: multi threaded process coredump don't make
> deadlock) erroneously avoids making the oom killer a no-op when an
> eligible thread other than current isfound to be exiting.  We want to
> detect this situation so that we may allow that exiting thread time to
> exit and free its memory; if it is able to exit on its own, that should
> free memory so current is no loner oom.  If it is not able to exit on its
> own, the oom killer will nominate it for oom kill which, in this case,
> only means it will get access to memory reserves.
> 
> Without this change, it is easy for the oom killer to unnecessarily
> target tasks when all threads of a victim don't exit before the thread
> group leader or, in the worst case, panic the machine.
> 

You missed deadlock is more worse than panic. And again, task overkill
is a part of OOM killer design. it is necessary to avoid deadlock. If
you want to change this spec, you need to remove deadlock change at first.


> Signed-off-by: David Rientjes <rientjes@google.com>
> ---
>  mm/oom_kill.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -292,11 +292,11 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>  		unsigned long totalpages, struct mem_cgroup *mem,
>  		const nodemask_t *nodemask)
>  {
> -	struct task_struct *p;
> +	struct task_struct *g, *p;
>  	struct task_struct *chosen = NULL;
>  	*ppoints = 0;
>  
> -	for_each_process(p) {
> +	do_each_thread(g, p) {
>  		unsigned int points;
>  
>  		if (oom_unkillable_task(p, mem, nodemask))
> @@ -324,7 +324,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>  		 * the process of exiting and releasing its resources.
>  		 * Otherwise we could get an easy OOM deadlock.
>  		 */
> -		if (thread_group_empty(p) && (p->flags & PF_EXITING) && p->mm) {
> +		if ((p->flags & PF_EXITING) && p->mm) {
>
>  			if (p != current)
>  				return ERR_PTR(-1UL);
>  
> @@ -337,7 +337,7 @@ static struct task_struct *select_bad_process(unsigned int *ppoints,
>  			chosen = p;
>  			*ppoints = points;
>  		}
> -	}
> +	} while_each_thread(g, p);
>  
>  	return chosen;
>  }



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>