[patch][rfc] mm: have expand_stack honour VM_LOCKED

[patch][rfc] mm: have expand_stack honour VM_LOCKED

[Nick Piggin]

Is this valid?


It appears that direct callers of expand_stack may not properly lock the newly
expanded stack if they don't call make_pages_present (page fault handlers do
this).

Catch all these cases by moving make_pages_present to expand_stack.

Signed-off-by: Nick Piggin <npiggin@suse.de>
---
Index: linux-2.6/mm/mmap.c
===================================================================
--- linux-2.6.orig/mm/mmap.c
+++ linux-2.6/mm/mmap.c
@@ -1590,6 +1590,7 @@ static inline
 #endif
 int expand_upwards(struct vm_area_struct *vma, unsigned long address)
 {
+	unsigned long grow = 0;
 	int error;
 
 	if (!(vma->vm_flags & VM_GROWSUP))
@@ -1619,7 +1620,7 @@ int expand_upwards(struct vm_area_struct
 
 	/* Somebody else might have raced and expanded it already */
 	if (address > vma->vm_end) {
-		unsigned long size, grow;
+		unsigned long size;
 
 		size = address - vma->vm_start;
 		grow = (address - vma->vm_end) >> PAGE_SHIFT;
@@ -1629,6 +1630,11 @@ int expand_upwards(struct vm_area_struct
 			vma->vm_end = address;
 	}
 	anon_vma_unlock(vma);
+
+	if (grow && vma->vm_flags & VM_LOCKED)
+		make_pages_present(vma->vm_end - (grow << PAGE_SHIFT),
+								vma->vm_end);
+
 	return error;
 }
 #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
@@ -1639,6 +1645,7 @@ int expand_upwards(struct vm_area_struct
 static inline int expand_downwards(struct vm_area_struct *vma,
 				   unsigned long address)
 {
+	unsigned long grow = 0;
 	int error;
 
 	/*
@@ -1663,7 +1670,7 @@ static inline int expand_downwards(struc
 
 	/* Somebody else might have raced and expanded it already */
 	if (address < vma->vm_start) {
-		unsigned long size, grow;
+		unsigned long size;
 
 		size = vma->vm_end - address;
 		grow = (vma->vm_start - address) >> PAGE_SHIFT;
@@ -1675,6 +1682,11 @@ static inline int expand_downwards(struc
 		}
 	}
 	anon_vma_unlock(vma);
+
+	if (grow && vma->vm_flags & VM_LOCKED)
+		make_pages_present(vma->vm_start,
+				vma->vm_start + (grow << PAGE_SHIFT));
+
 	return error;
 }
 
@@ -1700,8 +1712,6 @@ find_extend_vma(struct mm_struct *mm, un
 		return vma;
 	if (!prev || expand_stack(prev, addr))
 		return NULL;
-	if (prev->vm_flags & VM_LOCKED)
-		make_pages_present(addr, prev->vm_end);
 	return prev;
 }
 #else
@@ -1727,8 +1737,6 @@ find_extend_vma(struct mm_struct * mm, u
 	start = vma->vm_start;
 	if (expand_stack(vma, addr))
 		return NULL;
-	if (vma->vm_flags & VM_LOCKED)
-		make_pages_present(addr, start);
 	return vma;
 }
 #endif

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[KOSAKI Motohiro]

Hi Nick,

> Is this valid?
> 
> 
> It appears that direct callers of expand_stack may not properly lock the newly
> expanded stack if they don't call make_pages_present (page fault handlers do
> this).

When happend this issue?

I think...

case 1. explit mlock to stack 

   1. mlock to stack
        -> make_pages_present is called via mlock(2).
   2. stack increased
        -> no page fault happened.

case 2. swapout and mlock stack

   1. stack swap out
   2. mlock to stack
        -> the page doesn't swap in at the time.
   3. page fault in the stack
        -> the page swap in
           (no need make_present_page())


So, it seems this patch isn't necessary.




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Nick Piggin]

On Fri, Oct 17, 2008 at 02:41:31PM +0900, KOSAKI Motohiro wrote:
> Hi Nick,
> 
> > Is this valid?
> > 
> > 
> > It appears that direct callers of expand_stack may not properly lock the newly
> > expanded stack if they don't call make_pages_present (page fault handlers do
> > this).
> 
> When happend this issue?
> 
> I think...
> 
> case 1. explit mlock to stack 
> 
>    1. mlock to stack
>         -> make_pages_present is called via mlock(2).
>    2. stack increased
>         -> no page fault happened.
> 
> case 2. swapout and mlock stack
> 
>    1. stack swap out
>    2. mlock to stack
>         -> the page doesn't swap in at the time.
>    3. page fault in the stack
>         -> the page swap in
>            (no need make_present_page())
> 
> 
> So, it seems this patch isn't necessary.

