Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1694495 > unrolled thread

[PATCH] mm, oom: allow oom reaper to race with exit_mmap

Back to article view | Back to linux.kernel

Contents

  [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-24 09:30 +0200
    Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-24 16:10 +0200
      Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-24 16:20 +0200
        Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-24 17:00 +0200
          Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-24 18:20 +0200
            Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-25 16:20 +0200
              Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 16:30 +0200
                Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-25 17:10 +0200
                  Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 17:20 +0200
            Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 16:30 +0200
              Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-25 17:20 +0200
                Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 17:30 +0200
                  Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap "Kirill A. Shutemov" <kirill@shutemov.name> - 2017-07-25 17:40 +0200
                    Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 18:10 +0200
                      Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-25 23:30 +0200
                        Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-26 07:50 +0200
                          Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-26 18:30 +0200
                            Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-26 18:50 +0200
                            Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-27 09:00 +0200
                              Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-27 17:00 +0200
                                Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-28 08:30 +0200
    Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-24 17:30 +0200
    Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-24 20:20 +0200
    Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-25 17:30 +0200
      Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-25 17:50 +0200
        Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-25 20:30 +0200
          Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-26 07:50 +0200
            Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Andrea Arcangeli <aarcange@redhat.com> - 2017-07-26 18:40 +0200
              Re: [PATCH] mm, oom: allow oom reaper to race with exit_mmap Michal Hocko <mhocko@kernel.org> - 2017-07-27 08:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1698494

On Thu 27-07-17 16:55:59, Andrea Arcangeli wrote:
> On Thu, Jul 27, 2017 at 08:50:24AM +0200, Michal Hocko wrote:
> > Yes this will work and it won't depend on the oom_lock. But isn't it
> > just more ugly than simply doing
> > 
> > 	if (tsk_is_oom_victim) {
> > 		down_write(&mm->mmap_sem);
> > 		locked = true;
> > 	}
> > 	free_pgtables(...)
> > 	[...]
> > 	if (locked)
> > 		down_up(&mm->mmap_sem);
> 
> To me not doing if (tsk_is_oom...) { down_write; up_write } is by
> default a confusing implementation, because it's not strict and not
> strict code is not self documenting and you've to think twice of why
> you're doing something the way you're doing it.

I disagree. But this is a matter of taste, I guess. I prefer when
locking is explicit and clear about the scope. An empty locked region
used for synchronization is just obscure and you have to think much more
what it means when you can see what the lock actually protects. It is
also less error prone I believe.

> The doubt on what was the point to hold the mmap_sem during
> free_pgtables is precisely why I started digging into this issue
> because it didn't look possible you could truly benefit from holding
> the mmap_sem during free_pgtables.

The point is that you cannot remove page tables while somebody is
walking them. This is enforced in all other paths except for exit which
was kind of special because nobody could race there.

> I also don't like having a new invariant that your solution relies on,
> that is mm->mmap = NULL, when we can make just set the MMF_OOM_SKIP a
> bit earlier that it gets set anyway and use that to control the other
> side of the race.

Well, again a matter of taste. I prefer making it clear that mm->mmap is
no longer valid because it points to a freed pointer. Relying on
MMF_OOM_SKIP for the handshake is just abusing the flag for a different
purpose than it was intended.
 
> I like strict code that uses as fewer invariants as possible and that
> never holds a lock for any instruction more than it is required (again
> purely for self documenting reasons, the CPU won't notice much one
> instruction more or less).
> 
> Even with your patch the two branches are unnecessary, that may not be
> measurable, but it's still wasted CPU. It's all about setting mm->mmap
> before the up_write. In fact my patch should at least put an incremental
> unlikely around my single branch added to exit_mmap.
>
> I see the {down_write;up_write} Hugh's ksm_exit-like as a strict
> solution to this issue and I wrote it specifically while trying to
> research a way to be more strict because from the start it didn't look
> the holding of the mmap_sem during free_pgtables was necessary.

While we have discussed that with Hugh he has shown an interest to
actually get rid of this (peculiar) construct which would be possible if
the down_write was implicit in exit_mmap [1].

> I'm also fine to drop the oom_lock but I think it can be done
> incrementally as it's a separate issue, my second patch should allow
> for it with no adverse side effects.
>
> All I care about is the exit_mmap path because it runs too many times
> not to pay deep attention to every bit of it ;).

