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

Michal Hocko
2014-12-23T12:24:04Z

On Tue 23-12-14 20:46:07, Tetsuo Handa wrote:
> Michal Hocko wrote:
> > > Also, why not to call set_tsk_thread_flag() and do_send_sig_info() together
> > > like below
> >
> > What would be an advantage? I am not really sure whether the two locks
> > might nest as well.
>
> I imagined that current thread sets TIF_MEMDIE on a victim thread, then
> sleeps for 30 seconds immediately after task_unlock() (it's an overdone
> delay),

Only if the current task was preempted for such a long time. Which
doesn't sound too probable to me.

> and finally sets SIGKILL on that victim thread. If such a delay
> happened, that victim thread is free to abuse TIF_MEMDIE for that period.
> Thus, I thought sending SIGKILL followed by setting TIF_MEMDIE is better.

I don't know, I can hardly find a scenario where it would make any
difference in the real life. If the victim needs to allocate a memory to
finish then it would trigger OOM again and have to wait/loop until this
OOM killer releases the oom zonelist lock just to find out it already
has TIF_MEMDIE set and can dive into memory reserves. Which way is more
correct is a question but I wouldn't change it without having a really
good reason. This whole code is subtle already, let's not make it even
more so.

>
> rcu_read_unlock();
>
> - set_tsk_thread_flag(victim, TIF_MEMDIE);
> do_send_sig_info(SIGKILL, SEND_SIG_FORCED, victim, true);
> + task_lock(victim);
> + if (victim->mm)
> + set_tsk_thread_flag(victim, TIF_MEMDIE);
> + task_unlock(victim);
> put_task_struct(victim);
>
> If such a delay is theoretically impossible, I'm OK with your patch.
--
Michal Hocko
SUSE Labs