On Mon, Oct 22, 2018 at 09:13:23AM +0200, Michal Hocko wrote: > From: Michal Hocko <mhocko@xxxxxxxx> > > Tetsuo has reported [1] that a single process group memcg might easily > swamp the log with no-eligible oom victim reports due to race between > the memcg charge and oom_reaper > > Thread 1 Thread2 oom_reaper > try_charge try_charge > mem_cgroup_out_of_memory > mutex_lock(oom_lock) > mem_cgroup_out_of_memory > mutex_lock(oom_lock) > out_of_memory > select_bad_process > oom_kill_process(current) > wake_oom_reaper > oom_reap_task > MMF_OOM_SKIP->victim > mutex_unlock(oom_lock) > out_of_memory > select_bad_process # no task > > If Thread1 didn't race it would bail out from try_charge and force the > charge. We can achieve the same by checking tsk_is_oom_victim inside > the oom_lock and therefore close the race. > > [1] http://lkml.kernel.org/r/bb2074c0-34fe-8c2c-1c7d-db71338f1e7f@xxxxxxxxxxxxxxxxxxx > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx> > --- > mm/memcontrol.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index e79cb59552d9..a9dfed29967b 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1380,10 +1380,22 @@ static bool mem_cgroup_out_of_memory(struct mem_cgroup *memcg, gfp_t gfp_mask, > .gfp_mask = gfp_mask, > .order = order, > }; > - bool ret; > + bool ret = true; > > mutex_lock(&oom_lock); > + > + /* > + * multi-threaded tasks might race with oom_reaper and gain > + * MMF_OOM_SKIP before reaching out_of_memory which can lead > + * to out_of_memory failure if the task is the last one in > + * memcg which would be a false possitive failure reported > + */ > + if (tsk_is_oom_victim(current)) > + goto unlock; > + > ret = out_of_memory(&oc); We already check tsk_is_oom_victim(current) in try_charge() before looping on the OOM killer, so at most we'd have a single "no eligible tasks" message from such a race before we force the charge - right? While that's not perfect, I don't think it warrants complicating this code even more. I honestly find it near-impossible to follow the code and the possible scenarios at this point. out_of_memory() bails on task_will_free_mem(current), which specifically *excludes* already reaped tasks. Why are we then adding a separate check before that to bail on already reaped victims? Do we want to bail if current is a reaped victim or not? I don't see how we could skip it safely in general: the current task might have been killed and reaped and gotten access to the memory reserve and still fail to allocate on its way out. It needs to kill the next task if there is one, or warn if there isn't another one. Because we're genuinely oom without reclaimable tasks. There is of course the scenario brought forward in this thread, where multiple threads of a process race and the second one enters oom even though it doesn't need to anymore. What the global case does to catch this is to grab the oom lock and do one last alloc attempt. Should memcg lock the oom_lock and try one more time to charge the memcg? Some simplification in this area would really be great. I'm reluctant to ack patches like the above, even if they have some optical benefits for the user, because the code is already too tricky for what it does.