Well, all I care about is to fix this over-eager oom killing due to oom
reaper setting MMF_OOM_SKIP too early. That is a regression. I think we
can agree that both proposed solutions are functionally equivalent. I do
not want to push mine too hard so if you _believe_ that yours is really
better feel free to post it for inclusion.

[1] http://lkml.kernel.org/r/alpine.LSU.2.11.1707191716030.2055@eggly.anvils
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1694811

Dohh, the full conflict resolution didn't make it into the commit. The
full patch is below
---
From be69b355a56649167dce901d24c2296ef3a3f7ec Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 24 Jul 2017 09:23:32 +0200
Subject: [PATCH] mm, oom: allow oom reaper to race with exit_mmap

David has noticed that the oom killer might kill additional tasks while
the exiting oom victim hasn't terminated yet because the oom_reaper marks
the curent victim MMF_OOM_SKIP too early when mm->mm_users dropped down
to 0. The race is as follows

oom_reap_task				do_exit
					  exit_mm
  __oom_reap_task_mm
					    mmput
					      __mmput
    mmget_not_zero # fails
    						exit_mmap # frees memory
  set_bit(MMF_OOM_SKIP)

The victim is still visible to the OOM killer until it is unhashed.

Currently we try to reduce a risk of this race by taking oom_lock
and wait for out_of_memory sleep while holding the lock to give the
victim some time to exit. This is quite suboptimal approach because
there is no guarantee the victim (especially a large one) will manage
to unmap its address space and free enough memory to the particular oom
domain which needs a memory (e.g. a specific NUMA node).

Fix this problem by allowing __oom_reap_task_mm and __mmput path to
race. __oom_reap_task_mm is basically MADV_DONTNEED and that is allowed
to run in parallel with other unmappers (hence the mmap_sem for read).

The only tricky part is to exclude page tables tear down and all
operations which modify the address space in the __mmput path. exit_mmap
doesn't expect any other users so it doesn't use any locking. Nothing
really forbids us to use mmap_sem for write, though. In fact we are
already relying on this lock earlier in the __mmput path to synchronize
with ksm and khugepaged.

Take the exclusive mmap_sem when calling free_pgtables and destroying
vmas to sync with __oom_reap_task_mm which take the lock for read. All
other operations can safely race with the parallel unmap.

Changes
- bail on null mm->mmap early as per David Rientjes

Reported-by: David Rientjes <rientjes@google.com>
Fixes: 26db62f179d1 ("oom: keep mm of the killed task available")
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/mmap.c     |  7 +++++++
 mm/oom_kill.c | 47 ++++++++---------------------------------------
 2 files changed, 15 insertions(+), 39 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 24e9261bdcc0..0eeb658caa30 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2993,6 +2993,11 @@ void exit_mmap(struct mm_struct *mm)
 	/* Use -1 here to ensure all VMAs in the mm are unmapped */
 	unmap_vmas(&tlb, vma, 0, -1);
 
+	/*
+	 * oom reaper might race with exit_mmap so make sure we won't free
+	 * page tables or unmap VMAs under its feet
+	 */
+	down_write(&mm->mmap_sem);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
@@ -3005,7 +3010,9 @@ void exit_mmap(struct mm_struct *mm)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 	}
+	mm->mmap = NULL;
 	vm_unacct_memory(nr_accounted);
+	up_write(&mm->mmap_sem);
 }
 
 /* Insert vm structure into process list sorted by address
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f030c1c..b1c96e1910f2 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -470,40 +470,15 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 {
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
-	bool ret = true;
-
-	/*
-	 * We have to make sure to not race with the victim exit path
-	 * and cause premature new oom victim selection:
-	 * __oom_reap_task_mm		exit_mm
-	 *   mmget_not_zero
-	 *				  mmput
-	 *				    atomic_dec_and_test
-	 *				  exit_oom_victim
-	 *				[...]
-	 *				out_of_memory
-	 *				  select_bad_process
-	 *				    # no TIF_MEMDIE task selects new victim
-	 *  unmap_page_range # frees some memory
-	 */
-	mutex_lock(&oom_lock);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
-		ret = false;
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		return false;
 	}
 
-	/*
-	 * increase mm_users only after we know we will reap something so
-	 * that the mmput_async is called only when we have reaped something
-	 * and delayed __mmput doesn't matter that much
-	 */
-	if (!mmget_not_zero(mm)) {
-		up_read(&mm->mmap_sem);
-		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
-	}
+	/* There is nothing to reap so bail out without signs in the log */
+	if (!mm->mmap)
+		goto unlock;
 
 	trace_start_task_reaping(tsk->pid);
 
