Re: Multiple oom_reaper BUGs: unmap_page_range racing with exit_mmap

[Michal Hocko]

On Tue 05-12-17 18:43:48, David Rientjes wrote:
> Hi,
> 
> I'd like to understand the synchronization between the oom_reaper's 
> unmap_page_range() and exit_mmap().  The latter does not hold 
> mm->mmap_sem: it's supposed to be the last thread operating on the mm 
> before it is destroyed.
> 
> If unmap_page_range() races with unmap_vmas(), we trivially call 
> page_remove_rmap() twice on the same page:

Well, the oom reaper is basically MADV_DONTNEED and that allows
parallel tear down (it takes only mmap_sem for read). The exit path
doesn't take the mmap_sem during unmap_vmas but that shouldn't make any
difference because both path would take it for read anyway. The
essential synchronization between oom reaper and exit_mmap is
exit_mmap:	
	set_bit(MMF_OOM_SKIP, &mm->flags);
	if (unlikely(tsk_is_oom_victim(current))) {
		/*
		 * Wait for oom_reap_task() to stop working on this
		 * mm. Because MMF_OOM_SKIP is already set before
		 * calling down_read(), oom_reap_task() will not run
		 * on this "mm" post up_write().
		 *
		 * tsk_is_oom_victim() cannot be set from under us
		 * either because current->mm is already set to NULL
		 * under task_lock before calling mmput and oom_mm is
		 * set not NULL by the OOM killer only if current->mm
		 * is found not NULL while holding the task_lock.
		 */
		down_write(&mm->mmap_sem);
		up_write(&mm->mmap_sem);
	}

oom_reaper
	if (!down_read_trylock(&mm->mmap_sem)) {

	/*
	 * MMF_OOM_SKIP is set by exit_mmap when the OOM reaper can't
	 * work on the mm anymore. The check for MMF_OOM_SKIP must run
	 * under mmap_sem for reading because it serializes against the
	 * down_write();up_write() cycle in exit_mmap().
	 */
	if (test_bit(MMF_OOM_SKIP, &mm->flags)) {

which makes sure that the reaper doesn't race with free_pgtables.

> BUG: Bad page map in process oom_reaper  pte:6353826300000000 pmd:00000000

Hmm, this is really strange. This is a pte without a pmd or is the
output just incomplete.

> addr:00007f50cab1d000 vm_flags:08100073 anon_vma:ffff9eea335603f0 mapping:          (null) index:7f50cab1d
> file:          (null) fault:          (null) mmap:          (null) readpage:          (null)
> CPU: 2 PID: 1001 Comm: oom_reaper
> Call Trace:
>  [<ffffffffa4bd967d>] dump_stack+0x4d/0x70
>  [<ffffffffa4a03558>] unmap_page_range+0x1068/0x1130

could you use addr2line to get the exact spot where this triggered
please?

>  [<ffffffffa4a2e07f>] __oom_reap_task_mm+0xd5/0x16b
>  [<ffffffffa4a2e226>] oom_reaper+0xff/0x14c
>  [<ffffffffa48d6ad1>] kthread+0xc1/0xe0
> 
> And there are more examples of badness from an unmap_page_range() racing 
> with unmap_vmas().  In this case, MMF_OOM_SKIP is doing two things: (1) 
> avoiding additional oom kills until unmap_vmas() returns and (2) avoid the 
> oom_reaper working on the mm after unmap_vmas().  In (2), there's nothing 
> preventing the oom reaper from calling unmap_page_range() in parallel with 
> the final thread doing unmap_vmas() -- we no longer do mmget() to prevent 
> exit_mmap() from being called.

Yes and that is an intentional behavior. There shouldn't be any reason
to exclude the two because this should be equivalent to calling
MADV_DONTNEED in parallel.

I will get to the rest of your email later because the above is the
essential assumption 212925802454 ("mm: oom: let oom_reap_task and
exit_mmap run concurrently") builds on. If it is not correct then we
have a bigger problem.
-- 
Michal Hocko
SUSE Labs