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

Groups > linux.kernel > #1345807 > unrolled thread

[PATCH 07/18] mm, proc: make clear_refs killable

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 07/18] mm, proc: make clear_refs killable Michal Hocko <mhocko@kernel.org> - 2016-02-29 14:30 +0100
    Re: [PATCH 07/18] mm, proc: make clear_refs killable kbuild test robot <lkp@intel.com> - 2016-02-29 14:50 +0100
    Re: [PATCH 07/18] mm, proc: make clear_refs killable Oleg Nesterov <oleg@redhat.com> - 2016-02-29 18:40 +0100
      Re: [PATCH 07/18] mm, proc: make clear_refs killable Michal Hocko <mhocko@kernel.org> - 2016-02-29 19:00 +0100
        Re: [PATCH 07/18] mm, proc: make clear_refs killable Oleg Nesterov <oleg@redhat.com> - 2016-02-29 19:00 +0100
          Re: [PATCH 07/18] mm, proc: make clear_refs killable Michal Hocko <mhocko@kernel.org> - 2016-02-29 19:10 +0100
    [PATCH] mm, proc: make clear_refs killable Michal Hocko <mhocko@kernel.org> - 2016-02-29 19:10 +0100
      Re: [PATCH] mm, proc: make clear_refs killable Oleg Nesterov <oleg@redhat.com> - 2016-02-29 19:20 +0100
      Re: [PATCH] mm, proc: make clear_refs killable kbuild test robot <lkp@intel.com> - 2016-02-29 21:30 +0100

#1345807 — [PATCH 07/18] mm, proc: make clear_refs killable

From: Michal Hocko <mhocko@suse.com>

CLEAR_REFS_MM_HIWATER_RSS and CLEAR_REFS_SOFT_DIRTY are relying on
mmap_sem for write. If the waiting task gets killed by the oom killer
and it would operate on the current's mm it would block oom_reaper from
asynchronous address space reclaim and reduce the chances of timely OOM
resolving. Wait for the lock in the killable mode and return with EINTR
if the task got killed while waiting. This will also expedite the return
to the userspace and do_exit even if the mm is remote.

Cc: Petr Cermak <petrcermak@chromium.org>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/task_mmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9df431642042..fc303fa1f5c0 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		};
 
 		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
+			if (down_write_killable(&mm->mmap_sem)) {
+				put_task_struct(task);
+				return -EINTR;
+			}
+
 			/*
 			 * Writing 5 to /proc/pid/clear_refs resets the peak
 			 * resident set size to this mm's current rss value.
 			 */
-			down_write(&mm->mmap_sem);
 			reset_mm_hiwater_rss(mm);
 			up_write(&mm->mmap_sem);
 			goto out_mm;
@@ -1043,7 +1047,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 				if (!(vma->vm_flags & VM_SOFTDIRTY))
 					continue;
 				up_read(&mm->mmap_sem);
