Re: [PATCH] mm,oom: use per signal_struct flag rather than clear TIF_MEMDIE

[Oleg Nesterov <oleg@redhat.com>]

On 06/27, Michal Hocko wrote:
>
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -237,6 +237,8 @@ void free_task(struct task_struct *tsk)
>  	ftrace_graph_exit_task(tsk);
>  	put_seccomp_filter(tsk);
>  	arch_release_task_struct(tsk);
> +	if (tsk->active_mm)
> +		mmdrop(tsk->active_mm);
>  	free_task_struct(tsk);
>  }
>  EXPORT_SYMBOL(free_task);
> @@ -1022,6 +1024,8 @@ static int copy_mm(unsigned long clone_flags, struct task_struct *tsk)
>  good_mm:
>  	tsk->mm = mm;
>  	tsk->active_mm = mm;
> +	/* to be release in the final task_put */
> +	atomic_inc(&mm->mm_count);
>  	return 0;

No, I don't think this can work.

Note that tsk->active_mm in free_task() points to the random mm "borrowed"
from the previous/random task in context_switch() if task->mm == NULL. This
is true for kthreads and for the task which has already called exit_mm().

> -	p = find_lock_task_mm(tsk);
> -	if (!p)
> -		goto unlock_oom;
> -	mm = p->mm;
> +	task_lock(tsk);
> +	mm = tsk->active_mm;

The same. We can't know where this ->active_mm points to.

Just suppose that this tsk schedules after exit_mm(). When it gets CPU
again tsk->active_mm will point to ->mm of another task which in turns
called schedule() to make this tsk active.

Yes I agree, it would be nice to remove find_lock_task_mm(). And in
fact it would be nice to kill task_struct->mm (but this needs a lot
of cleanups). We probably want signal_struct->mm, but this is a bit
complicated (locking).

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>