Re: [PATCH] mm/oom: Suppress unnecessary "sharing same memory" message.

[Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>]

Michal Hocko wrote:
> On Sat 30-05-15 02:20:23, Tetsuo Handa wrote:
> > Michal Hocko wrote:
> > > On Fri 29-05-15 21:40:47, Tetsuo Handa wrote:
> [...]
> > > > Is it possible that thread1 is doing memory allocation between
> > > > down_write(&current->mm->mmap_sem) and up_write(&current->mm->mmap_sem),
> > > > thread2 sharing the same mm is waiting at down_read(&current->mm->mmap_sem),
> > > > and the OOM killer invoked by thread3 chooses thread2 as the OOM victim and
> > > > sets TIF_MEMDIE to thread2?
> > > 
> > > Your usage of thread is confusing. Threads are of no concerns because
> > > those get killed when the group leader is killed. If you refer to
> > > processes then this is exactly what is handled by:
> > >         for_each_process(p)
> > >                 if (p->mm == mm && !same_thread_group(p, victim) &&
> > >                     !(p->flags & PF_KTHREAD)) {
> > >                         if (p->signal->oom_score_adj == OOM_SCORE_ADJ_MIN)
> > >                                 continue;
> > > 
> > >                         task_lock(p);   /* Protect ->comm from prctl() */
> > >                         pr_err("Kill process %d (%s) sharing same memory\n",
> > >                                 task_pid_nr(p), p->comm);
> > >                         task_unlock(p);
> > >                         do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
> > >                 }
> > 
> > I refer to both "Thread-1 in process-1, thread-2 in process-1" case and
> > "thread-1 in process-1, thread-2 in process-2" case. Thread-3 can be in
> > process-1 or process-2 or neither.
> 
> And that makes it confusing because threads in the same thread group
> case is not really interesting. All the threads have fatal signal
> pending and they would get access to memory reserves as they hit the oom
> killer.

Excuse me, but I didn't understand it.

TIF_MEMDIE is per a "struct task_struct" attribute which is set on
its corresponding "struct thread_info"->flags member, isn't it?
Two "struct task_struct" can't share the same "struct thread_info"->flags
member, can it?

And the condition which we allow access to memory reserves is not
"whether SIGKILL is pending or not" but "whether TIF_MEMDIE is set or not",
doesn't it?

----------
static inline int
gfp_to_alloc_flags(gfp_t gfp_mask)
{
(...snipped...)
	if (likely(!(gfp_mask & __GFP_NOMEMALLOC))) {
		if (gfp_mask & __GFP_MEMALLOC)
			alloc_flags |= ALLOC_NO_WATERMARKS;
		else if (in_serving_softirq() && (current->flags & PF_MEMALLOC))
			alloc_flags |= ALLOC_NO_WATERMARKS;
		else if (!in_interrupt() &&
			 ((current->flags & PF_MEMALLOC) ||
			  unlikely(test_thread_flag(TIF_MEMDIE))))
			alloc_flags |= ALLOC_NO_WATERMARKS;
	}
(...snipped...)
}
----------

How can all fatal_signal_pending() "struct task_struct" get access to memory
reserves when only one of fatal_signal_pending() "struct task_struct" has
TIF_MEMDIE ?

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