@@ -540,18 +515,12 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 			K(get_mm_counter(mm, MM_ANONPAGES)),
 			K(get_mm_counter(mm, MM_FILEPAGES)),
 			K(get_mm_counter(mm, MM_SHMEMPAGES)));
-	up_read(&mm->mmap_sem);
 
-	/*
-	 * Drop our reference but make sure the mmput slow path is called from a
-	 * different context because we shouldn't risk we get stuck there and
-	 * put the oom_reaper out of the way.
-	 */
-	mmput_async(mm);
 	trace_finish_task_reaping(tsk->pid);
-unlock_oom:
-	mutex_unlock(&oom_lock);
-	return ret;
+unlock:
+	up_read(&mm->mmap_sem);
+
+	return true;
 }
 
 #define MAX_OOM_REAP_RETRIES 10
-- 
2.13.2

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1694977

On Tue 25-07-17 00:42:05, kbuild test robot wrote:
> Hi Michal,
> 
> [auto build test ERROR on mmotm/master]
> [also build test ERROR on v4.13-rc2 next-20170724]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-oom-allow-oom-reaper-to-race-with-exit_mmap/20170724-233159
> base:   git://git.cmpxchg.org/linux-mmotm.git master
> config: x86_64-randconfig-x016-201730 (attached as .config)
> compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=x86_64 
> 
> All error/warnings (new ones prefixed by >>):
> 
>    mm/oom_kill.c: In function '__oom_reap_task_mm':
> >> mm/oom_kill.c:523:9: error: 'ret' undeclared (first use in this function)
>      return ret;
>             ^~~

Fixed by http://lkml.kernel.org/r/20170724152703.GP25221@dhcp22.suse.cz
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1695844

On Mon, Jul 24, 2017 at 09:23:32AM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> David has noticed that the oom killer might kill additional tasks while
> the exiting oom victim hasn't terminated yet because the oom_reaper marks
> the curent victim MMF_OOM_SKIP too early when mm->mm_users dropped down
> to 0. The race is as follows
> 
> oom_reap_task				do_exit
> 					  exit_mm
>   __oom_reap_task_mm
> 					    mmput
> 					      __mmput
>     mmget_not_zero # fails
>     						exit_mmap # frees memory
>   set_bit(MMF_OOM_SKIP)
> 
> The victim is still visible to the OOM killer until it is unhashed.

I think this is a very minor problem, in the worst case you get a
false positive oom kill, and it requires a race condition for it to
happen. I wouldn't add mmap_sem in exit_mmap just for this considering
the mmget_not_zero is already enough to leave exit_mmap alone.

Could you first clarify these points then I'll understand better what
the above is about:

1) if exit_mmap runs for a long time with terabytes of RAM with
   mmap_sem held for writing like your patch does, wouldn't then
   oom_reap_task_mm fail the same way after a few tries on
   down_read_trylock? Despite your patch got applied? Isn't that
   simply moving the failure that leads to set_bit(MMF_OOM_SKIP) from
   mmget_not_zero to down_read_trylock?

2) why isn't __oom_reap_task_mm returning different retvals in case
   mmget_not_zero fails? What is the point to schedule_timeout
   and retry MAX_OOM_REAP_RETRIES times if mmget_not_zero caused it to
   return null as it can't do anything about such task anymore? Why
   are we scheduling those RETRIES times if mm_users is 0?

3) if exit_mmap is freeing lots of memory already, why should there be
   another OOM immediately? I thought oom reaper only was needed when
   the task on the right column couldn't reach the final mmput to set
   mm_users to 0. Why exactly is a problem that MMF_OOM_SKIP gets set
   on the mm, if exit_mmap is already guaranteed to be running? Why
   isn't the oom reaper happy to just stop in such case and wait it to
   complete? exit_mmap doesn't even take the mmap_sem and it's running
   in R state, how would it block in a way that requires the OOM
   reaper to free memory from another process to complete?

4) how is it safe to overwrite a VM_FAULT_RETRY that returns without
   mmap_sem and then the arch code will release the mmap_sem despite
   it was already released by handle_mm_fault? Anonymous memory faults
   aren't common to return VM_FAULT_RETRY but an userfault
   can. Shouldn't there be a block that prevents overwriting if
   VM_FAULT_RETRY is set below? (not only VM_FAULT_ERROR)

	if (unlikely((current->flags & PF_KTHREAD) && !(ret & VM_FAULT_ERROR)
				&& test_bit(MMF_UNSTABLE, &vma->vm_mm->flags)))
		ret = VM_FAULT_SIGBUS;

