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 1 of 2  [1] 2  Next page →

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

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.

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>
---
Hi,
I've sent this as an RFC [1] previously and it seems that the original
issue has been resolved [2], although an explicit tested-by would be
appreciated of course. Hugh has pointed out [3] that using mmap_sem
in exit_mmap will allow to drop the tricky
	down_write(mmap_sem);
	up_write(mmap_sem);
in both paths. I hope I will get to that in a forseeable future.

I am not yet sure this is important enough to merge to stable trees,
I would rather wait for a report to show up.

[1] http://lkml.kernel.org/r/20170626130346.26314-1-mhocko@kernel.org
[2] http://lkml.kernel.org/r/alpine.DEB.2.10.1707111336250.60183@chino.kir.corp.google.com
[3] http://lkml.kernel.org/r/alpine.LSU.2.11.1707191716030.2055@eggly.anvils

 mm/mmap.c     |  7 +++++++
 mm/oom_kill.c | 45 +++++++--------------------------------------
 2 files changed, 14 insertions(+), 38 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..a6dabe3691c1 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,17 +515,11 @@ 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);
+unlock:
+	up_read(&mm->mmap_sem);
+
 	return ret;
 }
 
-- 
2.13.2

[toc] | [next] | [standalone]

#1694753

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.
> 
> 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.

That's true, but we take mmap_sem there for small portion of cases.

It's quite different from taking the lock unconditionally. I'm worry about
scalability implication of such move. On bigger machines it can be big
hit.

Should we do performance/scalability evaluation of the patch before
getting it applied?

-- 
 Kirill A. Shutemov

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

#1694759

On Mon 24-07-17 17:00:08, Kirill A. Shutemov 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.
> > 
> > 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.
> 
> That's true, but we take mmap_sem there for small portion of cases.
> 
> It's quite different from taking the lock unconditionally. I'm worry about
> scalability implication of such move. On bigger machines it can be big
> hit.

What kind of scalability implication you have in mind? There is
basically a zero contention on the mmap_sem that late in the exit path
so this should be pretty much a fast path of the down_write. I agree it
is not 0 cost but the cost of the address space freeing should basically
make it a noise.

> Should we do performance/scalability evaluation of the patch before
> getting it applied?

What kind of test(s) would you be interested in?
-- 
Michal Hocko
SUSE Labs

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

#1694777

On Mon, Jul 24, 2017 at 04:15:26PM +0200, Michal Hocko wrote:
> On Mon 24-07-17 17:00:08, Kirill A. Shutemov 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.
> > > 
> > > 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.
> > 
> > That's true, but we take mmap_sem there for small portion of cases.
> > 
> > It's quite different from taking the lock unconditionally. I'm worry about
> > scalability implication of such move. On bigger machines it can be big
> > hit.
> 
> What kind of scalability implication you have in mind? There is
> basically a zero contention on the mmap_sem that late in the exit path
> so this should be pretty much a fast path of the down_write. I agree it
> is not 0 cost but the cost of the address space freeing should basically
> make it a noise.

Even in fast path case, it adds two atomic operation per-process. If the
cache line is not exclusive to the core by the time of exit(2) it can be
noticible.

... but I guess it's not very hot scenario.

I guess I'm just too cautious here. :)

> > Should we do performance/scalability evaluation of the patch before
> > getting it applied?
> 
> What kind of test(s) would you be interested in?

Can we at lest check that number of /bin/true we can spawn per second
wouldn't be harmed by the patch? ;)

-- 
 Kirill A. Shutemov

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

#1694865

On Mon 24-07-17 17:51:42, Kirill A. Shutemov wrote:
> On Mon, Jul 24, 2017 at 04:15:26PM +0200, Michal Hocko wrote:
[...]
> > What kind of scalability implication you have in mind? There is
> > basically a zero contention on the mmap_sem that late in the exit path
> > so this should be pretty much a fast path of the down_write. I agree it
> > is not 0 cost but the cost of the address space freeing should basically
> > make it a noise.
> 
> Even in fast path case, it adds two atomic operation per-process. If the
> cache line is not exclusive to the core by the time of exit(2) it can be
> noticible.
> 
> ... but I guess it's not very hot scenario.
> 
> I guess I'm just too cautious here. :)

I definitely did not want to handwave your concern. I just think we can
rule out the slow path and didn't think about the fast path overhead.

> > > Should we do performance/scalability evaluation of the patch before
> > > getting it applied?
> > 
> > What kind of test(s) would you be interested in?
> 
> Can we at lest check that number of /bin/true we can spawn per second
> wouldn't be harmed by the patch? ;)

OK, so measuring a single /bin/true doesn't tell anything so I've done
root@test1:~# cat a.sh 
#!/bin/sh

NR=$1
for i in $(seq $NR)
do
        /bin/true
