[patch]vmscan: protect exectuable page from inactive list scan

[patch]vmscan: protect exectuable page from inactive list scan

[Shaohua Li]

With commit 645747462435, pte referenced file page isn't activated in inactive
list scan. For VM_EXEC page, if it can't get a chance to active list, the
executable page protect loses its effect. We protect such page in inactive scan
here, now such page will be guaranteed cached in a full scan of active and
inactive list, which restores previous behavior.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/mm/vmscan.c b/mm/vmscan.c
index c5dfabf..b973048 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
 		 * quickly recovered.
 		 */
 		SetPageReferenced(page);
-
-		if (referenced_page)
+		/*
+		 * Identify pte referenced and file-backed pages and give them
+		 * one trip around the active list. So that executable code get
+		 * better chances to stay in memory under moderate memory
+		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
+		 * ignore them here.
+		 */
+		if (referenced_page || ((vm_flags & VM_EXEC) &&
+		    page_is_file_cache(page)))
 			return PAGEREF_ACTIVATE;
 
 		return PAGEREF_KEEP;


--
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]vmscan: protect exectuable page from inactive list scan

[Johannes Weiner]

On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> With commit 645747462435, pte referenced file page isn't activated in inactive
> list scan. For VM_EXEC page, if it can't get a chance to active list, the
> executable page protect loses its effect. We protect such page in inactive scan
> here, now such page will be guaranteed cached in a full scan of active and
> inactive list, which restores previous behavior.

This change was in the back of my head since the used-once detection
was merged but there were never any regressions reported that would
indicate a requirement for it.

Does this patch fix a problem you observed?

> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
>  		 * quickly recovered.
>  		 */
>  		SetPageReferenced(page);
> -
> -		if (referenced_page)
> +		/*
> +		 * Identify pte referenced and file-backed pages and give them
> +		 * one trip around the active list. So that executable code get
> +		 * better chances to stay in memory under moderate memory
> +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> +		 * ignore them here.

PTE-referenced PageAnon() pages are activated unconditionally a few
lines further up, so the page_is_file_cache() check filters only shmem
pages.  I doubt this was your intention...?

> +		 */
> +		if (referenced_page || ((vm_flags & VM_EXEC) &&
> +		    page_is_file_cache(page)))
>  			return PAGEREF_ACTIVATE;
>  
>  		return PAGEREF_KEEP;

--
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]vmscan: protect exectuable page from inactive list scan

[Shaohua Li]

On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > With commit 645747462435, pte referenced file page isn't activated in inactive
> > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > executable page protect loses its effect. We protect such page in inactive scan
> > here, now such page will be guaranteed cached in a full scan of active and
> > inactive list, which restores previous behavior.
> 
> This change was in the back of my head since the used-once detection
> was merged but there were never any regressions reported that would
> indicate a requirement for it.
The executable page protect is to improve responsibility. I would expect
it's hard for user to report such regression. 

> Does this patch fix a problem you observed?
No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.

> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> >  		 * quickly recovered.
> >  		 */
> >  		SetPageReferenced(page);
> > -
> > -		if (referenced_page)
> > +		/*
> > +		 * Identify pte referenced and file-backed pages and give them
> > +		 * one trip around the active list. So that executable code get
> > +		 * better chances to stay in memory under moderate memory
> > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > +		 * ignore them here.
> 
> PTE-referenced PageAnon() pages are activated unconditionally a few
> lines further up, so the page_is_file_cache() check filters only shmem
> pages.  I doubt this was your intention...?
This is intented. the executable page protect is just to protect
executable file pages. please see 8cab4754d24a0f.

--
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]vmscan: protect exectuable page from inactive list scan

[KOSAKI Motohiro]

> On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > executable page protect loses its effect. We protect such page in inactive scan
> > > here, now such page will be guaranteed cached in a full scan of active and
> > > inactive list, which restores previous behavior.
> > 
> > This change was in the back of my head since the used-once detection
> > was merged but there were never any regressions reported that would
> > indicate a requirement for it.
> The executable page protect is to improve responsibility. I would expect
> it's hard for user to report such regression. 

Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
So, I wonder why current people can't feel the same lag if it is.


> > Does this patch fix a problem you observed?
> No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.

But, I am usually not against a number. If you will finished to test them I'm happy :)


> 
> > > --- a/mm/vmscan.c
> > > +++ b/mm/vmscan.c
> > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > >  		 * quickly recovered.
> > >  		 */
> > >  		SetPageReferenced(page);
> > > -
> > > -		if (referenced_page)
> > > +		/*
> > > +		 * Identify pte referenced and file-backed pages and give them
> > > +		 * one trip around the active list. So that executable code get
> > > +		 * better chances to stay in memory under moderate memory
> > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > +		 * ignore them here.
> > 
> > PTE-referenced PageAnon() pages are activated unconditionally a few
> > lines further up, so the page_is_file_cache() check filters only shmem
> > pages.  I doubt this was your intention...?
> This is intented. the executable page protect is just to protect
> executable file pages. please see 8cab4754d24a0f.

8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
8cab4754d24a0f doesn't tell us the reason of the change, no?



--
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]vmscan: protect exectuable page from inactive list scan

[Wu Fengguang]

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

On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > inactive list, which restores previous behavior.
> > > 
> > > This change was in the back of my head since the used-once detection
> > > was merged but there were never any regressions reported that would
> > > indicate a requirement for it.
> > The executable page protect is to improve responsibility. I would expect
> > it's hard for user to report such regression. 
> 
> Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> So, I wonder why current people can't feel the same lag if it is.
> 
> 
> > > Does this patch fix a problem you observed?
> > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> 
> But, I am usually not against a number. If you will finished to test them I'm happy :)

Yeah, it needs good numbers for adding such special case code.
I attached the scripts used for 8cab4754d24a0f, hope this helps.

Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
as an indicator whether the extra logic is enabled. This is a convenient
trick I sometimes play with new code:

+                       extern int suid_dumpable;
+                       if (suid_dumpable)
                        if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
                                list_add(&page->lru, &l_active);
                                continue;

> > 
> > > > --- a/mm/vmscan.c
> > > > +++ b/mm/vmscan.c
> > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > >  		 * quickly recovered.
> > > >  		 */
> > > >  		SetPageReferenced(page);
> > > > -
> > > > -		if (referenced_page)
> > > > +		/*
> > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > +		 * one trip around the active list. So that executable code get
> > > > +		 * better chances to stay in memory under moderate memory
> > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > +		 * ignore them here.
> > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > +                   page_is_file_cache(page)))
> > > >                         return PAGEREF_ACTIVATE;

> > > 
> > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > lines further up, so the page_is_file_cache() check filters only shmem
> > > pages.  I doubt this was your intention...?
> > This is intented. the executable page protect is just to protect
> > executable file pages. please see 8cab4754d24a0f.
> 
> 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> 8cab4754d24a0f doesn't tell us the reason of the change, no?

What if the executable file happen to be on tmpfs?  The !PageAnon()
test also covers that case. The page_is_file_cache() test here seems
unnecessary. And it looks better to move the VM_EXEC test above the
SetPageReferenced() line to avoid possible side effects.

Thanks,
Fengguang

[-- Attachment #2: run-many-x-apps.sh --]
[-- Type: application/x-sh, Size: 1751 bytes --]

[-- Attachment #3: test-mmap-exec-prot.sh --]
[-- Type: application/x-sh, Size: 220 bytes --]

[-- Attachment #4: iotrace.rb --]
[-- Type: application/x-ruby, Size: 8999 bytes --]

Re: [patch]vmscan: protect exectuable page from inactive list scan

[KOSAKI Motohiro]

> > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > pages.  I doubt this was your intention...?
> > > This is intented. the executable page protect is just to protect
> > > executable file pages. please see 8cab4754d24a0f.
> > 
> > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> 
> What if the executable file happen to be on tmpfs?  The !PageAnon()
> test also covers that case. The page_is_file_cache() test here seems
> unnecessary. And it looks better to move the VM_EXEC test above the
> SetPageReferenced() line to avoid possible side effects.

Both agree :)



--
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]vmscan: protect exectuable page from inactive list scan

[Shaohua Li]

On Thu, 2010-09-30 at 10:57 +0800, Wu, Fengguang wrote:
> On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > > inactive list, which restores previous behavior.
> > > > 
> > > > This change was in the back of my head since the used-once detection
> > > > was merged but there were never any regressions reported that would
> > > > indicate a requirement for it.
> > > The executable page protect is to improve responsibility. I would expect
> > > it's hard for user to report such regression. 
> > 
> > Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> > So, I wonder why current people can't feel the same lag if it is.
> > 
> > 
> > > > Does this patch fix a problem you observed?
> > > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> > 
> > But, I am usually not against a number. If you will finished to test them I'm happy :)
> 
> Yeah, it needs good numbers for adding such special case code.
> I attached the scripts used for 8cab4754d24a0f, hope this helps.
> 
> Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
> as an indicator whether the extra logic is enabled. This is a convenient
> trick I sometimes play with new code:
> 
> +                       extern int suid_dumpable;
> +                       if (suid_dumpable)
>                         if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
>                                 list_add(&page->lru, &l_active);
>                                 continue;
ok, I'll test them, but might a little later, after a 7-day holiday.

> > > 
> > > > > --- a/mm/vmscan.c
> > > > > +++ b/mm/vmscan.c
> > > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > > >  		 * quickly recovered.
> > > > >  		 */
> > > > >  		SetPageReferenced(page);
> > > > > -
> > > > > -		if (referenced_page)
> > > > > +		/*
> > > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > > +		 * one trip around the active list. So that executable code get
> > > > > +		 * better chances to stay in memory under moderate memory
> > > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > > +		 * ignore them here.
> > > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > > +                   page_is_file_cache(page)))
> > > > >                         return PAGEREF_ACTIVATE;
> 
> > > > 
> > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > pages.  I doubt this was your intention...?
> > > This is intented. the executable page protect is just to protect
> > > executable file pages. please see 8cab4754d24a0f.
> > 
> > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> 
> What if the executable file happen to be on tmpfs?  The !PageAnon()
> test also covers that case. The page_is_file_cache() test here seems
> unnecessary. And it looks better to move the VM_EXEC test above the
> SetPageReferenced() line to avoid possible side effects.
oops, I should mention this commit 41e20983fe553 here. That commit
changes it to page_is_file_cache()

--
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]vmscan: protect exectuable page from inactive list scan

[Wu Fengguang]

On Thu, Sep 30, 2010 at 11:20:45AM +0800, Li, Shaohua wrote:
> On Thu, 2010-09-30 at 10:57 +0800, Wu, Fengguang wrote:
> > On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > > > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > > > inactive list, which restores previous behavior.
> > > > > 
> > > > > This change was in the back of my head since the used-once detection
> > > > > was merged but there were never any regressions reported that would
> > > > > indicate a requirement for it.
> > > > The executable page protect is to improve responsibility. I would expect
> > > > it's hard for user to report such regression. 
> > > 
> > > Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> > > So, I wonder why current people can't feel the same lag if it is.
> > > 
> > > 
> > > > > Does this patch fix a problem you observed?
> > > > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> > > 
> > > But, I am usually not against a number. If you will finished to test them I'm happy :)
> > 
> > Yeah, it needs good numbers for adding such special case code.
> > I attached the scripts used for 8cab4754d24a0f, hope this helps.
> > 
> > Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
> > as an indicator whether the extra logic is enabled. This is a convenient
> > trick I sometimes play with new code:
> > 
> > +                       extern int suid_dumpable;
> > +                       if (suid_dumpable)
> >                         if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
> >                                 list_add(&page->lru, &l_active);
> >                                 continue;
> ok, I'll test them, but might a little later, after a 7-day holiday.

Have a good time :)

> > > > 
> > > > > > --- a/mm/vmscan.c
> > > > > > +++ b/mm/vmscan.c
> > > > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > > > >  		 * quickly recovered.
> > > > > >  		 */
> > > > > >  		SetPageReferenced(page);
> > > > > > -
> > > > > > -		if (referenced_page)
> > > > > > +		/*
> > > > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > > > +		 * one trip around the active list. So that executable code get
> > > > > > +		 * better chances to stay in memory under moderate memory
> > > > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > > > +		 * ignore them here.
> > > > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > > > +                   page_is_file_cache(page)))
> > > > > >                         return PAGEREF_ACTIVATE;
> > 
> > > > > 
> > > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > > pages.  I doubt this was your intention...?
> > > > This is intented. the executable page protect is just to protect
> > > > executable file pages. please see 8cab4754d24a0f.
> > > 
> > > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> > 
> > What if the executable file happen to be on tmpfs?  The !PageAnon()
> > test also covers that case. The page_is_file_cache() test here seems
> > unnecessary. And it looks better to move the VM_EXEC test above the
> > SetPageReferenced() line to avoid possible side effects.
> oops, I should mention this commit 41e20983fe553 here. That commit
> changes it to page_is_file_cache()

Oops, forgot about that "real but beyond my imagination" corner case..

Thanks,
Fengguang

--
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]vmscan: protect exectuable page from inactive list scan

[KOSAKI Motohiro]

> On Thu, 2010-09-30 at 10:57 +0800, Wu, Fengguang wrote:
> > On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > > > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > > > inactive list, which restores previous behavior.
> > > > > 
> > > > > This change was in the back of my head since the used-once detection
> > > > > was merged but there were never any regressions reported that would
> > > > > indicate a requirement for it.
> > > > The executable page protect is to improve responsibility. I would expect
> > > > it's hard for user to report such regression. 
> > > 
> > > Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> > > So, I wonder why current people can't feel the same lag if it is.
> > > 
> > > 
> > > > > Does this patch fix a problem you observed?
> > > > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> > > 
> > > But, I am usually not against a number. If you will finished to test them I'm happy :)
> > 
> > Yeah, it needs good numbers for adding such special case code.
> > I attached the scripts used for 8cab4754d24a0f, hope this helps.
> > 
> > Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
> > as an indicator whether the extra logic is enabled. This is a convenient
> > trick I sometimes play with new code:
> > 
> > +                       extern int suid_dumpable;
> > +                       if (suid_dumpable)
> >                         if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
> >                                 list_add(&page->lru, &l_active);
> >                                 continue;
> ok, I'll test them, but might a little later, after a 7-day holiday.
> 
> > > > 
> > > > > > --- a/mm/vmscan.c
> > > > > > +++ b/mm/vmscan.c
> > > > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > > > >  		 * quickly recovered.
> > > > > >  		 */
> > > > > >  		SetPageReferenced(page);
> > > > > > -
> > > > > > -		if (referenced_page)
> > > > > > +		/*
> > > > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > > > +		 * one trip around the active list. So that executable code get
> > > > > > +		 * better chances to stay in memory under moderate memory
> > > > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > > > +		 * ignore them here.
> > > > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > > > +                   page_is_file_cache(page)))
> > > > > >                         return PAGEREF_ACTIVATE;
> > 
> > > > > 
> > > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > > pages.  I doubt this was your intention...?
> > > > This is intented. the executable page protect is just to protect
> > > > executable file pages. please see 8cab4754d24a0f.
> > > 
> > > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> > 
> > What if the executable file happen to be on tmpfs?  The !PageAnon()
> > test also covers that case. The page_is_file_cache() test here seems
> > unnecessary. And it looks better to move the VM_EXEC test above the
> > SetPageReferenced() line to avoid possible side effects.
> oops, I should mention this commit 41e20983fe553 here. That commit
> changes it to page_is_file_cache()

Hmmm...

I think 41e20983fe553 is red herring fix. because 1) Even if all pages are
VM_EXEC, we don't have to make OOM anyway. tmpfs or not is not good
decision source. (note: On embedded, regular file-system can be smaller than tmpfs)
2) We've already fixed tmpfs used once page issue. (e9d6c15738 and 
vmscantmpfs-treat-used-once-pages-on-tmpfs-as-used-once.patch in -mm)

Is the issue still exist?


--
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]vmscan: protect exectuable page from inactive list scan

[Shaohua Li]

On Thu, 2010-09-30 at 12:46 +0800, KOSAKI Motohiro wrote:
> > On Thu, 2010-09-30 at 10:57 +0800, Wu, Fengguang wrote:
> > > On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > > > > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > > > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > > > > inactive list, which restores previous behavior.
> > > > > > 
> > > > > > This change was in the back of my head since the used-once detection
> > > > > > was merged but there were never any regressions reported that would
> > > > > > indicate a requirement for it.
> > > > > The executable page protect is to improve responsibility. I would expect
> > > > > it's hard for user to report such regression. 
> > > > 
> > > > Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> > > > So, I wonder why current people can't feel the same lag if it is.
> > > > 
> > > > 
> > > > > > Does this patch fix a problem you observed?
> > > > > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> > > > 
> > > > But, I am usually not against a number. If you will finished to test them I'm happy :)
> > > 
> > > Yeah, it needs good numbers for adding such special case code.
> > > I attached the scripts used for 8cab4754d24a0f, hope this helps.
> > > 
> > > Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
> > > as an indicator whether the extra logic is enabled. This is a convenient
> > > trick I sometimes play with new code:
> > > 
> > > +                       extern int suid_dumpable;
> > > +                       if (suid_dumpable)
> > >                         if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
> > >                                 list_add(&page->lru, &l_active);
> > >                                 continue;
> > ok, I'll test them, but might a little later, after a 7-day holiday.
> > 
> > > > > 
> > > > > > > --- a/mm/vmscan.c
> > > > > > > +++ b/mm/vmscan.c
> > > > > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > > > > >  		 * quickly recovered.
> > > > > > >  		 */
> > > > > > >  		SetPageReferenced(page);
> > > > > > > -
> > > > > > > -		if (referenced_page)
> > > > > > > +		/*
> > > > > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > > > > +		 * one trip around the active list. So that executable code get
> > > > > > > +		 * better chances to stay in memory under moderate memory
> > > > > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > > > > +		 * ignore them here.
> > > > > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > > > > +                   page_is_file_cache(page)))
> > > > > > >                         return PAGEREF_ACTIVATE;
> > > 
> > > > > > 
> > > > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > > > pages.  I doubt this was your intention...?
> > > > > This is intented. the executable page protect is just to protect
> > > > > executable file pages. please see 8cab4754d24a0f.
> > > > 
> > > > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > > > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> > > 
> > > What if the executable file happen to be on tmpfs?  The !PageAnon()
> > > test also covers that case. The page_is_file_cache() test here seems
> > > unnecessary. And it looks better to move the VM_EXEC test above the
> > > SetPageReferenced() line to avoid possible side effects.
> > oops, I should mention this commit 41e20983fe553 here. That commit
> > changes it to page_is_file_cache()
> 
> Hmmm...
> 
> I think 41e20983fe553 is red herring fix. because 1) Even if all pages are
> VM_EXEC, we don't have to make OOM anyway. tmpfs or not is not good
> decision source. (note: On embedded, regular file-system can be smaller than tmpfs)
> 2) We've already fixed tmpfs used once page issue. (e9d6c15738 and 
> vmscantmpfs-treat-used-once-pages-on-tmpfs-as-used-once.patch in -mm)
IIRC, e9d6c15738 solved the used once issue at the time when the page is
added to lru list at the first time. while this issue is moving a page
from inactive list to active list or give the page another around to
active list. if we have no the filter, moving vast executable tmpfs
pages to activate list can still increasing anon list rotate rate and
cause more file pages scan and oom killer.

--
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]vmscan: protect exectuable page from inactive list scan

[KOSAKI Motohiro]

> On Thu, 2010-09-30 at 12:46 +0800, KOSAKI Motohiro wrote:
> > > On Thu, 2010-09-30 at 10:57 +0800, Wu, Fengguang wrote:
> > > > On Thu, Sep 30, 2010 at 10:27:04AM +0800, KOSAKI Motohiro wrote:
> > > > > > On Wed, 2010-09-29 at 18:17 +0800, Johannes Weiner wrote:
> > > > > > > On Wed, Sep 29, 2010 at 10:57:40AM +0800, Shaohua Li wrote:
> > > > > > > > With commit 645747462435, pte referenced file page isn't activated in inactive
> > > > > > > > list scan. For VM_EXEC page, if it can't get a chance to active list, the
> > > > > > > > executable page protect loses its effect. We protect such page in inactive scan
> > > > > > > > here, now such page will be guaranteed cached in a full scan of active and
> > > > > > > > inactive list, which restores previous behavior.
> > > > > > > 
> > > > > > > This change was in the back of my head since the used-once detection
> > > > > > > was merged but there were never any regressions reported that would
> > > > > > > indicate a requirement for it.
> > > > > > The executable page protect is to improve responsibility. I would expect
> > > > > > it's hard for user to report such regression. 
> > > > > 
> > > > > Seems strange. 8cab4754d24a0f was introduced for fixing real world problem.
> > > > > So, I wonder why current people can't feel the same lag if it is.
> > > > > 
> > > > > 
> > > > > > > Does this patch fix a problem you observed?
> > > > > > No, I haven't done test where Fengguang does in commit 8cab4754d24a0f.
> > > > > 
> > > > > But, I am usually not against a number. If you will finished to test them I'm happy :)
> > > > 
> > > > Yeah, it needs good numbers for adding such special case code.
> > > > I attached the scripts used for 8cab4754d24a0f, hope this helps.
> > > > 
> > > > Note that the test-mmap-exec-prot.sh used /proc/sys/fs/suid_dumpable
> > > > as an indicator whether the extra logic is enabled. This is a convenient
> > > > trick I sometimes play with new code:
> > > > 
> > > > +                       extern int suid_dumpable;
> > > > +                       if (suid_dumpable)
> > > >                         if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
> > > >                                 list_add(&page->lru, &l_active);
> > > >                                 continue;
> > > ok, I'll test them, but might a little later, after a 7-day holiday.
> > > 
> > > > > > 
> > > > > > > > --- a/mm/vmscan.c
> > > > > > > > +++ b/mm/vmscan.c
> > > > > > > > @@ -608,8 +608,15 @@ static enum page_references page_check_references(struct page *page,
> > > > > > > >  		 * quickly recovered.
> > > > > > > >  		 */
> > > > > > > >  		SetPageReferenced(page);
> > > > > > > > -
> > > > > > > > -		if (referenced_page)
> > > > > > > > +		/*
> > > > > > > > +		 * Identify pte referenced and file-backed pages and give them
> > > > > > > > +		 * one trip around the active list. So that executable code get
> > > > > > > > +		 * better chances to stay in memory under moderate memory
> > > > > > > > +		 * pressure. JVM can create lots of anon VM_EXEC pages, so we
> > > > > > > > +		 * ignore them here.
> > > > > > > > +               if (referenced_page || ((vm_flags & VM_EXEC) &&
> > > > > > > > +                   page_is_file_cache(page)))
> > > > > > > >                         return PAGEREF_ACTIVATE;
> > > > 
> > > > > > > 
> > > > > > > PTE-referenced PageAnon() pages are activated unconditionally a few
> > > > > > > lines further up, so the page_is_file_cache() check filters only shmem
> > > > > > > pages.  I doubt this was your intention...?
> > > > > > This is intented. the executable page protect is just to protect
> > > > > > executable file pages. please see 8cab4754d24a0f.
> > > > > 
> > > > > 8cab4754d24a0f was using !PageAnon() but your one are using page_is_file_cache.
> > > > > 8cab4754d24a0f doesn't tell us the reason of the change, no?
> > > > 
> > > > What if the executable file happen to be on tmpfs?  The !PageAnon()
> > > > test also covers that case. The page_is_file_cache() test here seems
> > > > unnecessary. And it looks better to move the VM_EXEC test above the
> > > > SetPageReferenced() line to avoid possible side effects.
> > > oops, I should mention this commit 41e20983fe553 here. That commit
> > > changes it to page_is_file_cache()
> > 
> > Hmmm...
> > 
> > I think 41e20983fe553 is red herring fix. because 1) Even if all pages are
> > VM_EXEC, we don't have to make OOM anyway. tmpfs or not is not good
> > decision source. (note: On embedded, regular file-system can be smaller than tmpfs)
> > 2) We've already fixed tmpfs used once page issue. (e9d6c15738 and 
> > vmscantmpfs-treat-used-once-pages-on-tmpfs-as-used-once.patch in -mm)
> IIRC, e9d6c15738 solved the used once issue at the time when the page is
> added to lru list at the first time. while this issue is moving a page
> from inactive list to active list or give the page another around to
> active list. if we have no the filter, moving vast executable tmpfs
> pages to activate list can still increasing anon list rotate rate and
> cause more file pages scan and oom killer.

before invoking OOM, we scan pages about two times between priority-12
and priority-0. then, one time activation is harmless. but two times
activation and/or another rotate factor can be made trouble.

before vmscantmpfs-treat-used-once-pages-on-tmpfs-as-used-once.patch,
tmpfs pages are always rotated at least once. and VM_EXEC add +1 time.
Thus, two times rotate occur and bring to trouble. I think.

If this is not enough solution, of cource we need additional fix. but
tmpfs is not special. It's fs independent issue. 


However I agree page_is_file_cache() check is harmless because typical
guys don't put executable into tmpfs. So, If you can't agree me, I can
accept your one.

Thanks.



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