Thanks,
Andrea

[toc] | [prev] | [next] | [standalone]

#1695865

On Tue 25-07-17 17:26:39, Andrea Arcangeli wrote:
> On Mon, Jul 24, 2017 at 09:23:32AM +0200, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > David has noticed that the oom killer might kill additional tasks while
> > the exiting oom victim hasn't terminated yet because the oom_reaper marks
> > the curent victim MMF_OOM_SKIP too early when mm->mm_users dropped down
> > to 0. The race is as follows
> > 
> > oom_reap_task				do_exit
> > 					  exit_mm
> >   __oom_reap_task_mm
> > 					    mmput
> > 					      __mmput
> >     mmget_not_zero # fails
> >     						exit_mmap # frees memory
> >   set_bit(MMF_OOM_SKIP)
> > 
> > The victim is still visible to the OOM killer until it is unhashed.
> 
> I think this is a very minor problem, in the worst case you get a
> false positive oom kill, and it requires a race condition for it to
> happen. I wouldn't add mmap_sem in exit_mmap just for this considering
> the mmget_not_zero is already enough to leave exit_mmap alone.

That problem is real though as reported by David.

> Could you first clarify these points then I'll understand better what
> the above is about:
> 
> 1) if exit_mmap runs for a long time with terabytes of RAM with
>    mmap_sem held for writing like your patch does, wouldn't then
>    oom_reap_task_mm fail the same way after a few tries on
>    down_read_trylock? Despite your patch got applied? Isn't that
>    simply moving the failure that leads to set_bit(MMF_OOM_SKIP) from
>    mmget_not_zero to down_read_trylock?

No, it's not because the exclusive lock in exit_mmap is taken _after_ we
unmapped the address space. unmap_vmas will happily race with the oom
reaper.
 
> 2) why isn't __oom_reap_task_mm returning different retvals in case
>    mmget_not_zero fails? What is the point to schedule_timeout
>    and retry MAX_OOM_REAP_RETRIES times if mmget_not_zero caused it to
>    return null as it can't do anything about such task anymore? Why
>    are we scheduling those RETRIES times if mm_users is 0?

We are not. __oom_reap_task_mm will return true if the mm_users is 0 and
bail out.

> 3) if exit_mmap is freeing lots of memory already, why should there be
>    another OOM immediately?

Because the memory can be freed from a different oom domain (e.g. a
different NUMA node).

>    I thought oom reaper only was needed when
>    the task on the right column couldn't reach the final mmput to set
>    mm_users to 0. Why exactly is a problem that MMF_OOM_SKIP gets set
>    on the mm, if exit_mmap is already guaranteed to be running?

MMF_OOM_SKIP will hide this task from the OOM killer and so we will
select another victim if we are still under oom. We _want_ to postpone
setting MMF_OOM_SKIP until we know that the oom victim no longer
interesting and we can go on to select another one.

>    Why
>    isn't the oom reaper happy to just stop in such case and wait it to
>    complete?

Because there is no _guarantee_ that the final __mmput will release the
memory in finite time. And we cannot guarantee that longterm.

>    exit_mmap doesn't even take the mmap_sem and it's running
>    in R state, how would it block in a way that requires the OOM
>    reaper to free memory from another process to complete?

it is not only about exit_mmap. __mmput calls into exit_aio and that can
wait for completion and there is no way to guarantee this will finish in
finite time.

> 4) how is it safe to overwrite a VM_FAULT_RETRY that returns without
>    mmap_sem and then the arch code will release the mmap_sem despite
>    it was already released by handle_mm_fault? Anonymous memory faults
>    aren't common to return VM_FAULT_RETRY but an userfault
>    can. Shouldn't there be a block that prevents overwriting if
>    VM_FAULT_RETRY is set below? (not only VM_FAULT_ERROR)
> 
> 	if (unlikely((current->flags & PF_KTHREAD) && !(ret & VM_FAULT_ERROR)
> 				&& test_bit(MMF_UNSTABLE, &vma->vm_mm->flags)))
> 		ret = VM_FAULT_SIGBUS;