done

in my virtual machine (on a otherwise idle host) with 4 cpus and 2GB of
RAM

Unpatched kernel
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 53.57
        System time (seconds): 26.12
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.46
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 53.90
        System time (seconds): 26.23
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.77
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 54.02
        System time (seconds): 26.18
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.92

patched kernel
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 53.81
        System time (seconds): 26.55
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.99
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 53.78
        System time (seconds): 26.15
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.67
root@test1:~# /usr/bin/time -v ./a.sh 100000 
        Command being timed: "./a.sh 100000"
        User time (seconds): 54.08
        System time (seconds): 26.87
        Percent of CPU this job got: 100%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:20.52

the results very quite a lot (have a look at the user time which
shouldn't have no reason to vary at all - maybe the virtual machine
aspect?). I would say that we are still reasonably close to a noise
here. Considering that /bin/true would close to the worst case I think
this looks reasonably. What do you think?

If you absolutely insist, I can make the lock conditional only for oom
victims. That would still mean current->signal->oom_mm pointers fetches
and a 2 branches.
-- 
Michal Hocko
SUSE Labs

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

#1695774

On Mon, Jul 24, 2017 at 06:11:47PM +0200, Michal Hocko wrote:
> On Mon 24-07-17 17:51:42, Kirill A. Shutemov wrote:
> > On Mon, Jul 24, 2017 at 04:15:26PM +0200, Michal Hocko wrote:
> [...]
> > > What kind of scalability implication you have in mind? There is
> > > basically a zero contention on the mmap_sem that late in the exit path
> > > so this should be pretty much a fast path of the down_write. I agree it
> > > is not 0 cost but the cost of the address space freeing should basically
> > > make it a noise.
> > 
> > Even in fast path case, it adds two atomic operation per-process. If the
> > cache line is not exclusive to the core by the time of exit(2) it can be
> > noticible.
> > 
> > ... but I guess it's not very hot scenario.
> > 
> > I guess I'm just too cautious here. :)
> 
> I definitely did not want to handwave your concern. I just think we can
> rule out the slow path and didn't think about the fast path overhead.
> 
> > > > Should we do performance/scalability evaluation of the patch before
> > > > getting it applied?
> > > 
> > > What kind of test(s) would you be interested in?
> > 
> > Can we at lest check that number of /bin/true we can spawn per second
> > wouldn't be harmed by the patch? ;)
> 
> OK, so measuring a single /bin/true doesn't tell anything so I've done
> root@test1:~# cat a.sh 
> #!/bin/sh
> 
> NR=$1
> for i in $(seq $NR)
> do
>         /bin/true
> done
> 
> in my virtual machine (on a otherwise idle host) with 4 cpus and 2GB of
> RAM
> 
> Unpatched kernel
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 53.57
>         System time (seconds): 26.12
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.46
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 53.90
>         System time (seconds): 26.23
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.77
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 54.02
>         System time (seconds): 26.18
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.92
> 
> patched kernel
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 53.81
>         System time (seconds): 26.55
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.99
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 53.78
>         System time (seconds): 26.15
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:19.67
> root@test1:~# /usr/bin/time -v ./a.sh 100000 
>         Command being timed: "./a.sh 100000"
>         User time (seconds): 54.08
>         System time (seconds): 26.87
>         Percent of CPU this job got: 100%
>         Elapsed (wall clock) time (h:mm:ss or m:ss): 1:20.52
> 
> the results very quite a lot (have a look at the user time which
> shouldn't have no reason to vary at all - maybe the virtual machine
> aspect?). I would say that we are still reasonably close to a noise
> here. Considering that /bin/true would close to the worst case I think
> this looks reasonably. What do you think?
> 
> If you absolutely insist, I can make the lock conditional only for oom
> victims. That would still mean current->signal->oom_mm pointers fetches
> and a 2 branches.


Below are numbers for the same test case, but from bigger machine (48
threads, 64GiB of RAM).

v4.13-rc2:

 Performance counter stats for './a.sh 100000' (5 runs):

     159857.233790      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  3.21% )
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
         8,761,843      page-faults:u             #    0.055 M/sec                    ( +-  0.64% )
    38,725,763,026      cycles:u                  #    0.242 GHz                      ( +-  0.18% )
   272,691,643,016      stalled-cycles-frontend:u #  704.16% frontend cycles idle     ( +-  3.16% )
    22,221,416,575      instructions:u            #    0.57  insn per cycle
                                                  #   12.27  stalled cycles per insn  ( +-  0.00% )
     5,306,829,649      branches:u                #   33.197 M/sec                    ( +-  0.00% )
       240,783,599      branch-misses:u           #    4.54% of all branches          ( +-  0.15% )

     159.808721098 seconds time elapsed                                          ( +-  3.15% )