What if you you page fault the stack further than a single page down?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[KOSAKI Motohiro]

> > Hi Nick,
> > 
> > > Is this valid?
> > > 
> > > 
> > > It appears that direct callers of expand_stack may not properly lock the newly
> > > expanded stack if they don't call make_pages_present (page fault handlers do
> > > this).
> > 
> > When happend this issue?
> > 
> > I think...
> > 
> > case 1. explit mlock to stack 
> > 
> >    1. mlock to stack
> >         -> make_pages_present is called via mlock(2).
> >    2. stack increased
> >         -> no page fault happened.
> > 
> > case 2. swapout and mlock stack
> > 
> >    1. stack swap out
> >    2. mlock to stack
> >         -> the page doesn't swap in at the time.
> >    3. page fault in the stack
> >         -> the page swap in
> >            (no need make_present_page())
> > 
> > 
> > So, it seems this patch isn't necessary.
> 
> What if you you page fault the stack further than a single page down?
> 

I see. thanks.

But unfortunately, this patch conflicted against unevictable patch series.
I'll make for -mm version patch few days after if you don't like do that.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[KOSAKI Motohiro]

> I see. thanks.
> 
> But unfortunately, this patch conflicted against unevictable patch series.
> I'll make for -mm version patch few days after if you don't like do that.
                                                              ^^^^
                                                              dislike I do that.

HAHAHA, I am very stupid guy.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Nick Piggin]

On Fri, Oct 17, 2008 at 06:32:07PM +0900, KOSAKI Motohiro wrote:
> > > Hi Nick,
> > > 
> > > > Is this valid?
> > > > 
> > > > 
> > > > It appears that direct callers of expand_stack may not properly lock the newly
> > > > expanded stack if they don't call make_pages_present (page fault handlers do
> > > > this).
> > > 
> > > When happend this issue?
> > > 
> > > I think...
> > > 
> > > case 1. explit mlock to stack 
> > > 
> > >    1. mlock to stack
> > >         -> make_pages_present is called via mlock(2).
> > >    2. stack increased
> > >         -> no page fault happened.
> > > 
> > > case 2. swapout and mlock stack
> > > 
> > >    1. stack swap out
> > >    2. mlock to stack
> > >         -> the page doesn't swap in at the time.
> > >    3. page fault in the stack
> > >         -> the page swap in
> > >            (no need make_present_page())
> > > 
> > > 
> > > So, it seems this patch isn't necessary.
> > 
> > What if you you page fault the stack further than a single page down?
> > 
> 
> I see. thanks.
> 
> But unfortunately, this patch conflicted against unevictable patch series.
> I'll make for -mm version patch few days after if you don't like do that.

Well, it's not a big deal. I just wanted to get some comments to see whether
the patch seems to be valid. Let's wait and see what gets merged in this
window, then I'll resubmit this patch unless anybody sees a problem with it.

Thanks for looking at it,

Nick

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Lee Schermerhorn]