I am not sure I understand what you mean and how this is related to the
patch?

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1696011

On Tue, Jul 25, 2017 at 05:45:14PM +0200, Michal Hocko wrote:
> That problem is real though as reported by David.

I'm not against fixing it, I just think it's not a major concern, and
the solution doesn't seem optimal as measured by Kirill.

I'm just skeptical it's the best to solve that tiny race, 99.9% of the
time such down_write is unnecessary.

> it is not only about exit_mmap. __mmput calls into exit_aio and that can
> wait for completion and there is no way to guarantee this will finish in
> finite time.

exit_aio blocking is actually the only good point for wanting this
concurrency where exit_mmap->unmap_vmas and
oom_reap_task->unmap_page_range have to run concurrently on the same
mm.

exit_mmap would have no issue, if there was enough time in the
lifetime CPU to allocate the memory, sure the memory will also be
freed in finite amount of time by exit_mmap.

In fact you mentioned multiple OOM in the NUMA case, exit_mmap may not
solve that, so depending on the runtime it may have been better not to
wait all memory of the process to be freed before moving to the next
task, but only a couple of seconds before the OOM reaper moves to a
new candidate. Again this is only a tradeoff between solving the OOM
faster vs risk of false positives OOM.

If it wasn't because of exit_aio (which may have to wait I/O
completion), changing the OOM reaper to return "false" if
mmget_not_zero returns zero and MMF_OOM_SKIP is not set yet, would
have been enough (and depending on the runtime it may have solved OOM
faster in NUMA) and there would be absolutely no need to run OOM
reaper and exit_mmap concurrently on the same mm. However there's such
exit_aio..

Raw I/O mempools never require memory allocations, although aio if it
involves a filesystem to complete may run into filesystem or buffering
locks which are known to loop forever or depend on other tasks stuck
in kernel allocations, so I didn't go down that chain too long.

So the simplest is to use a similar trick to what ksm_exit uses, this
is untested just to show the idea it may require further adjustment as
the bit isn't used only for the test_and_set_bit locking, but I didn't
see much issues with other set_bit/test_bit.

From f414244480fdc1f771b3148feb3fac77ec813e4c Mon Sep 17 00:00:00 2001
From: Andrea Arcangeli <aarcange@redhat.com>
Date: Tue, 25 Jul 2017 20:02:27 +0200
Subject: [PATCH 1/1] mm: oom: let oom_reap_task and exit_mmap to run
 concurrently

This is purely required because exit_aio() may block and exit_mmap() may
never start, if the oom_reap_task cannot start running on a mm with
mm_users == 0.

At the same time if the OOM reaper doesn't wait at all for the memory
of the current OOM candidate to be freed by exit_mmap->unmap_vmas, it
would generate a spurious OOM kill.

If it wasn't because of the exit_aio it would be enough to change the
oom_reap_task in the case it finds mm_users == 0, to wait for a
timeout or to wait for __mmput to set MMF_OOM_SKIP itself, but it's
not just exit_mmap the problem here so the concurrency of exit_mmap
and oom_reap_task is apparently warranted.

It's a non standard runtime, exit_mmap runs without mmap_sem, and
oom_reap_task runs with the mmap_sem for reading as usual (kind of
MADV_DONTNEED).

The race between the two is solved with pair of test_and_set_bit and a
dummy down_write/up_write cycle on the same lines of the ksm_exit
method.

If the OOM reaper sets the bit first, exit_mmap will wait it to finish
in down_write (before taking down mm structures that would make the
oom_reap_task fail with use after free).

If exit_mmap comes first, oom reaper will skip the mm as it's already
all freed.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 kernel/fork.c |  1 -
 mm/mmap.c     |  5 +++++
 mm/oom_kill.c | 15 ++++++++++++---
 3 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 9ec98b0c4675..ed412d85a596 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -910,7 +910,6 @@ static inline void __mmput(struct mm_struct *mm)
 	}
 	if (mm->binfmt)
 		module_put(mm->binfmt->module);
-	set_bit(MMF_OOM_SKIP, &mm->flags);
 	mmdrop(mm);
 }
 
diff --git a/mm/mmap.c b/mm/mmap.c
index f19efcf75418..615133762b99 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2993,6 +2993,11 @@ void exit_mmap(struct mm_struct *mm)
 	/* Use -1 here to ensure all VMAs in the mm are unmapped */
 	unmap_vmas(&tlb, vma, 0, -1);
 