v4.13-rc2 + the patch:

 Performance counter stats for './a.sh 100000' (5 runs):

     167628.094556      task-clock:u (msec)       #    1.007 CPUs utilized            ( +-  1.63% )
                 0      context-switches:u        #    0.000 K/sec
                 0      cpu-migrations:u          #    0.000 K/sec
         8,838,314      page-faults:u             #    0.053 M/sec                    ( +-  0.26% )
    38,862,240,137      cycles:u                  #    0.232 GHz                      ( +-  0.10% )
   282,105,057,553      stalled-cycles-frontend:u #  725.91% frontend cycles idle     ( +-  1.64% )
    22,219,273,623      instructions:u            #    0.57  insn per cycle
                                                  #   12.70  stalled cycles per insn  ( +-  0.00% )
     5,306,165,194      branches:u                #   31.654 M/sec                    ( +-  0.00% )
       240,473,075      branch-misses:u           #    4.53% of all branches          ( +-  0.07% )

     166.497005412 seconds time elapsed                                          ( +-  1.61% )

IMO, there is something to think about. ~4% slowdown is not insignificant.
I expect effect to be bigger for larger machines.

-- 
 Kirill A. Shutemov

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

#1695779

On Tue 25-07-17 17:17:23, Kirill A. Shutemov wrote:
[...]
> Below are numbers for the same test case, but from bigger machine (48
> threads, 64GiB of RAM).
> 
> v4.13-rc2:
> 
>  Performance counter stats for './a.sh 100000' (5 runs):
> 
>      159857.233790      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  3.21% )
>                  0      context-switches:u        #    0.000 K/sec
>                  0      cpu-migrations:u          #    0.000 K/sec
>          8,761,843      page-faults:u             #    0.055 M/sec                    ( +-  0.64% )
>     38,725,763,026      cycles:u                  #    0.242 GHz                      ( +-  0.18% )
>    272,691,643,016      stalled-cycles-frontend:u #  704.16% frontend cycles idle     ( +-  3.16% )
>     22,221,416,575      instructions:u            #    0.57  insn per cycle
>                                                   #   12.27  stalled cycles per insn  ( +-  0.00% )
>      5,306,829,649      branches:u                #   33.197 M/sec                    ( +-  0.00% )
>        240,783,599      branch-misses:u           #    4.54% of all branches          ( +-  0.15% )
> 
>      159.808721098 seconds time elapsed                                          ( +-  3.15% )
> 
> v4.13-rc2 + the patch:
> 
>  Performance counter stats for './a.sh 100000' (5 runs):
> 
>      167628.094556      task-clock:u (msec)       #    1.007 CPUs utilized            ( +-  1.63% )
>                  0      context-switches:u        #    0.000 K/sec
>                  0      cpu-migrations:u          #    0.000 K/sec
>          8,838,314      page-faults:u             #    0.053 M/sec                    ( +-  0.26% )
>     38,862,240,137      cycles:u                  #    0.232 GHz                      ( +-  0.10% )
>    282,105,057,553      stalled-cycles-frontend:u #  725.91% frontend cycles idle     ( +-  1.64% )
>     22,219,273,623      instructions:u            #    0.57  insn per cycle
>                                                   #   12.70  stalled cycles per insn  ( +-  0.00% )
>      5,306,165,194      branches:u                #   31.654 M/sec                    ( +-  0.00% )
>        240,473,075      branch-misses:u           #    4.53% of all branches          ( +-  0.07% )
> 
>      166.497005412 seconds time elapsed                                          ( +-  1.61% )
> 
> IMO, there is something to think about. ~4% slowdown is not insignificant.
> I expect effect to be bigger for larger machines.

Thanks for retesting Kirill. Are those numbers stable over runs? E.g.
the run without the patch has ~3% variance while the one with the patch
has it smaller. This sounds suspicious to me. There shouldn't be any
lock contention (except for the oom killer) so the lock shouldn't make
any difference wrt. variability.

Also I was about to post a more targeted test. Could you try it with it
as well, please?

-- 
Michal Hocko
SUSE Labs

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

#1695820

