Re: [PATCH v2] mm, oom: don't set TIF_MEMDIE on a mm-less thread.

[Oleg Nesterov <oleg@redhat.com>]

On 06/24, Tetsuo Handa wrote:
>
> On CONFIG_MMU=n kernels, nothing
> will clear TIF_MEMDIE and the system can OOM livelock if TIF_MEMDIE was
> by error set to a mm-less thread group leader.

and btw this needs more cleanups imo. I mean, the fact we pass task_struct
to wake_oom_reaper() looks, well, strange. But this is off-topic right now.

> @@ -839,9 +839,19 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	 * its children or threads, just set TIF_MEMDIE so it can die quickly
>  	 */
>  	if (task_will_free_mem(p)) {
> -		mark_oom_victim(p);
> -		wake_oom_reaper(p);
> -		put_task_struct(p);
> +		p = find_lock_task_mm(victim);
> +		if (!p) {

Well, this doesn't really matter... but imo

		victim = find_lock_task_mm(p);
		if (!victim) {

will look much more readable, and this way we won't depend on the
early "victim = p" initialization at the start.

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

Tetsuo but this is horrible ;)

At least this needs a comment to explain _why_. Because this looks
"obviously unnecessary"; exit_oom_victim() does find_lock_task_mm()
too and "p" can exit right after we drop task_lock(). Not to mention
that task_will_free_mem() called find_lock_task_mm() right before
that, so this is sub-optimal in any case.

IOW, this should explain that we only need this for mark_oom_victim(),
and only if CONFIG_MMU=n. And this leads to other questions:

	- Why we can livelock in this case? This should be documented
	  too imo,

	  I have to admit I don't understand why. But yes, yes, sorry,
	  I ignored a lot of emails in this area :/

	- Why mark_oom_victim() can't check ->mm != NULL itself?

Oleg.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>