+	if (test_and_set_bit(MMF_OOM_SKIP, &mm->flags)) {
+		/* wait the OOM reaper to stop working on this mm */
+		down_write(&mm->mmap_sem);
+		up_write(&mm->mmap_sem);
+	}
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 9e8b4f030c1c..2a7000995784 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -471,6 +471,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
 	bool ret = true;
+	bool mmgot = true;
 
 	/*
 	 * We have to make sure to not race with the victim exit path
@@ -500,9 +501,16 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 * and delayed __mmput doesn't matter that much
 	 */
 	if (!mmget_not_zero(mm)) {
-		up_read(&mm->mmap_sem);
 		trace_skip_task_reaping(tsk->pid);
-		goto unlock_oom;
+		/*
+		 * MMF_OOM_SKIP is set by exit_mmap when the OOM
+		 * reaper can't work on the mm anymore.
+		 */
+		if (test_and_set_bit(MMF_OOM_SKIP, &mm->flags)) {
+			up_read(&mm->mmap_sem);
+			goto unlock_oom;
+		}
+		mmgot = false;
 	}
 
 	trace_start_task_reaping(tsk->pid);
@@ -547,7 +555,8 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	 * different context because we shouldn't risk we get stuck there and
 	 * put the oom_reaper out of the way.
 	 */
-	mmput_async(mm);
+	if (mmgot)
+		mmput_async(mm);
 	trace_finish_task_reaping(tsk->pid);
 unlock_oom:
 	mutex_unlock(&oom_lock);

This will perform the same as then the set_bit in __mmput can be
removed and test_and_set_bit doesn't cost more (at least on x86, on
other archs it requires an smp_mb too, but it's marginal difference),
still an atomic op that is.

> > 4) how is it safe to overwrite a VM_FAULT_RETRY that returns without
> >    mmap_sem and then the arch code will release the mmap_sem despite
> >    it was already released by handle_mm_fault? Anonymous memory faults
> >    aren't common to return VM_FAULT_RETRY but an userfault
> >    can. Shouldn't there be a block that prevents overwriting if
> >    VM_FAULT_RETRY is set below? (not only VM_FAULT_ERROR)
> > 
> > 	if (unlikely((current->flags & PF_KTHREAD) && !(ret & VM_FAULT_ERROR)
> > 				&& test_bit(MMF_UNSTABLE, &vma->vm_mm->flags)))
> > 		ret = VM_FAULT_SIGBUS;
> 
> I am not sure I understand what you mean and how this is related to the
> patch?

It's not related to the patch but it involves the OOM reaper as it
only happens when MMF_UNSTABLE is set which is set only by the OOM
reaper. I was simply reading the OOM reaper code and following up what
MMF_UNSTABLE does and it ringed a bell.

[toc] | [prev] | [next] | [standalone]

#1696795

On Tue 25-07-17 20:26:19, Andrea Arcangeli wrote:
> On Tue, Jul 25, 2017 at 05:45:14PM +0200, Michal Hocko wrote:
> > That problem is real though as reported by David.
> 
> I'm not against fixing it, I just think it's not a major concern, and
> the solution doesn't seem optimal as measured by Kirill.
> 
> I'm just skeptical it's the best to solve that tiny race, 99.9% of the
> time such down_write is unnecessary.
> 
> > it is not only about exit_mmap. __mmput calls into exit_aio and that can
> > wait for completion and there is no way to guarantee this will finish in
> > finite time.
> 
> exit_aio blocking is actually the only good point for wanting this
> concurrency where exit_mmap->unmap_vmas and
> oom_reap_task->unmap_page_range have to run concurrently on the same
> mm.

Yes, exit_aio is the only blocking call I know of currently. But I would
like this to be as robust as possible and so I do not want to rely on
the current implementation. This can change in future and I can
guarantee that nobody will think about the oom path when adding
something to the final __mmput path.

> exit_mmap would have no issue, if there was enough time in the
> lifetime CPU to allocate the memory, sure the memory will also be
> freed in finite amount of time by exit_mmap.

I am not sure I understand. Say that any call prior to unmap_vmas blocks
on a lock which is held by another call path which cannot proceed with
the allocation...
 
> In fact you mentioned multiple OOM in the NUMA case, exit_mmap may not
> solve that, so depending on the runtime it may have been better not to
> wait all memory of the process to be freed before moving to the next
> task, but only a couple of seconds before the OOM reaper moves to a
> new candidate. Again this is only a tradeoff between solving the OOM
> faster vs risk of false positives OOM.