On Tue, Jul 25, 2017 at 04:26:17PM +0200, Michal Hocko wrote:
> On Tue 25-07-17 17:17:23, Kirill A. Shutemov wrote:
> [...]
> > Below are numbers for the same test case, but from bigger machine (48
> > threads, 64GiB of RAM).
> > 
> > v4.13-rc2:
> > 
> >  Performance counter stats for './a.sh 100000' (5 runs):
> > 
> >      159857.233790      task-clock:u (msec)       #    1.000 CPUs utilized            ( +-  3.21% )
> >                  0      context-switches:u        #    0.000 K/sec
> >                  0      cpu-migrations:u          #    0.000 K/sec
> >          8,761,843      page-faults:u             #    0.055 M/sec                    ( +-  0.64% )
> >     38,725,763,026      cycles:u                  #    0.242 GHz                      ( +-  0.18% )
> >    272,691,643,016      stalled-cycles-frontend:u #  704.16% frontend cycles idle     ( +-  3.16% )
> >     22,221,416,575      instructions:u            #    0.57  insn per cycle
> >                                                   #   12.27  stalled cycles per insn  ( +-  0.00% )
> >      5,306,829,649      branches:u                #   33.197 M/sec                    ( +-  0.00% )
> >        240,783,599      branch-misses:u           #    4.54% of all branches          ( +-  0.15% )
> > 
> >      159.808721098 seconds time elapsed                                          ( +-  3.15% )
> > 
> > v4.13-rc2 + the patch:
> > 
> >  Performance counter stats for './a.sh 100000' (5 runs):
> > 
> >      167628.094556      task-clock:u (msec)       #    1.007 CPUs utilized            ( +-  1.63% )
> >                  0      context-switches:u        #    0.000 K/sec
> >                  0      cpu-migrations:u          #    0.000 K/sec
> >          8,838,314      page-faults:u             #    0.053 M/sec                    ( +-  0.26% )
> >     38,862,240,137      cycles:u                  #    0.232 GHz                      ( +-  0.10% )
> >    282,105,057,553      stalled-cycles-frontend:u #  725.91% frontend cycles idle     ( +-  1.64% )
> >     22,219,273,623      instructions:u            #    0.57  insn per cycle
> >                                                   #   12.70  stalled cycles per insn  ( +-  0.00% )
> >      5,306,165,194      branches:u                #   31.654 M/sec                    ( +-  0.00% )
> >        240,473,075      branch-misses:u           #    4.53% of all branches          ( +-  0.07% )
> > 
> >      166.497005412 seconds time elapsed                                          ( +-  1.61% )
> > 
> > IMO, there is something to think about. ~4% slowdown is not insignificant.
> > I expect effect to be bigger for larger machines.
> 
> Thanks for retesting Kirill. Are those numbers stable over runs? E.g.
> the run without the patch has ~3% variance while the one with the patch
> has it smaller. This sounds suspicious to me. There shouldn't be any
> lock contention (except for the oom killer) so the lock shouldn't make
> any difference wrt. variability.

There's run-to-tun variability. I'll post new numbers for your new test.

-- 
 Kirill A. Shutemov

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

#1695829

On Tue 25-07-17 18:07:19, Kirill A. Shutemov wrote:
> On Tue, Jul 25, 2017 at 04:26:17PM +0200, Michal Hocko wrote:
[...]
> > Thanks for retesting Kirill. Are those numbers stable over runs? E.g.
> > the run without the patch has ~3% variance while the one with the patch
> > has it smaller. This sounds suspicious to me. There shouldn't be any
> > lock contention (except for the oom killer) so the lock shouldn't make
> > any difference wrt. variability.
> 
> There's run-to-tun variability. I'll post new numbers for your new test.

That's what I've seen and the variance was quite large. I suspected
shell but if you look at the more dedicated test, the std over avg is
still quite large.
-- 
Michal Hocko
SUSE Labs

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

#1695781

[Multipart message — attachments visible in raw view] — view raw

On Mon 24-07-17 18:11:46, Michal Hocko wrote:
> On Mon 24-07-17 17:51:42, Kirill A. Shutemov wrote:
> > On Mon, Jul 24, 2017 at 04:15:26PM +0200, Michal Hocko wrote:
> [...]
> > > What kind of scalability implication you have in mind? There is
> > > basically a zero contention on the mmap_sem that late in the exit path
> > > so this should be pretty much a fast path of the down_write. I agree it
> > > is not 0 cost but the cost of the address space freeing should basically
> > > make it a noise.
> > 
> > Even in fast path case, it adds two atomic operation per-process. If the
> > cache line is not exclusive to the core by the time of exit(2) it can be
> > noticible.
> > 
> > ... but I guess it's not very hot scenario.
> > 
> > I guess I'm just too cautious here. :)
> 
> I definitely did not want to handwave your concern. I just think we can
> rule out the slow path and didn't think about the fast path overhead.
> 
> > > > Should we do performance/scalability evaluation of the patch before
> > > > getting it applied?
> > > 
> > > What kind of test(s) would you be interested in?
> > 
> > Can we at lest check that number of /bin/true we can spawn per second
> > wouldn't be harmed by the patch? ;)
> 
> OK, so measuring a single /bin/true doesn't tell anything so I've done
> root@test1:~# cat a.sh 
> #!/bin/sh
> 
> NR=$1
> for i in $(seq $NR)
> do
>         /bin/true
> done

