Re: [RFC PATCH] oom: Don't count on mm-less current process.

Tetsuo Handa
2014-12-23T01:52:29Z

Michal Hocko wrote:
> OOM killer tries to exlude tasks which do not have mm_struct associated
s/exlude/exclude/

> Fix this by checking task->mm and setting TIF_MEMDIE flag under task_lock
> which will serialize the OOM killer with exit_mm which sets task->mm to
> NULL.
Nice idea.

By the way, find_lock_task_mm(victim) may succeed if victim->mm == NULL and
one of threads in victim thread-group has non-NULL mm. That case is handled
by victim != p branch below. But where was p->signal->oom_score_adj !=
OOM_SCORE_ADJ_MIN checked? (In other words, don't we need to check like
t->mm && t->signal->oom_score_adj != OOM_SCORE_ADJ_MIN at find_lock_task_mm()
for OOM-kill case?)

Also, why not to call set_tsk_thread_flag() and do_send_sig_info() together
like below

p = find_lock_task_mm(victim);
if (!p) {
put_task_struct(victim);
return;
} else if (victim != p) {
get_task_struct(p);
put_task_struct(victim);
victim = p;
}

/* mm cannot safely be dereferenced after task_unlock(victim) */
mm = victim->mm;
+ set_tsk_thread_flag(victim, TIF_MEMDIE);
+ do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
pr_err("Killed process %d (%s) total-vm:%lukB, anon-rss:%lukB, file-rss:%lukB\n",
task_pid_nr(victim), victim->comm, K(victim->mm->total_vm),
K(get_mm_counter(victim->mm, MM_ANONPAGES)),
K(get_mm_counter(victim->mm, MM_FILEPAGES)));
task_unlock(victim);

than wait for for_each_process() loop in case current task went to sleep
immediately after task_unlock(victim)? Or is there a reason we had been
setting TIF_MEMDIE after the for_each_process() loop? If the reason was
to minimize the duration of OOM killer being disabled due to TIF_MEMDIE,
shouldn't we do like below?

rcu_read_unlock();

- set_tsk_thread_flag(victim, TIF_MEMDIE);
+ task_lock(victim);
+ if (victim->mm)
+ set_tsk_thread_flag(victim, TIF_MEMDIE);
+ task_unlock(victim);
do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
put_task_struct(victim);