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

[Oleg Nesterov <oleg@redhat.com>]

Michal,

I am already sleeping, I'll try to reply to other parts of your email
(and other emails) tomorrow, just some notes about the patch you propose.

And cough sorry for noise... I personally hate-hate-hate every new "oom"
member you and Tetsuo add into task/signal_struct ;) But not in this case,
because I _think_ we need signal_struct->mm anyway in the long term.

So at first glance this patch makes sense, but unless I missed something
(the patch doesn't apply I can be easily wrong),

On 06/28, Michal Hocko wrote:
>
> @@ -245,6 +245,8 @@ static inline void free_signal_struct(struct signal_struct *sig)
>  {
>  	taskstats_tgid_free(sig);
>  	sched_autogroup_exit(sig);
> +	if (sig->oom_mm)
> +		mmdrop(sig->oom_mm);
>  	kmem_cache_free(signal_cachep, sig);
>  }

OK, iiuc this is not that bad because only oom-killer can set it,

> +void mark_oom_victim(struct task_struct *tsk, struct mm_struct *mm)
>  {
>  	WARN_ON(oom_killer_disabled);
>  	/* OOM killer might race with memcg OOM */
>  	if (test_and_set_tsk_thread_flag(tsk, TIF_MEMDIE))
>  		return;
> +
>  	atomic_inc(&tsk->signal->oom_victims);
> +
> +	/* oom_mm is bound to the signal struct life time */
> +	if (!tsk->signal->oom_mm) {
> +		atomic_inc(&mm->mm_count);
> +		tsk->signal->oom_mm = mm;

Looks racy, but it is not because we rely on oom_lock? Perhaps a comment
makes sense.

> @@ -828,7 +816,7 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	struct task_struct *victim = p;
>  	struct task_struct *child;
>  	struct task_struct *t;
> -	struct mm_struct *mm;
> +	struct mm_struct *mm = READ_ONCE(p->mm);
>  	unsigned int victim_points = 0;
>  	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
>  					      DEFAULT_RATELIMIT_BURST);
> @@ -838,8 +826,8 @@ void oom_kill_process(struct oom_control *oc, struct task_struct *p,
>  	 * If the task is already exiting, don't alarm the sysadmin or kill
>  	 * its children or threads, just set TIF_MEMDIE so it can die quickly
>  	 */
> -	if (task_will_free_mem(p)) {
> -		mark_oom_victim(p);
> +	if (mm && task_will_free_mem(p)) {
> +		mark_oom_victim(p, mm);

And this looks really racy at first glance. Suppose that this memory hog execs
(this changes its ->mm) and then exits so that task_will_free_mem() == T, in
this case "mm" has nothing to do with tsk->mm and it can be already freed.

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>