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

Groups > linux.kernel > #1340157 > unrolled thread

Re: [PATCH 2/5] oom reaper: handle mlocked pages

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

  Re: [PATCH 2/5] oom reaper: handle mlocked pages David Rientjes <rientjes@google.com> - 2016-02-23 02:40 +0100
    Re: [PATCH 2/5] oom reaper: handle mlocked pages Michal Hocko <mhocko@kernel.org> - 2016-02-23 14:30 +0100
      Re: [PATCH 2/5] oom reaper: handle mlocked pages Hugh Dickins <hughd@google.com> - 2016-02-29 04:20 +0100
        Re: [PATCH 2/5] oom reaper: handle mlocked pages Michal Hocko <mhocko@kernel.org> - 2016-02-29 14:50 +0100

#1340157 — Re: [PATCH 2/5] oom reaper: handle mlocked pages

On Wed, 3 Feb 2016, Michal Hocko wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 9a0e4e5f50b4..840e03986497 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -443,13 +443,6 @@ static bool __oom_reap_vmas(struct mm_struct *mm)
>  			continue;
>  
>  		/*
> -		 * mlocked VMAs require explicit munlocking before unmap.
> -		 * Let's keep it simple here and skip such VMAs.
> -		 */
> -		if (vma->vm_flags & VM_LOCKED)
> -			continue;
> -
> -		/*
>  		 * Only anonymous pages have a good chance to be dropped
>  		 * without additional steps which we cannot afford as we
>  		 * are OOM already.
> @@ -459,9 +452,12 @@ static bool __oom_reap_vmas(struct mm_struct *mm)
>  		 * we do not want to block exit_mmap by keeping mm ref
>  		 * count elevated without a good reason.
>  		 */
> -		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
> +		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED)) {
> +			if (vma->vm_flags & VM_LOCKED)
> +				munlock_vma_pages_all(vma);
>  			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
>  					 &details);
> +		}
>  	}
>  	tlb_finish_mmu(&tlb, 0, -1);
>  	up_read(&mm->mmap_sem);

Are we concerned about munlock_vma_pages_all() taking lock_page() and 
perhaps stalling forever, the same way it would stall in exit_mmap() for 
VM_LOCKED vmas, if another thread has locked the same page and is doing an 
allocation?  I'm wondering if in that case it would be better to do a 
best-effort munlock_vma_pages_all() with trylock_page() and just give up 
on releasing memory from that particular vma.  In that case, there may be 
other memory that can be freed with unmap_page_range() that would handle 
this livelock.

[toc] | [next] | [standalone]

#1340605

On Mon 22-02-16 17:36:07, David Rientjes wrote:
> On Wed, 3 Feb 2016, Michal Hocko wrote:
> 
> > diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> > index 9a0e4e5f50b4..840e03986497 100644
> > --- a/mm/oom_kill.c
> > +++ b/mm/oom_kill.c
> > @@ -443,13 +443,6 @@ static bool __oom_reap_vmas(struct mm_struct *mm)
> >  			continue;
> >  
> >  		/*
> > -		 * mlocked VMAs require explicit munlocking before unmap.
> > -		 * Let's keep it simple here and skip such VMAs.
> > -		 */
> > -		if (vma->vm_flags & VM_LOCKED)
> > -			continue;
> > -
> > -		/*
> >  		 * Only anonymous pages have a good chance to be dropped
> >  		 * without additional steps which we cannot afford as we
> >  		 * are OOM already.
> > @@ -459,9 +452,12 @@ static bool __oom_reap_vmas(struct mm_struct *mm)
> >  		 * we do not want to block exit_mmap by keeping mm ref
> >  		 * count elevated without a good reason.
> >  		 */
> > -		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED))
> > +		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED)) {
> > +			if (vma->vm_flags & VM_LOCKED)
> > +				munlock_vma_pages_all(vma);
> >  			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
> >  					 &details);
> > +		}
> >  	}
> >  	tlb_finish_mmu(&tlb, 0, -1);
> >  	up_read(&mm->mmap_sem);
> 
> Are we concerned about munlock_vma_pages_all() taking lock_page() and 
> perhaps stalling forever, the same way it would stall in exit_mmap() for 
> VM_LOCKED vmas, if another thread has locked the same page and is doing an 
> allocation?

This is a good question. I have checked for that particular case
previously and managed to convinced myself that this is OK(ish).
munlock_vma_pages_range locks only THP pages to prevent from the
parallel split-up AFAICS. And split_huge_page_to_list doesn't seem
to depend on an allocation. It can block on anon_vma lock but I didn't
see any allocation requests from there either. I might be missing
something of course. Do you have any specific path in mind?

