Re: [PATCH] mm: mempolicy: don't select exited threads as OOM victims

[Michal Hocko <mhocko@kernel.org>]

On Tue 02-07-19 22:19:27, Tetsuo Handa wrote:
> On 2019/07/01 23:16, Michal Hocko wrote:
> > Thinking about it some more it seems that we can go with your original
> > fix if we also reorder oom_evaluate_task
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index f719b64741d6..e5feb0f72e3b 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -318,9 +318,6 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
> >  	struct oom_control *oc = arg;
> >  	unsigned long points;
> >  
> > -	if (oom_unkillable_task(task, NULL, oc->nodemask))
> > -		goto next;
> > -
> >  	/*
> >  	 * This task already has access to memory reserves and is being killed.
> >  	 * Don't allow any other task to have access to the reserves unless
> > @@ -333,6 +330,9 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
> >  		goto abort;
> >  	}
> >  
> > +	if (oom_unkillable_task(task, NULL, oc->nodemask))
> > +		goto next;
> > +
> >  	/*
> >  	 * If task is allocating a lot of memory and has been marked to be
> >  	 * killed first if it triggers an oom, then select it.
> > 
> > I do not see any strong reason to keep the current ordering. OOM victim
> > check is trivial so it shouldn't add a visible overhead for few
> > unkillable tasks that we might encounter.
> > 
> 
> Yes if we can tolerate that there can be only one OOM victim for !memcg OOM events
> (because an OOM victim in a different OOM context will hit "goto abort;" path).

You are right. Considering that we now have a guarantee of a forward
progress then this should be tolerateable (a victim in a disjoint
numaset will go away and other one can go ahead and trigger its own
OOM).
 
> Thinking again, I think that the same problem exists for mask == NULL path
> as long as "a process with dying leader and live threads" is possible. Then,
> fixing up after has_intersects_mems_allowed()/cpuset_mems_allowed_intersects()
> judged that some thread is eligible is better.

This is getting more and more hair for something that is not really
clear to be an actual problem. Don't you think?

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index d1c9c4e..43e499e 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -109,8 +109,23 @@ static bool oom_cpuset_eligible(struct task_struct *start,
>  			 */
>  			ret = cpuset_mems_allowed_intersects(current, tsk);
>  		}
> -		if (ret)
> -			break;
> +		if (ret) {
> +			/*
> +			 * Exclude dead threads as ineligible when selecting
> +			 * an OOM victim. But include dead threads as eligible
> +			 * when waiting for OOM victims to get MMF_OOM_SKIP.
> +			 *
> +			 * Strictly speaking, tsk->mm should be checked under
> +			 * task lock because cpuset_mems_allowed_intersects()
> +			 * does not take task lock. But racing with exit_mm()
> +			 * is not fatal. Thus, use cheaper barrier rather than
> +			 * strict task lock.
> +			 */
> +			smp_rmb();
> +			if (tsk->mm || tsk_is_oom_victim(tsk))
> +				break;
> +			ret = false;
> +		}
>  	}
>  	rcu_read_unlock();
>  

-- 
Michal Hocko
SUSE Labs