I really do not want to rely on any timing. This just too fragile. Once
we have killed a task then we shouldn't pick another victim until it
passed exit_mmap or the oom_reaper did its job. Otherwise we just risk
false positives while we have already disrupted the workload.
 
> If it wasn't because of exit_aio (which may have to wait I/O
> completion), changing the OOM reaper to return "false" if
> mmget_not_zero returns zero and MMF_OOM_SKIP is not set yet, would
> have been enough (and depending on the runtime it may have solved OOM
> faster in NUMA) and there would be absolutely no need to run OOM
> reaper and exit_mmap concurrently on the same mm. However there's such
> exit_aio..
> 
> Raw I/O mempools never require memory allocations, although aio if it
> involves a filesystem to complete may run into filesystem or buffering
> locks which are known to loop forever or depend on other tasks stuck
> in kernel allocations, so I didn't go down that chain too long.

Exactly. We simply cannot assume anything here because veryfying this
basically impossible.
 
[...]
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f19efcf75418..615133762b99 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2993,6 +2993,11 @@ void exit_mmap(struct mm_struct *mm)
>  	/* Use -1 here to ensure all VMAs in the mm are unmapped */
>  	unmap_vmas(&tlb, vma, 0, -1);
>  
> +	if (test_and_set_bit(MMF_OOM_SKIP, &mm->flags)) {
> +		/* wait the OOM reaper to stop working on this mm */
> +		down_write(&mm->mmap_sem);
> +		up_write(&mm->mmap_sem);
> +	}
>  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
>  	tlb_finish_mmu(&tlb, 0, -1);
>  
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 9e8b4f030c1c..2a7000995784 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -471,6 +471,7 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  	struct mmu_gather tlb;
>  	struct vm_area_struct *vma;
>  	bool ret = true;
> +	bool mmgot = true;
>  
>  	/*
>  	 * We have to make sure to not race with the victim exit path
> @@ -500,9 +501,16 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
>  	 * and delayed __mmput doesn't matter that much
>  	 */
>  	if (!mmget_not_zero(mm)) {
> -		up_read(&mm->mmap_sem);
>  		trace_skip_task_reaping(tsk->pid);
> -		goto unlock_oom;
> +		/*
> +		 * MMF_OOM_SKIP is set by exit_mmap when the OOM
> +		 * reaper can't work on the mm anymore.
> +		 */
> +		if (test_and_set_bit(MMF_OOM_SKIP, &mm->flags)) {
> +			up_read(&mm->mmap_sem);
> +			goto unlock_oom;
> +		}
> +		mmgot = false;
>  	}

This will work more or less the same to what we have currently.

[victim]		[oom reaper]				[oom killer]
do_exit			__oom_reap_task_mm
  mmput
    __mmput
			  mmget_not_zero
			    test_and_set_bit(MMF_OOM_SKIP)
			    					oom_evaluate_task
								   # select next victim 
			  # reap the mm
      unmap_vmas

so we can select a next victim while the current one is still not
completely torn down.

> > > 4) how is it safe to overwrite a VM_FAULT_RETRY that returns without
> > >    mmap_sem and then the arch code will release the mmap_sem despite
> > >    it was already released by handle_mm_fault? Anonymous memory faults
> > >    aren't common to return VM_FAULT_RETRY but an userfault
> > >    can. Shouldn't there be a block that prevents overwriting if
> > >    VM_FAULT_RETRY is set below? (not only VM_FAULT_ERROR)
> > > 
> > > 	if (unlikely((current->flags & PF_KTHREAD) && !(ret & VM_FAULT_ERROR)
> > > 				&& test_bit(MMF_UNSTABLE, &vma->vm_mm->flags)))
> > > 		ret = VM_FAULT_SIGBUS;
> > 
> > I am not sure I understand what you mean and how this is related to the
> > patch?
> 
> It's not related to the patch but it involves the OOM reaper as it
> only happens when MMF_UNSTABLE is set which is set only by the OOM
> reaper. I was simply reading the OOM reaper code and following up what
> MMF_UNSTABLE does and it ringed a bell.

