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