On Fri, 2008-10-17 at 11:37 +0200, Nick Piggin wrote:
> On Fri, Oct 17, 2008 at 06:32:07PM +0900, KOSAKI Motohiro wrote:
> > > > Hi Nick,
> > > > 
> > > > > Is this valid?
> > > > > 
> > > > > 
> > > > > It appears that direct callers of expand_stack may not properly lock the newly
> > > > > expanded stack if they don't call make_pages_present (page fault handlers do
> > > > > this).
> > > > 
> > > > When happend this issue?
> > > > 
> > > > I think...
> > > > 
> > > > case 1. explit mlock to stack 
> > > > 
> > > >    1. mlock to stack
> > > >         -> make_pages_present is called via mlock(2).
> > > >    2. stack increased
> > > >         -> no page fault happened.
> > > > 
> > > > case 2. swapout and mlock stack
> > > > 
> > > >    1. stack swap out
> > > >    2. mlock to stack
> > > >         -> the page doesn't swap in at the time.
> > > >    3. page fault in the stack
> > > >         -> the page swap in
> > > >            (no need make_present_page())
> > > > 
> > > > 
> > > > So, it seems this patch isn't necessary.
> > > 
> > > What if you you page fault the stack further than a single page down?
> > > 
> > 
> > I see. thanks.
> > 
> > But unfortunately, this patch conflicted against unevictable patch series.
> > I'll make for -mm version patch few days after if you don't like do that.
> 
> Well, it's not a big deal. I just wanted to get some comments to see whether
> the patch seems to be valid. Let's wait and see what gets merged in this
> window, then I'll resubmit this patch unless anybody sees a problem with it.
> 
> Thanks for looking at it,

Hmmm.  guess I should drain my inbox--not to mention a couple more cups
of coffee--before responding...

Lee

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Lee Schermerhorn]

On Fri, 2008-10-17 at 07:01 +0200, Nick Piggin wrote:
> Is this valid?
> 
??? not sure I can answer that, however...
> 
> It appears that direct callers of expand_stack may not properly lock the newly
> expanded stack if they don't call make_pages_present (page fault handlers do
> this).
> 
> Catch all these cases by moving make_pages_present to expand_stack.

in current -mm/mmotm [and, I hope, upstream soon?] we've replaced
make_pages_present() with mlock_vma_pages_range() [sound familiar? :)]
when we want to populate an mlocked range.  Vis a vis stack locking, see
find_extend_vma() in mmotm.  The patch is:
mmap-handle-mlocked-pages-during-map-remap-unmap.patch
and the plethora of unfolded tweaks thereto.

If there's any chance of the unevictable lru patches making the .28
merge window [Andrew?], you might want to recast this patch atop mmotm.
But, if you do decide to leave it as make_pages_present(), we'll "cull"
them during reclaim.  We'll just have to do a little extra work that
could have been avoided by using mlock_vma_pages_range() in
expand_stack().  

Lee

> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> Index: linux-2.6/mm/mmap.c
> ===================================================================
> --- linux-2.6.orig/mm/mmap.c
> +++ linux-2.6/mm/mmap.c
> @@ -1590,6 +1590,7 @@ static inline
>  #endif
>  int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>  {
> +	unsigned long grow = 0;
>  	int error;
>  
>  	if (!(vma->vm_flags & VM_GROWSUP))
> @@ -1619,7 +1620,7 @@ int expand_upwards(struct vm_area_struct
>  
>  	/* Somebody else might have raced and expanded it already */
>  	if (address > vma->vm_end) {
> -		unsigned long size, grow;
> +		unsigned long size;
>  
>  		size = address - vma->vm_start;
>  		grow = (address - vma->vm_end) >> PAGE_SHIFT;
> @@ -1629,6 +1630,11 @@ int expand_upwards(struct vm_area_struct
>  			vma->vm_end = address;
>  	}
>  	anon_vma_unlock(vma);
> +
> +	if (grow && vma->vm_flags & VM_LOCKED)
> +		make_pages_present(vma->vm_end - (grow << PAGE_SHIFT),
> +								vma->vm_end);
> +
>  	return error;
>  }
>  #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
> @@ -1639,6 +1645,7 @@ int expand_upwards(struct vm_area_struct
>  static inline int expand_downwards(struct vm_area_struct *vma,
>  				   unsigned long address)
>  {
> +	unsigned long grow = 0;
>  	int error;
>  
>  	/*
> @@ -1663,7 +1670,7 @@ static inline int expand_downwards(struc
>  
>  	/* Somebody else might have raced and expanded it already */
>  	if (address < vma->vm_start) {
> -		unsigned long size, grow;
> +		unsigned long size;
>  
>  		size = vma->vm_end - address;
>  		grow = (vma->vm_start - address) >> PAGE_SHIFT;
> @@ -1675,6 +1682,11 @@ static inline int expand_downwards(struc
>  		}
>  	}
>  	anon_vma_unlock(vma);
> +
> +	if (grow && vma->vm_flags & VM_LOCKED)
> +		make_pages_present(vma->vm_start,
> +				vma->vm_start + (grow << PAGE_SHIFT));
> +
>  	return error;
>  }
>  
> @@ -1700,8 +1712,6 @@ find_extend_vma(struct mm_struct *mm, un
>  		return vma;
>  	if (!prev || expand_stack(prev, addr))
>  		return NULL;
> -	if (prev->vm_flags & VM_LOCKED)
> -		make_pages_present(addr, prev->vm_end);
>  	return prev;
>  }
>  #else
> @@ -1727,8 +1737,6 @@ find_extend_vma(struct mm_struct * mm, u
>  	start = vma->vm_start;
>  	if (expand_stack(vma, addr))
>  		return NULL;
> -	if (vma->vm_flags & VM_LOCKED)
> -		make_pages_present(addr, start);
>  	return vma;
>  }
>  #endif
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Hugh Dickins]

On Fri, 17 Oct 2008, Nick Piggin wrote:

> Is this valid?

Well.  I find it really hard to get excited about the case of
a stack fault more than a page below the current stack pointer not
faulting in the intervening untouched pages when the stack is mlocked.

(That's what it amounts to, isn't it? though your description doesn't
make that at all clear.)

Do you have a case where it actually matters e.g. does get_user_pages
or something like it assume that every page in a VM_LOCKED area must
already be present?  Or do you worry that we might easily add such
an assumption?

I don't think your patch is wrong, but I'd feel a wee bit safer just
to leave things as is: somehow, I prefer the idea of the arch fault
routines faulting in the (normal case) one page for themselves, than
it happening underneath them in make_pages_present's get_user_pages.

One minor (ha ha) defect of doing it your way is that the minor fault
will get counted twice.

But I don't feel strongly about it.

Hugh

> 
> It appears that direct callers of expand_stack may not properly lock the newly
> expanded stack if they don't call make_pages_present (page fault handlers do
> this).

They do the don't.

> 
> Catch all these cases by moving make_pages_present to expand_stack.
> 
> Signed-off-by: Nick Piggin <npiggin@suse.de>
> ---
> Index: linux-2.6/mm/mmap.c
> ===================================================================
> --- linux-2.6.orig/mm/mmap.c
> +++ linux-2.6/mm/mmap.c
> @@ -1590,6 +1590,7 @@ static inline
>  #endif
>  int expand_upwards(struct vm_area_struct *vma, unsigned long address)
>  {
> +	unsigned long grow = 0;
>  	int error;
>  
>  	if (!(vma->vm_flags & VM_GROWSUP))
> @@ -1619,7 +1620,7 @@ int expand_upwards(struct vm_area_struct
>  
>  	/* Somebody else might have raced and expanded it already */
>  	if (address > vma->vm_end) {
> -		unsigned long size, grow;
> +		unsigned long size;
>  
>  		size = address - vma->vm_start;
>  		grow = (address - vma->vm_end) >> PAGE_SHIFT;
> @@ -1629,6 +1630,11 @@ int expand_upwards(struct vm_area_struct
>  			vma->vm_end = address;
>  	}
>  	anon_vma_unlock(vma);
> +
> +	if (grow && vma->vm_flags & VM_LOCKED)
> +		make_pages_present(vma->vm_end - (grow << PAGE_SHIFT),
> +								vma->vm_end);
> +
>  	return error;
>  }
>  #endif /* CONFIG_STACK_GROWSUP || CONFIG_IA64 */
> @@ -1639,6 +1645,7 @@ int expand_upwards(struct vm_area_struct
>  static inline int expand_downwards(struct vm_area_struct *vma,
>  				   unsigned long address)
>  {
> +	unsigned long grow = 0;
>  	int error;
>  
>  	/*
> @@ -1663,7 +1670,7 @@ static inline int expand_downwards(struc
>  
>  	/* Somebody else might have raced and expanded it already */
>  	if (address < vma->vm_start) {
> -		unsigned long size, grow;
> +		unsigned long size;
>  
>  		size = vma->vm_end - address;
>  		grow = (vma->vm_start - address) >> PAGE_SHIFT;
> @@ -1675,6 +1682,11 @@ static inline int expand_downwards(struc
>  		}
>  	}
>  	anon_vma_unlock(vma);
> +
> +	if (grow && vma->vm_flags & VM_LOCKED)
> +		make_pages_present(vma->vm_start,
> +				vma->vm_start + (grow << PAGE_SHIFT));
> +
>  	return error;
>  }
>  
> @@ -1700,8 +1712,6 @@ find_extend_vma(struct mm_struct *mm, un
>  		return vma;
>  	if (!prev || expand_stack(prev, addr))
>  		return NULL;
> -	if (prev->vm_flags & VM_LOCKED)
> -		make_pages_present(addr, prev->vm_end);
>  	return prev;
>  }
>  #else
> @@ -1727,8 +1737,6 @@ find_extend_vma(struct mm_struct * mm, u
>  	start = vma->vm_start;
>  	if (expand_stack(vma, addr))
>  		return NULL;
> -	if (vma->vm_flags & VM_LOCKED)
> -		make_pages_present(addr, start);
>  	return vma;
>  }
>  #endif

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Nick Piggin]

On Fri, Oct 17, 2008 at 02:42:56PM +0100, Hugh Dickins wrote:
> On Fri, 17 Oct 2008, Nick Piggin wrote:
> 
> > Is this valid?
> 
> Well.  I find it really hard to get excited about the case of
> a stack fault more than a page below the current stack pointer not
> faulting in the intervening untouched pages when the stack is mlocked.
> 
> (That's what it amounts to, isn't it? though your description doesn't
> make that at all clear.)
> 
> Do you have a case where it actually matters e.g. does get_user_pages
> or something like it assume that every page in a VM_LOCKED area must
> already be present?  Or do you worry that we might easily add such
> an assumption?
> 
> I don't think your patch is wrong, but I'd feel a wee bit safer just
> to leave things as is: somehow, I prefer the idea of the arch fault
> routines faulting in the (normal case) one page for themselves, than
> it happening underneath them in make_pages_present's get_user_pages.
> 
> One minor (ha ha) defect of doing it your way is that the minor fault
> will get counted twice.
> 
> But I don't feel strongly about it.

No, no "real" case in mind, I was just looking at the code.

How do critical apps reserve and lock a required amount of stack? I
thought there might be cases where failing to lock pages could cause
problems there.

Minor faults... good spotting :) I don't think I'd worry about that
yet.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch][rfc] mm: have expand_stack honour VM_LOCKED

[Hugh Dickins]

On Fri, 17 Oct 2008, Nick Piggin wrote:
> 
> How do critical apps reserve and lock a required amount of stack? I
> thought there might be cases where failing to lock pages could cause
> problems there.

I'd have thought that an app that was desperate to avoid even minor
faults would be mlocking all the stack it might ever need later on:
not extending its stack with (one or more) minor faults but relying
on those to prevent minor faults on the gaps.

Ah, you think it might mlock the stack it already has, then touch a
lower address, and expect that to fault in the intervening pages:
well, I suppose it might expect that, but we've never done it,
so we'd have been causing it unnoticed problems for many years.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>