Re: [PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

Re: [PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

Hi,

[auto build test WARNING on next-20160607]
[cannot apply to v4.7-rc2 v4.7-rc1 v4.6-rc7 v4.7-rc2]
[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_reaper-make-sure-that-mmput_async-is-called-only-when-memory-was-reaped/20160607-211715
config: mn10300-asb2364_defconfig (attached as .config)
compiler: am33_2.0-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mn10300 

All warnings (new ones prefixed by >>):

   mm/oom_kill.c: In function '__oom_reap_task':
>> mm/oom_kill.c:490:7: warning: passing argument 1 of 'mmget_not_zero' from incompatible pointer type
     if (!mmget_not_zero(&mm->mm_users)) {
          ^
   In file included from include/linux/oom.h:5:0,
                    from mm/oom_kill.c:20:
   include/linux/sched.h:2746:20: note: expected 'struct mm_struct *' but argument is of type 'struct atomic_t *'
    static inline bool mmget_not_zero(struct mm_struct *mm)
                       ^

vim +/mmget_not_zero +490 mm/oom_kill.c

   474		if (!p)
   475			goto unlock_oom;
   476		mm = p->mm;
   477		atomic_inc(&mm->mm_count);
   478		task_unlock(p);
   479	
   480		if (!down_read_trylock(&mm->mmap_sem)) {
   481			ret = false;
   482			goto mm_drop;
   483		}
   484	
   485		/*
   486		 * increase mm_users only after we know we will reap something so
   487		 * that the mmput_async is called only when we have reaped something
   488		 * and delayed __mmput doesn't matter that much
   489		 */
 > 490		if (!mmget_not_zero(&mm->mm_users)) {
   491			up_read(&mm->mmap_sem);
   492			goto mm_drop;
   493		}
   494	
   495		tlb_gather_mmu(&tlb, mm, 0, -1);
   496		for (vma = mm->mmap ; vma; vma = vma->vm_next) {
   497			if (is_vm_hugetlb_page(vma))
   498				continue;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 9136 bytes --]

[PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Tetsuo is worried that mmput_async might still lead to a premature
new oom victim selection due to the following race:

__oom_reap_task				exit_mm
  find_lock_task_mm
  atomic_inc(mm->mm_users) # = 2
  task_unlock
  					  task_lock
					  task->mm = NULL
					  up_read(&mm->mmap_sem)
		< somebody write locks mmap_sem >
					  task_unlock
					  mmput
  					    atomic_dec_and_test # = 1
					  exit_oom_victim
  down_read_trylock # failed - no reclaim
  mmput_async # Takes unpredictable amount of time
  		< new OOM situation >

the final __mmput will be executed in the delayed context which might
happen far in the future. Such a race is highly unlikely because the
write holder of mmap_sem would have to be an external task (all direct
holders are already killed or exiting) and it usually have to pin
mm_users in order to do anything reasonable.

We can, however, make sure that the mmput_async is only called when we
do not back off and reap some memory. That would reduce the impact of
the delayed __mmput because the real content would be already freed.
Pin mm_count to keep it alive after we drop task_lock and before we try
to get mmap_sem. If the mmap_sem succeeds we can try to grab mm_users
reference and then go on with unmapping the address space.

It is not clear whether this race is possible at all but it is better
to be more robust and do not pin mm_users unless we are sure we are
actually doing some real work during __oom_reap_task.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index c11f8bdd0c12..a0371ea2c2c7 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -452,7 +452,7 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	 * We have to make sure to not race with the victim exit path
 	 * and cause premature new oom victim selection:
 	 * __oom_reap_task		exit_mm
-	 *   atomic_inc_not_zero
+	 *   mmget_not_zero
 	 *				  mmput
 	 *				    atomic_dec_and_test
 	 *				  exit_oom_victim
@@ -474,12 +474,22 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	if (!p)
 		goto unlock_oom;
 	mm = p->mm;
-	atomic_inc(&mm->mm_users);
+	atomic_inc(&mm->mm_count);
 	task_unlock(p);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
 		ret = false;
-		goto unlock_oom;
+		goto mm_drop;
+	}
+
+	/*
+	 * 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->mm_users)) {
+		up_read(&mm->mmap_sem);
+		goto mm_drop;
 	}
 
 	tlb_gather_mmu(&tlb, mm, 0, -1);
@@ -521,15 +531,16 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	 * to release its memory.
 	 */
 	set_bit(MMF_OOM_REAPED, &mm->flags);
-unlock_oom:
-	mutex_unlock(&oom_lock);
 	/*
 	 * 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.
 	 */
-	if (mm)
-		mmput_async(mm);
+	mmput_async(mm);
+mm_drop:
+	mmdrop(mm);
+unlock_oom:
+	mutex_unlock(&oom_lock);
 	return ret;
 }
 
-- 
2.8.1

Re: [PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

Hi,

[auto build test ERROR on next-20160607]
[cannot apply to v4.7-rc2 v4.7-rc1 v4.6-rc7 v4.7-rc2]
[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_reaper-make-sure-that-mmput_async-is-called-only-when-memory-was-reaped/20160607-211715
config: i386-defconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   mm/oom_kill.c: In function '__oom_reap_task':
>> mm/oom_kill.c:490:22: error: passing argument 1 of 'mmget_not_zero' from incompatible pointer type [-Werror=incompatible-pointer-types]
     if (!mmget_not_zero(&mm->mm_users)) {
                         ^
   In file included from include/linux/oom.h:5:0,
                    from mm/oom_kill.c:20:
   include/linux/sched.h:2746:20: note: expected 'struct mm_struct *' but argument is of type 'atomic_t * {aka struct <anonymous> *}'
    static inline bool mmget_not_zero(struct mm_struct *mm)
                       ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/mmget_not_zero +490 mm/oom_kill.c

   484	
   485		/*
   486		 * increase mm_users only after we know we will reap something so
   487		 * that the mmput_async is called only when we have reaped something
   488		 * and delayed __mmput doesn't matter that much
   489		 */
 > 490		if (!mmget_not_zero(&mm->mm_users)) {
   491			up_read(&mm->mmap_sem);
   492			goto mm_drop;
   493		}

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24912 bytes --]

[PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

[Michal Hocko]

From: Michal Hocko <mhocko@suse.com>

Tetsuo is worried that mmput_async might still lead to a premature
new oom victim selection due to the following race:

__oom_reap_task				exit_mm
  find_lock_task_mm
  atomic_inc(mm->mm_users) # = 2
  task_unlock
  					  task_lock
					  task->mm = NULL
					  up_read(&mm->mmap_sem)
		< somebody write locks mmap_sem >
					  task_unlock
					  mmput
  					    atomic_dec_and_test # = 1
					  exit_oom_victim
  down_read_trylock # failed - no reclaim
  mmput_async # Takes unpredictable amount of time
  		< new OOM situation >

the final __mmput will be executed in the delayed context which might
happen far in the future. Such a race is highly unlikely because the
write holder of mmap_sem would have to be an external task (all direct
holders are already killed or exiting) and it usually have to pin
mm_users in order to do anything reasonable.

We can, however, make sure that the mmput_async is only called when we
do not back off and reap some memory. That would reduce the impact of
the delayed __mmput because the real content would be already freed.
Pin mm_count to keep it alive after we drop task_lock and before we try
to get mmap_sem. If the mmap_sem succeeds we can try to grab mm_users
reference and then go on with unmapping the address space.

It is not clear whether this race is possible at all but it is better
to be more robust and do not pin mm_users unless we are sure we are
actually doing some real work during __oom_reap_task.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/oom_kill.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index c11f8bdd0c12..d4a929d79470 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -452,7 +452,7 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	 * We have to make sure to not race with the victim exit path
 	 * and cause premature new oom victim selection:
 	 * __oom_reap_task		exit_mm
-	 *   atomic_inc_not_zero
+	 *   mmget_not_zero
 	 *				  mmput
 	 *				    atomic_dec_and_test
 	 *				  exit_oom_victim
@@ -474,12 +474,22 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	if (!p)
 		goto unlock_oom;
 	mm = p->mm;
-	atomic_inc(&mm->mm_users);
+	atomic_inc(&mm->mm_count);
 	task_unlock(p);
 
 	if (!down_read_trylock(&mm->mmap_sem)) {
 		ret = false;
-		goto unlock_oom;
+		goto mm_drop;
+	}
+
+	/*
+	 * 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);
+		goto mm_drop;
 	}
 
 	tlb_gather_mmu(&tlb, mm, 0, -1);
@@ -521,15 +531,16 @@ static bool __oom_reap_task(struct task_struct *tsk)
 	 * to release its memory.
 	 */
 	set_bit(MMF_OOM_REAPED, &mm->flags);
-unlock_oom:
-	mutex_unlock(&oom_lock);
 	/*
 	 * 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.
 	 */
-	if (mm)
-		mmput_async(mm);
+	mmput_async(mm);
+mm_drop:
+	mmdrop(mm);
+unlock_oom:
+	mutex_unlock(&oom_lock);
 	return ret;
 }
 
-- 
2.8.1

Re: [PATCH] mm, oom_reaper: make sure that mmput_async is called only when memory was reaped

[Michal Hocko]

On Tue 07-06-16 20:30:24, kbuild test robot wrote:
[...]
>    mm/oom_kill.c: In function '__oom_reap_task':
> >> mm/oom_kill.c:490:7: warning: passing argument 1 of 'mmget_not_zero' from incompatible pointer type
>      if (!mmget_not_zero(&mm->mm_users)) {
>           ^

Sigh... My rate of screw ups during rebase for last minute changes is
just too high. Sorry about that!
-- 
Michal Hocko
SUSE Labs