I wanted to reduce a potential shell side effects so I've come with a
simple program which forks and saves the timestamp before child exit and
right after waitpid (see attached) and then measured it 100k times. Sure
this still measures waitpid overhead and the signal delivery but this
should be more or less constant on an idle system, right? See attached.

before the patch
min: 306300.00 max: 6731916.00 avg: 437962.07 std: 92898.30 nr: 100000

after
min: 303196.00 max: 5728080.00 avg: 436081.87 std: 96165.98 nr: 100000

The results are well withing noise as I would expect.
-- 
Michal Hocko
SUSE Labs

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

#1695830

On Tue, Jul 25, 2017 at 04:26:26PM +0200, Michal Hocko wrote:
> On Mon 24-07-17 18:11:46, Michal Hocko wrote:
> > On Mon 24-07-17 17:51:42, Kirill A. Shutemov wrote:
> > > On Mon, Jul 24, 2017 at 04:15:26PM +0200, Michal Hocko wrote:
> > [...]
> > > > What kind of scalability implication you have in mind? There is
> > > > basically a zero contention on the mmap_sem that late in the exit path
> > > > so this should be pretty much a fast path of the down_write. I agree it
> > > > is not 0 cost but the cost of the address space freeing should basically
> > > > make it a noise.
> > > 
> > > Even in fast path case, it adds two atomic operation per-process. If the
> > > cache line is not exclusive to the core by the time of exit(2) it can be
> > > noticible.
> > > 
> > > ... but I guess it's not very hot scenario.
> > > 
> > > I guess I'm just too cautious here. :)
> > 
> > I definitely did not want to handwave your concern. I just think we can
> > rule out the slow path and didn't think about the fast path overhead.
> > 
> > > > > Should we do performance/scalability evaluation of the patch before
> > > > > getting it applied?
> > > > 
> > > > What kind of test(s) would you be interested in?
> > > 
> > > Can we at lest check that number of /bin/true we can spawn per second
> > > wouldn't be harmed by the patch? ;)
> > 
> > OK, so measuring a single /bin/true doesn't tell anything so I've done
> > root@test1:~# cat a.sh 
> > #!/bin/sh
> > 
> > NR=$1
> > for i in $(seq $NR)
> > do
> >         /bin/true
> > done
> 
> I wanted to reduce a potential shell side effects so I've come with a
> simple program which forks and saves the timestamp before child exit and
> right after waitpid (see attached) and then measured it 100k times. Sure
> this still measures waitpid overhead and the signal delivery but this
> should be more or less constant on an idle system, right? See attached.
> 
> before the patch
> min: 306300.00 max: 6731916.00 avg: 437962.07 std: 92898.30 nr: 100000
> 
> after
> min: 303196.00 max: 5728080.00 avg: 436081.87 std: 96165.98 nr: 100000
> 
> The results are well withing noise as I would expect.

I've silightly modified your test case: replaced cpuid + rdtsc with
rdtscp. cpuid overhead is measurable in such tight loop.

3 runs before the patch:
   Min. 1st Qu.  Median    Mean 3rd Qu.    Max.
 177200  205000  212900  217800  223700 2377000
 172400  201700  209700  214300  220600 1343000
 175700  203800  212300  217100  223000 1061000

3 runs after the patch:
   Min. 1st Qu.  Median    Mean 3rd Qu.    Max.
 175900  204800  213000  216400  223600 1989000
 180300  210900  219600  223600  230200 3184000
 182100  212500  222000  226200  232700 1473000

The difference is still measuarble. Around 3%.

-- 
 Kirill A. Shutemov

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

#1695843

On Tue 25-07-17 18:17:54, Kirill A. Shutemov wrote:
> > before the patch
> > min: 306300.00 max: 6731916.00 avg: 437962.07 std: 92898.30 nr: 100000
> > 
> > after
> > min: 303196.00 max: 5728080.00 avg: 436081.87 std: 96165.98 nr: 100000
> > 
> > The results are well withing noise as I would expect.
> 
> I've silightly modified your test case: replaced cpuid + rdtsc with
> rdtscp. cpuid overhead is measurable in such tight loop.
> 
> 3 runs before the patch:
>    Min. 1st Qu.  Median    Mean 3rd Qu.    Max.
>  177200  205000  212900  217800  223700 2377000
>  172400  201700  209700  214300  220600 1343000
>  175700  203800  212300  217100  223000 1061000
> 
> 3 runs after the patch:
>    Min. 1st Qu.  Median    Mean 3rd Qu.    Max.
>  175900  204800  213000  216400  223600 1989000
>  180300  210900  219600  223600  230200 3184000
>  182100  212500  222000  226200  232700 1473000
> 
> The difference is still measuarble. Around 3%.