> I'm wondering if in that case it would be better to do a 
> best-effort munlock_vma_pages_all() with trylock_page() and just give up 
> on releasing memory from that particular vma.  In that case, there may be 
> other memory that can be freed with unmap_page_range() that would handle 
> this livelock.

I have tried to code it up but I am not really sure the whole churn is
really worth it - unless I am missing something that would really make
the THP case likely to hit in the real life.

Just for the reference this is what I came up with (just compile tested).
---
diff --git a/mm/internal.h b/mm/internal.h
index cac6eb458727..63dcdd60aca8 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -249,11 +249,13 @@ void __vma_link_list(struct mm_struct *mm, struct vm_area_struct *vma,
 #ifdef CONFIG_MMU
 extern long populate_vma_page_range(struct vm_area_struct *vma,
 		unsigned long start, unsigned long end, int *nonblocking);
-extern void munlock_vma_pages_range(struct vm_area_struct *vma,
-			unsigned long start, unsigned long end);
-static inline void munlock_vma_pages_all(struct vm_area_struct *vma)
+
+/* Can fail only if enforce == false */
+extern int munlock_vma_pages_range(struct vm_area_struct *vma,
+			unsigned long start, unsigned long end, bool enforce);
+static inline int munlock_vma_pages_all(struct vm_area_struct *vma, bool enforce)
 {
-	munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end);
+	return munlock_vma_pages_range(vma, vma->vm_start, vma->vm_end, enforce);
 }
 
 /*
diff --git a/mm/mlock.c b/mm/mlock.c
index 96f001041928..934c0f8f8ebc 100644
--- a/mm/mlock.c
+++ b/mm/mlock.c
@@ -431,8 +431,9 @@ static unsigned long __munlock_pagevec_fill(struct pagevec *pvec,
  * and re-mlocked by try_to_{munlock|unmap} before we unmap and
  * free them.  This will result in freeing mlocked pages.
  */
-void munlock_vma_pages_range(struct vm_area_struct *vma,
-			     unsigned long start, unsigned long end)
+int munlock_vma_pages_range(struct vm_area_struct *vma,
+			     unsigned long start, unsigned long end,
+			     bool enforce)
 {
 	vma->vm_flags &= VM_LOCKED_CLEAR_MASK;
 
@@ -460,7 +461,13 @@ void munlock_vma_pages_range(struct vm_area_struct *vma,
 				VM_BUG_ON_PAGE(PageMlocked(page), page);
 				put_page(page); /* follow_page_mask() */
 			} else if (PageTransHuge(page)) {
-				lock_page(page);
+				if (enforce) {
+					lock_page(page);
+				} else if (!trylock_page(page)) {
+					put_page(page);
+					return -EAGAIN;
+				}
+
 				/*
 				 * Any THP page found by follow_page_mask() may
 				 * have gotten split before reaching
@@ -497,6 +504,8 @@ void munlock_vma_pages_range(struct vm_area_struct *vma,
 next:
 		cond_resched();
 	}
+
+	return 0;
 }
 
 /*
@@ -561,7 +570,7 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev,
 	if (lock)
 		vma->vm_flags = newflags;
 	else
-		munlock_vma_pages_range(vma, start, end);
+		munlock_vma_pages_range(vma, start, end, true);
 
 out:
 	*prev = vma;
diff --git a/mm/mmap.c b/mm/mmap.c
index cfc0cdca421e..7c2ed6e7b415 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -2592,7 +2592,7 @@ int do_munmap(struct mm_struct *mm, unsigned long start, size_t len)
 		while (tmp && tmp->vm_start < end) {
 			if (tmp->vm_flags & VM_LOCKED) {
 				mm->locked_vm -= vma_pages(tmp);
-				munlock_vma_pages_all(tmp);
+				munlock_vma_pages_all(tmp, true);
 			}
 			tmp = tmp->vm_next;
 		}
@@ -2683,7 +2683,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size,
 	if (vma->vm_flags & VM_LOCKED) {
 		flags |= MAP_LOCKED;
 		/* drop PG_Mlocked flag for over-mapped range */
-		munlock_vma_pages_range(vma, start, start + size);
+		munlock_vma_pages_range(vma, start, start + size, true);
 	}
 
 	file = get_file(vma->vm_file);
@@ -2825,7 +2825,7 @@ void exit_mmap(struct mm_struct *mm)
 		vma = mm->mmap;
 		while (vma) {
 			if (vma->vm_flags & VM_LOCKED)
-				munlock_vma_pages_all(vma);
+				munlock_vma_pages_all(vma, true);
 			vma = vma->vm_next;
 		}
 	}
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 32ce05b1aa10..09e6f3211f1c 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -473,7 +473,8 @@ static bool __oom_reap_task(struct task_struct *tsk)
 		 */
 		if (vma_is_anonymous(vma) || !(vma->vm_flags & VM_SHARED)) {
 			if (vma->vm_flags & VM_LOCKED)
-				munlock_vma_pages_all(vma);
+				if (munlock_vma_pages_all(vma, false))
+					continue;
 			unmap_page_range(&tlb, vma, vma->vm_start, vma->vm_end,
 					 &details);
 		}