I hope 3f70dc38cec2 ("mm: make sure that kthreads will not refault oom
reaped memory") will clarify this code. If not please start a new thread
so that we do not conflate different things together.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1697341

On Wed, Jul 26, 2017 at 07:45:33AM +0200, Michal Hocko wrote:
> Yes, exit_aio is the only blocking call I know of currently. But I would
> like this to be as robust as possible and so I do not want to rely on
> the current implementation. This can change in future and I can
> guarantee that nobody will think about the oom path when adding
> something to the final __mmput path.

I think ksm_exit may block too waiting for allocations, the generic
idea is those calls before exit_mmap can cause a problem yes.

> > exit_mmap would have no issue, if there was enough time in the
> > lifetime CPU to allocate the memory, sure the memory will also be
> > freed in finite amount of time by exit_mmap.
> 
> I am not sure I understand. Say that any call prior to unmap_vmas blocks
> on a lock which is held by another call path which cannot proceed with
> the allocation...

What I meant was, if three was no prior call to exit_mmap->unmap_vmas.

> I really do not want to rely on any timing. This just too fragile. Once
> we have killed a task then we shouldn't pick another victim until it
> passed exit_mmap or the oom_reaper did its job. Otherwise we just risk
> false positives while we have already disrupted the workload.

On smaller systems lack or parallelism in OOM killing surely isn't a
problem.

> This will work more or less the same to what we have currently.
> 
> [victim]		[oom reaper]				[oom killer]
> do_exit			__oom_reap_task_mm
>   mmput
>     __mmput
> 			  mmget_not_zero
> 			    test_and_set_bit(MMF_OOM_SKIP)
> 			    					oom_evaluate_task
> 								   # select next victim 
> 			  # reap the mm
>       unmap_vmas
>
> so we can select a next victim while the current one is still not
> completely torn down.

How does oom_evaluate_task possibly run at the same time of
test_and_set_bit in __oom_reap_task_mm considering both are running
under the oom_lock? It's hard to see how what you describe above could
materialize as second and third column cannot run in parallel because
of the oom_lock.

I don't think there was any issue, but then you pointed out the
locking on signal->oom_mm that is protected by the task_lock vs
current->mm NULL check, so I can replace in my patch the
test_and_set_bit with set_bit on one side and the oom_mm task_lock
protected locking on the other side. This way I can put back a set_bit
in the __mmput fast path (instead of test_and_set_bit) and it's even
more efficient. With such a change, I'll also stop depending on the
oom_lock to prevent second and third column to run in parallel.

I still didn't remove the oom_lock outright that seems orthogonal
change unrelated to this issue but now you could remove it as far as
the above is concerned.

> I hope 3f70dc38cec2 ("mm: make sure that kthreads will not refault oom
> reaped memory") will clarify this code. If not please start a new thread
> so that we do not conflate different things together.

I'll look into that, thanks.
Andrea

[toc] | [prev] | [next] | [standalone]

#1697743

On Wed 26-07-17 18:39:28, Andrea Arcangeli wrote:
> On Wed, Jul 26, 2017 at 07:45:33AM +0200, Michal Hocko wrote:
> > Yes, exit_aio is the only blocking call I know of currently. But I would
> > like this to be as robust as possible and so I do not want to rely on
> > the current implementation. This can change in future and I can
> > guarantee that nobody will think about the oom path when adding
> > something to the final __mmput path.
> 
> I think ksm_exit may block too waiting for allocations, the generic
> idea is those calls before exit_mmap can cause a problem yes.

I thought that ksm used __GFP_NORETRY but haven't checked too deeply.
Anyway I guess we agree that enabling oom_reaper to race with the final
__mmput is desirable?

[...]
> > This will work more or less the same to what we have currently.
> > 
> > [victim]		[oom reaper]				[oom killer]
> > do_exit			__oom_reap_task_mm
> >   mmput
> >     __mmput
> > 			  mmget_not_zero
> > 			    test_and_set_bit(MMF_OOM_SKIP)
> > 			    					oom_evaluate_task
> > 								   # select next victim 
> > 			  # reap the mm
> >       unmap_vmas
> >
> > so we can select a next victim while the current one is still not
> > completely torn down.
> 
> How does oom_evaluate_task possibly run at the same time of
> test_and_set_bit in __oom_reap_task_mm considering both are running
> under the oom_lock?

You are absolutely right. This race is impossible. It was just me
assuming we are going to get rid of the oom_lock because I have that
idea in the back of my head and I would really like to get rid of
it. Global locks are nasty and I would prefer dropping it if we can.

[...]
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web