what is stdev?
-- 
Michal Hocko
SUSE Labs

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

#1695860

On Tue, Jul 25, 2017 at 05:23:00PM +0200, Michal Hocko wrote:
> what is stdev?

Updated tables:

3 runs before the patch:
   Min. 1st Qu.  Median    Mean 3rd Qu.    Max.  Stdev
 177200  205000  212900  217800  223700 2377000  32868
 172400  201700  209700  214300  220600 1343000  31191
 175700  203800  212300  217100  223000 1061000  31195

3 runs after the patch:
   Min. 1st Qu.  Median    Mean 3rd Qu.    Max.  Stdev
 175900  204800  213000  216400  223600 1989000  27210
 180300  210900  219600  223600  230200 3184000  32609
 182100  212500  222000  226200  232700 1473000  32138

-- 
 Kirill A. Shutemov

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

#1695914

On Tue 25-07-17 18:31:10, Kirill A. Shutemov wrote:
> On Tue, Jul 25, 2017 at 05:23:00PM +0200, Michal Hocko wrote:
> > what is stdev?
> 
> Updated tables:
> 
> 3 runs before the patch:
>    Min. 1st Qu.  Median    Mean 3rd Qu.    Max.  Stdev
>  177200  205000  212900  217800  223700 2377000  32868
>  172400  201700  209700  214300  220600 1343000  31191
>  175700  203800  212300  217100  223000 1061000  31195
> 
> 3 runs after the patch:
>    Min. 1st Qu.  Median    Mean 3rd Qu.    Max.  Stdev
>  175900  204800  213000  216400  223600 1989000  27210
>  180300  210900  219600  223600  230200 3184000  32609
>  182100  212500  222000  226200  232700 1473000  32138

High std/avg ~15% matches my measurements (mine were even higher ~20%)
and that would suggest that 3% average difference is still somehing
within a "noise".

Anyway, I do not really need to take the lock unless the task is the
oom victim. Could you try whether those numbers improve if the lock is
conditional?

Thanks!
---
diff --git a/mm/mmap.c b/mm/mmap.c
index 0eeb658caa30..ca8a274485f8 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -44,6 +44,7 @@
 #include <linux/userfaultfd_k.h>
 #include <linux/moduleparam.h>
 #include <linux/pkeys.h>
+#include <linux/oom.h>
 
 #include <linux/uaccess.h>
 #include <asm/cacheflush.h>
@@ -2997,7 +2998,8 @@ void exit_mmap(struct mm_struct *mm)
 	 * 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);
+	if (tsk_is_oom_victim(current))
+		down_write(&mm->mmap_sem);
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
@@ -3012,7 +3014,8 @@ void exit_mmap(struct mm_struct *mm)
 	}
 	mm->mmap = NULL;
 	vm_unacct_memory(nr_accounted);