-- 
Michal Hocko
SUSE Labs

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

#1345443

On Tue, 23 Feb 2016, Michal Hocko wrote:
> On Mon 22-02-16 17:36:07, David Rientjes wrote:
> > 
> > Are we concerned about munlock_vma_pages_all() taking lock_page() and 
> > perhaps stalling forever, the same way it would stall in exit_mmap() for 
> > VM_LOCKED vmas, if another thread has locked the same page and is doing an 
> > allocation?
> 
> This is a good question. I have checked for that particular case
> previously and managed to convinced myself that this is OK(ish).
> munlock_vma_pages_range locks only THP pages to prevent from the
> parallel split-up AFAICS.

I think you're mistaken on that: there is also the lock_page()
on every page in Phase 2 of __munlock_pagevec().

> And split_huge_page_to_list doesn't seem
> to depend on an allocation. It can block on anon_vma lock but I didn't
> see any allocation requests from there either. I might be missing
> something of course. Do you have any specific path in mind?
> 
> > I'm wondering if in that case it would be better to do a 
> > best-effort munlock_vma_pages_all() with trylock_page() and just give up 
> > on releasing memory from that particular vma.  In that case, there may be 
> > other memory that can be freed with unmap_page_range() that would handle 
> > this livelock.

I agree with David, that we ought to trylock_page() throughout munlock:
just so long as it gets to do the TestClearPageMlocked without demanding
page lock, the rest is the usual sugarcoating for accurate Mlocked stats,
and leave the rest for reclaim to fix up.

> 
> I have tried to code it up but I am not really sure the whole churn is
> really worth it - unless I am missing something that would really make
> the THP case likely to hit in the real life.

Though I must have known about it forever, it was a shock to see all
those page locks demanded in exit, brought home to us a week or so ago.

The proximate cause in this case was my own change, to defer pte_alloc
to suit huge tmpfs: it had not previously occurred to me that I was
now doing the pte_alloc while __do_fault holds page lock.  Bad Hugh.
But change not yet upstream, so not so urgent for you.

From time immemorial, free_swap_and_cache() and free_swap_cache() only
ever trylock a page, precisely so that they never hold up munmap or exit
(well, if I looked harder, I might find lock ordering reasons too).

> 
> Just for the reference this is what I came up with (just compile tested).

I tried something similar internally (on an earlier kernel).  Like
you I've set that work aside for now, there were quicker ways to fix
the issue at hand.  But it does continue to offend me that munlock
demands all those page locks: so if you don't get back to it before me,
I shall eventually.

I didn't understand why you complicated yours with the "enforce"
arg to munlock_vma_pages_range(): why not just trylock in all cases?

Hugh

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

#1345836

On Sun 28-02-16 19:19:11, Hugh Dickins wrote:
> On Tue, 23 Feb 2016, Michal Hocko wrote:
> > On Mon 22-02-16 17:36:07, David Rientjes wrote:
> > > 
> > > Are we concerned about munlock_vma_pages_all() taking lock_page() and 
> > > perhaps stalling forever, the same way it would stall in exit_mmap() for 
> > > VM_LOCKED vmas, if another thread has locked the same page and is doing an 
> > > allocation?
> > 
> > This is a good question. I have checked for that particular case
> > previously and managed to convinced myself that this is OK(ish).
> > munlock_vma_pages_range locks only THP pages to prevent from the
> > parallel split-up AFAICS.
> 
> I think you're mistaken on that: there is also the lock_page()
> on every page in Phase 2 of __munlock_pagevec().

Ohh, I have missed that one. Thanks for pointing it out!

[...]
> > Just for the reference this is what I came up with (just compile tested).
> 
> I tried something similar internally (on an earlier kernel).  Like
> you I've set that work aside for now, there were quicker ways to fix
> the issue at hand.  But it does continue to offend me that munlock
> demands all those page locks: so if you don't get back to it before me,
> I shall eventually.
> 
> I didn't understand why you complicated yours with the "enforce"
> arg to munlock_vma_pages_range(): why not just trylock in all cases?

Well, I have to confess that I am not really sure I understand all the
consequences of the locking here. It has always been subtle and weird
issues popping up from time to time. So I only wanted to have that
change limitted to the oom_reaper. So I would really appreciate if
somebody more knowledgeable had a look. We can drop the mlock patch for
now.

Thanks for looking into this, Hugh!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web