-				down_write(&mm->mmap_sem);
+				if (down_write_killable(&mm->mmap_sem)) {
+					put_task_struct(task);
+					return -EINTR;
+				}
 				for (vma = mm->mmap; vma; vma = vma->vm_next) {
 					vma->vm_flags &= ~VM_SOFTDIRTY;
 					vma_set_page_prot(vma);
-- 
2.7.0

[toc] | [next] | [standalone]

#1345839

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

Hi Michal,

[auto build test WARNING on next-20160229]
[also build test WARNING on v4.5-rc6]
[cannot apply to drm/drm-next drm-intel/for-linux-next v4.5-rc6 v4.5-rc5 v4.5-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/change-mmap_sem-taken-for-write-killable/20160229-213258
config: i386-randconfig-x003-201609 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:4:0,
                    from arch/x86/include/asm/bug.h:35,
                    from include/linux/bug.h:4,
                    from include/linux/mmdebug.h:4,
                    from include/linux/mm.h:8,
                    from fs/proc/task_mmu.c:1:
   fs/proc/task_mmu.c: In function 'clear_refs_write':
   fs/proc/task_mmu.c:1030:8: error: implicit declaration of function 'down_write_killable' [-Werror=implicit-function-declaration]
       if (down_write_killable(&mm->mmap_sem)) {
           ^
   include/linux/compiler.h:151:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^
>> fs/proc/task_mmu.c:1030:4: note: in expansion of macro 'if'
       if (down_write_killable(&mm->mmap_sem)) {
       ^
   cc1: some warnings being treated as errors

vim +/if +1030 fs/proc/task_mmu.c

  1014		task = get_proc_task(file_inode(file));
  1015		if (!task)
  1016			return -ESRCH;
  1017		mm = get_task_mm(task);
  1018		if (mm) {
  1019			struct clear_refs_private cp = {
  1020				.type = type,
  1021			};
  1022			struct mm_walk clear_refs_walk = {
  1023				.pmd_entry = clear_refs_pte_range,
  1024				.test_walk = clear_refs_test_walk,
  1025				.mm = mm,
  1026				.private = &cp,
  1027			};
  1028	
  1029			if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> 1030				if (down_write_killable(&mm->mmap_sem)) {
  1031					put_task_struct(task);
  1032					return -EINTR;
  1033				}
  1034	
  1035				/*
  1036				 * Writing 5 to /proc/pid/clear_refs resets the peak
  1037				 * resident set size to this mm's current rss value.
  1038				 */

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

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

#1346021

On 02/29, Michal Hocko wrote:
>
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
>  		};
>
>  		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> +			if (down_write_killable(&mm->mmap_sem)) {
> +				put_task_struct(task);
> +				return -EINTR;
> +			}
> +
>  			/*
>  			 * Writing 5 to /proc/pid/clear_refs resets the peak
>  			 * resident set size to this mm's current rss value.
>  			 */
> -			down_write(&mm->mmap_sem);
>  			reset_mm_hiwater_rss(mm);
>  			up_write(&mm->mmap_sem);
>  			goto out_mm;
> @@ -1043,7 +1047,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
>  				if (!(vma->vm_flags & VM_SOFTDIRTY))
>  					continue;
>  				up_read(&mm->mmap_sem);
> -				down_write(&mm->mmap_sem);
> +				if (down_write_killable(&mm->mmap_sem)) {
> +					put_task_struct(task);
> +					return -EINTR;
> +				}

Both lack mmput() afaics. Don't you need "goto out_mm" rather then "return" ? In
this case you do not need put_task_struct().

Oleg.

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

#1346045

On Mon 29-02-16 18:38:45, Oleg Nesterov wrote:
> On 02/29, Michal Hocko wrote:
> >
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> >  		};
> >
> >  		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> > +			if (down_write_killable(&mm->mmap_sem)) {
> > +				put_task_struct(task);
> > +				return -EINTR;
> > +			}
> > +
> >  			/*
> >  			 * Writing 5 to /proc/pid/clear_refs resets the peak
> >  			 * resident set size to this mm's current rss value.
> >  			 */
> > -			down_write(&mm->mmap_sem);
> >  			reset_mm_hiwater_rss(mm);
> >  			up_write(&mm->mmap_sem);
> >  			goto out_mm;
> > @@ -1043,7 +1047,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
> >  				if (!(vma->vm_flags & VM_SOFTDIRTY))
> >  					continue;
> >  				up_read(&mm->mmap_sem);
> > -				down_write(&mm->mmap_sem);
> > +				if (down_write_killable(&mm->mmap_sem)) {
> > +					put_task_struct(task);
> > +					return -EINTR;
> > +				}
> 
> Both lack mmput() afaics. Don't you need "goto out_mm" rather then "return" ?

Of course I need! Thanks for catching that.

> In this case you do not need put_task_struct().

Why not? Both are after get_proc_task which takes a reference to the
task...

I will send an updated patch. Thanks!

-- 
Michal Hocko
SUSE Labs

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

#1346047

On 02/29, Michal Hocko wrote:
>
> On Mon 29-02-16 18:38:45, Oleg Nesterov wrote:
> 
> > In this case you do not need put_task_struct().
> 
> Why not? Both are after get_proc_task which takes a reference to the
> task...

Yes, but we already have put_task_struct(task) in the "out_mm" path, so
"goto out_mm" should work just fine?

Oleg.

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

#1346057

On Mon 29-02-16 18:58:17, Oleg Nesterov wrote:
> On 02/29, Michal Hocko wrote:
> >
> > On Mon 29-02-16 18:38:45, Oleg Nesterov wrote:
> > 
> > > In this case you do not need put_task_struct().
> > 
> > Why not? Both are after get_proc_task which takes a reference to the
> > task...
> 
> Yes, but we already have put_task_struct(task) in the "out_mm" path, so
> "goto out_mm" should work just fine?

OK, got what you mean now. That's what I did and is on the way. I just
thought you mean that put_task_struct is implicit for other reason.

-- 
Michal Hocko
SUSE Labs

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

#1346053 — [PATCH] mm, proc: make clear_refs killable

From: Michal Hocko <mhocko@suse.com>

CLEAR_REFS_MM_HIWATER_RSS and CLEAR_REFS_SOFT_DIRTY are relying on
mmap_sem for write. If the waiting task gets killed by the oom killer
and it would operate on the current's mm it would block oom_reaper from
asynchronous address space reclaim and reduce the chances of timely OOM
resolving. Wait for the lock in the killable mode and return with EINTR
if the task got killed while waiting. This will also expedite the return
to the userspace and do_exit even if the mm is remote.

Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Petr Cermak <petrcermak@chromium.org>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 fs/proc/task_mmu.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9df431642042..bb117356a04e 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 		};
 
 		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
+			if (down_write_killable(&mm->mmap_sem)) {
+				count = -EINTR;
+				goto out_mm;
+			}
+
 			/*
 			 * Writing 5 to /proc/pid/clear_refs resets the peak
 			 * resident set size to this mm's current rss value.
 			 */
-			down_write(&mm->mmap_sem);
 			reset_mm_hiwater_rss(mm);
 			up_write(&mm->mmap_sem);
 			goto out_mm;
@@ -1043,7 +1047,10 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
 				if (!(vma->vm_flags & VM_SOFTDIRTY))
 					continue;
 				up_read(&mm->mmap_sem);
-				down_write(&mm->mmap_sem);
+				if (down_write_killable(&mm->mmap_sem)) {
+					count = -EINTR;
+					goto out_mm;
+				}
 				for (vma = mm->mmap; vma; vma = vma->vm_next) {
 					vma->vm_flags &= ~VM_SOFTDIRTY;
 					vma_set_page_prot(vma);
-- 
2.7.0

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

#1346062 — Re: [PATCH] mm, proc: make clear_refs killable

On 02/29, Michal Hocko wrote:
>
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1027,11 +1027,15 @@ static ssize_t clear_refs_write(struct file *file, const char __user *buf,
>  		};
>
>  		if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> +			if (down_write_killable(&mm->mmap_sem)) {
> +				count = -EINTR;
> +				goto out_mm;
> +			}
> +

We do not even need to change count, userspace won't see it anyway. But I agree
it look more clean this way.

I believe the patch is fine.

Oleg.

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

#1346141 — Re: [PATCH] mm, proc: make clear_refs killable

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

Hi Michal,

[auto build test ERROR on v4.5-rc6]
[also build test ERROR on next-20160229]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-proc-make-clear_refs-killable/20160301-020426
config: x86_64-lkp (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   fs/proc/task_mmu.c: In function 'clear_refs_write':
>> fs/proc/task_mmu.c:1016:4: error: implicit declaration of function 'down_write_killable' [-Werror=implicit-function-declaration]
       if (down_write_killable(&mm->mmap_sem)) {
       ^
   cc1: some warnings being treated as errors

vim +/down_write_killable +1016 fs/proc/task_mmu.c

  1010				.test_walk = clear_refs_test_walk,
  1011				.mm = mm,
  1012				.private = &cp,
  1013			};
  1014	
  1015			if (type == CLEAR_REFS_MM_HIWATER_RSS) {
> 1016				if (down_write_killable(&mm->mmap_sem)) {
  1017					count = -EINTR;
  1018					goto out_mm;
  1019				}

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web