-	up_write(&mm->mmap_sem);
+	if (tsk_is_oom_victim(current))
+		up_write(&mm->mmap_sem);
 }
 
 /* Insert vm structure into process list sorted by address
-- 
Michal Hocko
SUSE Labs

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

#1696559

On Tue, Jul 25, 2017 at 06:04:00PM +0200, Michal Hocko wrote:
> -	down_write(&mm->mmap_sem);
> +	if (tsk_is_oom_victim(current))
> +		down_write(&mm->mmap_sem);
>  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
>  	tlb_finish_mmu(&tlb, 0, -1);
>  
> @@ -3012,7 +3014,8 @@ void exit_mmap(struct mm_struct *mm)
>  	}
>  	mm->mmap = NULL;
>  	vm_unacct_memory(nr_accounted);
> -	up_write(&mm->mmap_sem);
> +	if (tsk_is_oom_victim(current))
> +		up_write(&mm->mmap_sem);

How is this possibly safe? mark_oom_victim can run while exit_mmap is
running. Even if you cache the first read in the local stack, failure
to notice you marked it, could lead to use after free. Or at least
there's no comment on which lock should prevent the use after free
with the above.

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

#1696792

On Tue 25-07-17 21:19:52, Andrea Arcangeli wrote:
> On Tue, Jul 25, 2017 at 06:04:00PM +0200, Michal Hocko wrote:
> > -	down_write(&mm->mmap_sem);
> > +	if (tsk_is_oom_victim(current))
> > +		down_write(&mm->mmap_sem);
> >  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
> >  	tlb_finish_mmu(&tlb, 0, -1);
> >  
> > @@ -3012,7 +3014,8 @@ void exit_mmap(struct mm_struct *mm)
> >  	}
> >  	mm->mmap = NULL;
> >  	vm_unacct_memory(nr_accounted);
> > -	up_write(&mm->mmap_sem);
> > +	if (tsk_is_oom_victim(current))
> > +		up_write(&mm->mmap_sem);
> 
> How is this possibly safe? mark_oom_victim can run while exit_mmap is
> running.

I believe it cannot. We always call mark_oom_victim (on !current) with
task_lock held and check task->mm != NULL and we call do_exit->mmput after
mm is set to NULL under the same lock.
-- 
Michal Hocko
SUSE Labs

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

#1697330

On Wed, Jul 26, 2017 at 07:45:57AM +0200, Michal Hocko wrote:
> On Tue 25-07-17 21:19:52, Andrea Arcangeli wrote:
> > On Tue, Jul 25, 2017 at 06:04:00PM +0200, Michal Hocko wrote:
> > > -	down_write(&mm->mmap_sem);
> > > +	if (tsk_is_oom_victim(current))
> > > +		down_write(&mm->mmap_sem);
> > >  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
> > >  	tlb_finish_mmu(&tlb, 0, -1);
> > >  
> > > @@ -3012,7 +3014,8 @@ void exit_mmap(struct mm_struct *mm)
> > >  	}
> > >  	mm->mmap = NULL;
> > >  	vm_unacct_memory(nr_accounted);
> > > -	up_write(&mm->mmap_sem);
> > > +	if (tsk_is_oom_victim(current))
> > > +		up_write(&mm->mmap_sem);
> > 
> > How is this possibly safe? mark_oom_victim can run while exit_mmap is
> > running.
> 
> I believe it cannot. We always call mark_oom_victim (on !current) with
> task_lock held and check task->mm != NULL and we call do_exit->mmput after
> mm is set to NULL under the same lock.

Holding the mmap_sem for writing and setting mm->mmap to NULL to
filter which tasks already released the mmap_sem for writing post
free_pgtables still look unnecessary to solve this.

Using MMF_OOM_SKIP as flag had side effects of oom_badness() skipping
it, but we can use the same tsk_is_oom_victim instead and relay on the
locking in mark_oom_victim you pointed out above instead of the
test_and_set_bit of my patch, because current->mm is already NULL at
that point.

A race at the light of the above now is, because current->mm is NULL by the
time mmput is called, how can you start the oom_reap_task on a process
with current->mm NULL that called the last mmput and is blocked
in exit_aio? It looks like no false positive can get fixed until this
is solved first because 

Isn't this enough? If this is enough it avoids other modification to
the exit_mmap runtime that looks unnecessary: mm->mmap = NULL replaced
by MMF_OOM_SKIP that has to be set anyway by __mmput later and one
unnecessary branch to call the up_write.

From 3d9001490ee1a71f39c7bfaf19e96821f9d3ff16 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 or similar blocking functions in
the last mmput, 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 a combination of
tsk_is_oom_victim() (serialized by task_lock) and MMF_OOM_SKIP
(serialized by a dummy down_write/up_write cycle on the same lines of
the ksm_exit method).

If the oom_reap_task() may be running concurrently during exit_mmap,
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_reap_task() will skip the mm if
MMF_OOM_SKIP is already set and in turn all memory is already freed
and furthermore the mm data structures may already have been taken
down by free_pgtables.

Signed-off-by: Andrea Arcangeli <aarcange@redhat.com>
---
 kernel/fork.c |  1 -
 mm/mmap.c     | 17 +++++++++++++++++
 mm/oom_kill.c | 15 +++++----------
 3 files changed, 22 insertions(+), 11 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..bdab595ce25c 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2993,6 +2993,23 @@ 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);
 
+	set_bit(MMF_OOM_SKIP, &mm->flags);
+	if (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);
+	}
 	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..242a1f0d579b 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -495,11 +495,12 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 	}
 
 	/*
-	 * 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
+	 * 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 (!mmget_not_zero(mm)) {
+	if (test_bit(MMF_OOM_SKIP, &mm->flags)) {
 		up_read(&mm->mmap_sem);
 		trace_skip_task_reaping(tsk->pid);
 		goto unlock_oom;
@@ -542,12 +543,6 @@ static bool __oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm)
 			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);

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

#1697352

On Wed, Jul 26, 2017 at 06:29:12PM +0200, Andrea Arcangeli wrote:
> From 3d9001490ee1a71f39c7bfaf19e96821f9d3ff16 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 needs an incremental one liner...

diff --git a/mm/mmap.c b/mm/mmap.c
index bdab595ce25c..fd16996ee0a8 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -44,6 +44,7 @@
 #include <linux/userfaultfd_k.h>
 #include <linux/moduleparam.h>
 #include <linux/pkeys.h>
+#include <linux/oom.h>
 
 #include <linux/uaccess.h>
 #include <asm/cacheflush.h>

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

#1697753

On Wed 26-07-17 18:29:12, Andrea Arcangeli wrote:
> On Wed, Jul 26, 2017 at 07:45:57AM +0200, Michal Hocko wrote:
> > On Tue 25-07-17 21:19:52, Andrea Arcangeli wrote:
> > > On Tue, Jul 25, 2017 at 06:04:00PM +0200, Michal Hocko wrote:
> > > > -	down_write(&mm->mmap_sem);
> > > > +	if (tsk_is_oom_victim(current))
> > > > +		down_write(&mm->mmap_sem);
> > > >  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
> > > >  	tlb_finish_mmu(&tlb, 0, -1);
> > > >  
> > > > @@ -3012,7 +3014,8 @@ void exit_mmap(struct mm_struct *mm)
> > > >  	}
> > > >  	mm->mmap = NULL;
> > > >  	vm_unacct_memory(nr_accounted);
> > > > -	up_write(&mm->mmap_sem);
> > > > +	if (tsk_is_oom_victim(current))
> > > > +		up_write(&mm->mmap_sem);
> > > 
> > > How is this possibly safe? mark_oom_victim can run while exit_mmap is
> > > running.
> > 
> > I believe it cannot. We always call mark_oom_victim (on !current) with
> > task_lock held and check task->mm != NULL and we call do_exit->mmput after
> > mm is set to NULL under the same lock.
> 
> Holding the mmap_sem for writing and setting mm->mmap to NULL to
> filter which tasks already released the mmap_sem for writing post
> free_pgtables still look unnecessary to solve this.
> 
> Using MMF_OOM_SKIP as flag had side effects of oom_badness() skipping
> it, but we can use the same tsk_is_oom_victim instead and relay on the
> locking in mark_oom_victim you pointed out above instead of the
> test_and_set_bit of my patch, because current->mm is already NULL at
> that point.
> 
> A race at the light of the above now is, because current->mm is NULL by the
> time mmput is called, how can you start the oom_reap_task on a process
> with current->mm NULL that called the last mmput and is blocked
> in exit_aio?

Because we have that mm available. See tsk->signal->oom_mm in
oom_reap_task

> It looks like no false positive can get fixed until this
> is solved first because 
> 
> Isn't this enough? If this is enough it avoids other modification to
> the exit_mmap runtime that looks unnecessary: mm->mmap = NULL replaced
> by MMF_OOM_SKIP that has to be set anyway by __mmput later and one
> unnecessary branch to call the up_write.
> 
[...]
> diff --git a/mm/mmap.c b/mm/mmap.c
> index f19efcf75418..bdab595ce25c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -2993,6 +2993,23 @@ 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);
>  
> +	set_bit(MMF_OOM_SKIP, &mm->flags);
> +	if (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);
> +	}
>  	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
>  	tlb_finish_mmu(&tlb, 0, -1);

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);

in general I do not like empty locked sections much, to be honest. Now
with the conditional locking my patch looks as follows. It should be
pretty much equivalent to your patch. Would that be acceptable for you
or do you think there is a strong reason to go with yours?
---
From 2198654be88d11efb1f372e8579761f65e219206 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Thu, 27 Jul 2017 08:48:15 +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 v1
- bail on null mm->mmap early as per David Rientjes
- take exclusive mmap_sem in exit_mmap only for oom victims to reduce
  the lock overhead

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     | 16 ++++++++++++++++
 mm/oom_kill.c | 47 ++++++++---------------------------------------
 2 files changed, 24 insertions(+), 39 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 24e9261bdcc0..822e8860b9d2 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -44,6 +44,7 @@
 #include <linux/userfaultfd_k.h>
 #include <linux/moduleparam.h>
 #include <linux/pkeys.h>
+#include <linux/oom.h>
 
 #include <linux/uaccess.h>
 #include <asm/cacheflush.h>
@@ -2967,6 +2968,7 @@ void exit_mmap(struct mm_struct *mm)
 	struct mmu_gather tlb;
 	struct vm_area_struct *vma;
 	unsigned long nr_accounted = 0;
+	bool locked = false;
 
 	/* mm's last user has gone, and its about to be pulled down */
 	mmu_notifier_release(mm);
@@ -2993,6 +2995,17 @@ 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
+	 * Please note that mark_oom_victim is always called under task_lock
+	 * with tsk->mm != NULL checked on !current tasks which synchronizes
+	 * with exit_mm and so we cannot race here.
+	 */
+	if (tsk_is_oom_victim(current)) {
+		down_write(&mm->mmap_sem);
+		locked = true;
+	}
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb, 0, -1);
 
@@ -3005,7 +3018,10 @@ void exit_mmap(struct mm_struct *mm)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 	}
+	mm->mmap = NULL;
 	vm_unacct_memory(nr_accounted);
+	if (locked)
+		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]

#1698067

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.

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.

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.

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.

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 ;).

Thanks,
Andrea

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

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web