mm-more-likely-reclaim-madv_sequential-mappings.patch

mm-more-likely-reclaim-madv_sequential-mappings.patch

[Andrew Morton]

I have a note here that this patch needs better justification.  But the
changelog looks good and there are pretty graphs, so maybe my note is stale.

Can people please check it?

Thanks.




From: Johannes Weiner <hannes@saeurebad.de>

File pages accessed only once through sequential-read mappings between
fault and scan time are perfect candidates for reclaim.

This patch makes page_referenced() ignore these singular references and
the pages stay on the inactive list where they likely fall victim to the
next reclaim phase.

Already activated pages are still treated normally.  If they were accessed
multiple times and therefor promoted to the active list, we probably want
to keep them.

Benchmarks show that big (relative to the system's memory) MADV_SEQUENTIAL
mappings read sequentially cause much less kernel activity.  Especially
less LRU moving-around because we never activate read-once pages in the
first place just to demote them again.

And leaving these perfect reclaim candidates on the inactive list makes
it more likely for the real working set to survive the next reclaim
scan.

Benchmark graphs and the test-application can be found here:

	http://hannes.saeurebad.de/madvseq/

Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
Signed-off-by: Rik van Riel <riel@redhat.com>
Cc: "KOSAKI Motohiro" <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 mm/rmap.c |   20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff -puN mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings mm/rmap.c
--- a/mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings
+++ a/mm/rmap.c
@@ -327,8 +327,18 @@ static int page_referenced_one(struct pa
 		goto out_unmap;
 	}
 
-	if (ptep_clear_flush_young_notify(vma, address, pte))
-		referenced++;
+	if (ptep_clear_flush_young_notify(vma, address, pte)) {
+		/*
+		 * If there was just one sequential access to the
+		 * page, ignore it.  Otherwise, mark_page_accessed()
+		 * will have promoted the page to the active list and
+		 * it should be kept.
+		 */
+		if (VM_SequentialReadHint(vma) && !PageActive(page))
+			ClearPageReferenced(page);
+		else
+			referenced++;
+	}
 
 	/* Pretend the page is referenced if the task has the
 	   swap token and is in the middle of a page fault. */
@@ -449,9 +459,6 @@ int page_referenced(struct page *page, i
 {
 	int referenced = 0;
 
-	if (TestClearPageReferenced(page))
-		referenced++;
-
 	if (page_mapped(page) && page->mapping) {
 		if (PageAnon(page))
 			referenced += page_referenced_anon(page, mem_cont);
@@ -467,6 +474,9 @@ int page_referenced(struct page *page, i
 		}
 	}
 
+	if (TestClearPageReferenced(page))
+		referenced++;
+
 	if (page_test_and_clear_young(page))
 		referenced++;
 
_

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

> I have a note here that this patch needs better justification.  But the
> changelog looks good and there are pretty graphs, so maybe my note is stale.
> 
> Can people please check it?
> 
> Thanks.

maybe, I can run benchmark it.
please wait few hour.



> 
> 
> 
> 
> From: Johannes Weiner <hannes@saeurebad.de>
> 
> File pages accessed only once through sequential-read mappings between
> fault and scan time are perfect candidates for reclaim.
> 
> This patch makes page_referenced() ignore these singular references and
> the pages stay on the inactive list where they likely fall victim to the
> next reclaim phase.
> 
> Already activated pages are still treated normally.  If they were accessed
> multiple times and therefor promoted to the active list, we probably want
> to keep them.
> 
> Benchmarks show that big (relative to the system's memory) MADV_SEQUENTIAL
> mappings read sequentially cause much less kernel activity.  Especially
> less LRU moving-around because we never activate read-once pages in the
> first place just to demote them again.
> 
> And leaving these perfect reclaim candidates on the inactive list makes
> it more likely for the real working set to survive the next reclaim
> scan.
> 
> Benchmark graphs and the test-application can be found here:
> 
> 	http://hannes.saeurebad.de/madvseq/
> 
> Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Cc: "KOSAKI Motohiro" <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
> 
>  mm/rmap.c |   20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff -puN mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings mm/rmap.c
> --- a/mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings
> +++ a/mm/rmap.c
> @@ -327,8 +327,18 @@ static int page_referenced_one(struct pa
>  		goto out_unmap;
>  	}
>  
> -	if (ptep_clear_flush_young_notify(vma, address, pte))
> -		referenced++;
> +	if (ptep_clear_flush_young_notify(vma, address, pte)) {
> +		/*
> +		 * If there was just one sequential access to the
> +		 * page, ignore it.  Otherwise, mark_page_accessed()
> +		 * will have promoted the page to the active list and
> +		 * it should be kept.
> +		 */
> +		if (VM_SequentialReadHint(vma) && !PageActive(page))
> +			ClearPageReferenced(page);
> +		else
> +			referenced++;
> +	}
>  
>  	/* Pretend the page is referenced if the task has the
>  	   swap token and is in the middle of a page fault. */
> @@ -449,9 +459,6 @@ int page_referenced(struct page *page, i
>  {
>  	int referenced = 0;
>  
> -	if (TestClearPageReferenced(page))
> -		referenced++;
> -
>  	if (page_mapped(page) && page->mapping) {
>  		if (PageAnon(page))
>  			referenced += page_referenced_anon(page, mem_cont);
> @@ -467,6 +474,9 @@ int page_referenced(struct page *page, i
>  		}
>  	}
>  
> +	if (TestClearPageReferenced(page))
> +		referenced++;
> +
>  	if (page_test_and_clear_young(page))
>  		referenced++;
>  
> _
> 
> --
> 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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

> > I have a note here that this patch needs better justification.  But the
> > changelog looks good and there are pretty graphs, so maybe my note is stale.
> > 
> > Can people please check it?
> > 
> > Thanks.
> 
> maybe, I can run benchmark it.
> please wait few hour.

1. mesured various copy performance.
   using copybench -> http://code.google.com/p/copybench/

   my machine mem:   8GB
   target file size: 10GB (filesize > system mem)


                         2.6.27    mmotm-1010:
   ==============================================================
   rw_cp                 6:13      6:11
   rw_fadv_cp            6:09      6:06
   mm_sync_cp            5:51      5:55
   mm_sync_madv_cp       5:59      5:57
   mw_cp                 5:50      5:50
   mw_madv_cp            5:55      5:55


   So, no improvement, but no regression.


2. Latency degression ratio of Sequential copy v.s. Other I/O situation

	run following script (mm_sync_madv_cp is one of copybench program)

	$ dbench -D /disk2/ -c client.txt 100 &
	$ sleep 100
	$ time ./mm_sync_madv_cp src dst


                         2.6.27    mmotm-1010
   ==============================================================
   mm_sync_madv_cp       6:14      6:02         (min:sec)
   dbench throughput     12.1507   14.6273      (MB/s)
   dbench latency        33046     21779        (ms)


   So, throughput improvement is relativily a bit, but latency improvement is much.
   Then, I think the patch can improve "larege file copy (e.g. backup operation)
   attacks desktop latency" problem.

Any comments?


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>

Re: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Andrew Morton]

On Thu, 16 Oct 2008 15:01:01 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > > I have a note here that this patch needs better justification.  But the
> > > changelog looks good and there are pretty graphs, so maybe my note is stale.
> > > 
> > > Can people please check it?
> > > 
> > > Thanks.
> > 
> > maybe, I can run benchmark it.
> > please wait few hour.

Thanks, it really helps.

> 1. mesured various copy performance.
>    using copybench -> http://code.google.com/p/copybench/
> 
>    my machine mem:   8GB
>    target file size: 10GB (filesize > system mem)
> 
> 
>                          2.6.27    mmotm-1010:
>    ==============================================================
>    rw_cp                 6:13      6:11
>    rw_fadv_cp            6:09      6:06
>    mm_sync_cp            5:51      5:55
>    mm_sync_madv_cp       5:59      5:57
>    mw_cp                 5:50      5:50
>    mw_madv_cp            5:55      5:55
> 
> 
>    So, no improvement, but no regression.
> 
> 
> 2. Latency degression ratio of Sequential copy v.s. Other I/O situation
> 
> 	run following script (mm_sync_madv_cp is one of copybench program)
> 
> 	$ dbench -D /disk2/ -c client.txt 100 &
> 	$ sleep 100
> 	$ time ./mm_sync_madv_cp src dst
> 
> 
>                          2.6.27    mmotm-1010
>    ==============================================================
>    mm_sync_madv_cp       6:14      6:02         (min:sec)
>    dbench throughput     12.1507   14.6273      (MB/s)
>    dbench latency        33046     21779        (ms)
> 
> 
>    So, throughput improvement is relativily a bit, but latency improvement is much.
>    Then, I think the patch can improve "larege file copy (e.g. backup operation)
>    attacks desktop latency" problem.
> 
> Any comments?
> 

Sounds good.

But how do we know that it was this particular patch which improved the
latency performance?

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

Johannes, 

>                          2.6.27    mmotm-1010
>    ==============================================================
>    mm_sync_madv_cp       6:14      6:02         (min:sec)
>    dbench throughput     12.1507   14.6273      (MB/s)
>    dbench latency        33046     21779        (ms)
> 
> 
>    So, throughput improvement is relativily a bit, but latency improvement is much.
>    Then, I think the patch can improve "larege file copy (e.g. backup operation)
>    attacks desktop latency" problem.

That means, 
	Tested-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
	Ack-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>


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>

Re: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

> >                          2.6.27    mmotm-1010
> >    ==============================================================
> >    mm_sync_madv_cp       6:14      6:02         (min:sec)
> >    dbench throughput     12.1507   14.6273      (MB/s)
> >    dbench latency        33046     21779        (ms)
> > 
> > 
> >    So, throughput improvement is relativily a bit, but latency improvement is much.
> >    Then, I think the patch can improve "larege file copy (e.g. backup operation)
> >    attacks desktop latency" problem.
> > 
> > Any comments?
> > 
> 
> Sounds good.
> 
> But how do we know that it was this particular patch which improved the
> latency performance?

In my concern,

dbench's pages are touched multiple times, but copy's pages are touched only twice.
Then, on 2.6.27, copy's page transit to inactive -> active -> inactive -> free.
it decrease latency meaninglessly.

IOW, 2.6.27 model

1. shrink_inactive_lsit() promote copy's page to active (it touched twice (readahead + memcpy))
2. shrink_active_list() demote dbench's page
3. shrink_inactive_list() promote dbench's page (because it is touched multiple times)
4. shrink_active_list() demote copy's page
5. shrink_inactive_list() free copy's page


mmotm mode,

1, shrink_inactive_list() free copy's page.
2. end!



--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Andrew Morton]

On Thu, 16 Oct 2008 15:22:15 +0900 (JST) KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > >                          2.6.27    mmotm-1010
> > >    ==============================================================
> > >    mm_sync_madv_cp       6:14      6:02         (min:sec)
> > >    dbench throughput     12.1507   14.6273      (MB/s)
> > >    dbench latency        33046     21779        (ms)
> > > 
> > > 
> > >    So, throughput improvement is relativily a bit, but latency improvement is much.
> > >    Then, I think the patch can improve "larege file copy (e.g. backup operation)
> > >    attacks desktop latency" problem.
> > > 
> > > Any comments?
> > > 
> > 
> > Sounds good.
> > 
> > But how do we know that it was this particular patch which improved the
> > latency performance?
> 
> In my concern,
> 
> dbench's pages are touched multiple times, but copy's pages are touched only twice.
> Then, on 2.6.27, copy's page transit to inactive -> active -> inactive -> free.
> it decrease latency meaninglessly.
> 
> IOW, 2.6.27 model
> 
> 1. shrink_inactive_lsit() promote copy's page to active (it touched twice (readahead + memcpy))
> 2. shrink_active_list() demote dbench's page
> 3. shrink_inactive_list() promote dbench's page (because it is touched multiple times)
> 4. shrink_active_list() demote copy's page
> 5. shrink_inactive_list() free copy's page
> 
> 
> mmotm mode,
> 
> 1, shrink_inactive_list() free copy's page.
> 2. end!

OK.  But my concern is that perhaps the above latency improvement was
caused by one of the many other MM patches in mmotm.

Reverting mm-more-likely-reclaim-madv_sequential-mappings.patch from
mmotm and rerunning the tests would be the way to determine this. 
(hint :) - 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>

Re: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

> > mmotm mode,
> > 
> > 1, shrink_inactive_list() free copy's page.
> > 2. end!
> 
> OK.  But my concern is that perhaps the above latency improvement was
> caused by one of the many other MM patches in mmotm.
> 
> Reverting mm-more-likely-reclaim-madv_sequential-mappings.patch from
> mmotm and rerunning the tests would be the way to determine this. 
> (hint :) - thanks).

fair enough.
OK, please wait half hour.



--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

> > > mmotm mode,
> > > 
> > > 1, shrink_inactive_list() free copy's page.
> > > 2. end!
> > 
> > OK.  But my concern is that perhaps the above latency improvement was
> > caused by one of the many other MM patches in mmotm.
> > 
> > Reverting mm-more-likely-reclaim-madv_sequential-mappings.patch from
> > mmotm and rerunning the tests would be the way to determine this. 
> > (hint :) - thanks).
> 
> fair enough.
> OK, please wait half hour.

I mesured 2.6.27+sequential-patch.

                                   (NEW)
                         2.6.27    2.6.27+patch   mmotm-1010
   ==============================================================
   mm_sync_madv_cp       6:14      6:03           6:02         (min:sec)
   dbench throughput     12.1507   13.915         14.6273      (MB/s)
   dbench latency        33046     22062          21779        (ms)


Hm, I think other patches influence isn't so much.

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>

Re: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Thursday 16 October 2008 10:22, Andrew Morton wrote:
> I have a note here that this patch needs better justification.  But the
> changelog looks good and there are pretty graphs, so maybe my note is
> stale.

I think I was hoping for our VM to generically not be quite so stupid about
use-once access patterns and not behave so badly without this. But easier
said than done, and now I see the graphs show this is a fairly reasonable
change. One thing...

>
> Can people please check it?
>
> Thanks.
>
>
>
>
> From: Johannes Weiner <hannes@saeurebad.de>
>
> File pages accessed only once through sequential-read mappings between
> fault and scan time are perfect candidates for reclaim.
>
> This patch makes page_referenced() ignore these singular references and
> the pages stay on the inactive list where they likely fall victim to the
> next reclaim phase.
>
> Already activated pages are still treated normally.  If they were accessed
> multiple times and therefor promoted to the active list, we probably want
> to keep them.
>
> Benchmarks show that big (relative to the system's memory) MADV_SEQUENTIAL
> mappings read sequentially cause much less kernel activity.  Especially
> less LRU moving-around because we never activate read-once pages in the
> first place just to demote them again.
>
> And leaving these perfect reclaim candidates on the inactive list makes
> it more likely for the real working set to survive the next reclaim
> scan.
>
> Benchmark graphs and the test-application can be found here:
>
> 	http://hannes.saeurebad.de/madvseq/
>
> Signed-off-by: Johannes Weiner <hannes@saeurebad.de>
> Signed-off-by: Rik van Riel <riel@redhat.com>
> Cc: "KOSAKI Motohiro" <kosaki.motohiro@jp.fujitsu.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
>  mm/rmap.c |   20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff -puN mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings
> mm/rmap.c --- a/mm/rmap.c~mm-more-likely-reclaim-madv_sequential-mappings
> +++ a/mm/rmap.c
> @@ -327,8 +327,18 @@ static int page_referenced_one(struct pa
>  		goto out_unmap;
>  	}
>
> -	if (ptep_clear_flush_young_notify(vma, address, pte))
> -		referenced++;
> +	if (ptep_clear_flush_young_notify(vma, address, pte)) {
> +		/*
> +		 * If there was just one sequential access to the
> +		 * page, ignore it.  Otherwise, mark_page_accessed()
> +		 * will have promoted the page to the active list and
> +		 * it should be kept.
> +		 */
> +		if (VM_SequentialReadHint(vma) && !PageActive(page))
> +			ClearPageReferenced(page);
> +		else
> +			referenced++;
> +	}

ClearPageReferenced I don't know if it should be cleared like this.
PageReferenced is more of a bit for the mark_page_accessed state machine,
rather than the pte_young stuff. Although when unmapping, the latter
somewhat collapses back to the former, but I don't know if there is a
very good reason to fiddle with it here.

Ignoring the young bit in the pte for sequential hint maybe is OK (and
seems to be effective as per the benchmarks). But I would prefer not to
merge the PageReferenced parts unless they get their own justification.

So I'm happy with the ignoring pte_young part of the patch.


BTW. it seems like zap_pte_range should do a mark_page_accessed(), because
setting PG_accessed alone is quite a weak hint to reclaim... I don't think
it makes sense for mmap(), touch, munmap() access to arbitrarily be worth
less than a read(2).

I guess I should test and submit the patch...

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Rik van Riel]

Nick Piggin wrote:

> ClearPageReferenced I don't know if it should be cleared like this.
> PageReferenced is more of a bit for the mark_page_accessed state machine,
> rather than the pte_young stuff. Although when unmapping, the latter
> somewhat collapses back to the former, but I don't know if there is a
> very good reason to fiddle with it here.
> 
> Ignoring the young bit in the pte for sequential hint maybe is OK (and
> seems to be effective as per the benchmarks). But I would prefer not to
> merge the PageReferenced parts unless they get their own justification.

Unless we clear the PageReferenced bit, we will still activate
the page - even if its only access came through a sequential
mapping.

Faulting the page into the sequential mapping ends up setting
PageReferenced, IIRC.

-- 
All rights reversed.

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Friday 17 October 2008 04:04, Rik van Riel wrote:
> Nick Piggin wrote:
> > ClearPageReferenced I don't know if it should be cleared like this.
> > PageReferenced is more of a bit for the mark_page_accessed state machine,
> > rather than the pte_young stuff. Although when unmapping, the latter
> > somewhat collapses back to the former, but I don't know if there is a
> > very good reason to fiddle with it here.
> >
> > Ignoring the young bit in the pte for sequential hint maybe is OK (and
> > seems to be effective as per the benchmarks). But I would prefer not to
> > merge the PageReferenced parts unless they get their own justification.
>
> Unless we clear the PageReferenced bit, we will still activate
> the page - even if its only access came through a sequential
> mapping.
>
> Faulting the page into the sequential mapping ends up setting
> PageReferenced, IIRC.

Yes I see. But that's stupid because then you can end up putting a
sequential mapping on a page, and cause that to deactivate somebody
else's references... and the deactivation _only_ happens if the
sequential mapping pte is young and the page happens not to be
active, which is totally arbitrary.

Really, filemap_fault should not mark the page as accessed,
zap_pte_range should mark the page has accessed rather than just
set referenced, and this patch should not clear referenced.

I dislike having to hack around something in a way that does work
and improves a very particular situation, but is conceptually wrong.

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[KOSAKI Motohiro]

Hi Nick,

I don't have any opinion against this patch is good or wrong.
but I have a question.


> Really, filemap_fault should not mark the page as accessed,
> zap_pte_range should mark the page has accessed rather than just
> set referenced, and this patch should not clear referenced.

IIRC, sequential mapping pages are usually touched twice.
 1. page fault (caused by readahead)
 2. memcpy in userland

So, if we only drop accessed bit of the page at page fault, the page end up
having accessed bit by memcpy.

pointless?


--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Friday 17 October 2008 16:37, KOSAKI Motohiro wrote:
> Hi Nick,
>
> I don't have any opinion against this patch is good or wrong.
> but I have a question.
>
> > Really, filemap_fault should not mark the page as accessed,
> > zap_pte_range should mark the page has accessed rather than just
> > set referenced, and this patch should not clear referenced.
>
> IIRC, sequential mapping pages are usually touched twice.
>  1. page fault (caused by readahead)
>  2. memcpy in userland
>
> So, if we only drop accessed bit of the page at page fault, the page end up
> having accessed bit by memcpy.
>
> pointless?

Well, the pte will get the accessed bit set by the set_pte call. This
would probably not be set again by the memcpy (unless it was attempted
to be reclaimed in the meantime, but that should be fairly rare).

And the sequential mapping special case ignores the pte bits, so that's
OK.

The problem is that the page fault path also does a mark_page_accessed,
which sets the page's PG_dirty bit. Now this bit is mainly used by the
unmapped pagecache access / reclaim heuristics, but because we set it
here, then the sequential mapping case is forced to clear it. I think it
would be much cleaner not to set the bit in the fault handler to begin
with.

I would like to ask this sequential mapping patch is held off until then,
and I will send a patch to make the mark_page_accessed, after the dust
settles from Andrew's merge. (because I can't make such a change inside
the merge window)

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Johannes Weiner]

Nick Piggin <nickpiggin@yahoo.com.au> writes:

> On Friday 17 October 2008 04:04, Rik van Riel wrote:
>> Nick Piggin wrote:
>> > ClearPageReferenced I don't know if it should be cleared like this.
>> > PageReferenced is more of a bit for the mark_page_accessed state machine,
>> > rather than the pte_young stuff. Although when unmapping, the latter
>> > somewhat collapses back to the former, but I don't know if there is a
>> > very good reason to fiddle with it here.
>> >
>> > Ignoring the young bit in the pte for sequential hint maybe is OK (and
>> > seems to be effective as per the benchmarks). But I would prefer not to
>> > merge the PageReferenced parts unless they get their own justification.
>>
>> Unless we clear the PageReferenced bit, we will still activate
>> the page - even if its only access came through a sequential
>> mapping.
>>
>> Faulting the page into the sequential mapping ends up setting
>> PageReferenced, IIRC.
>
> Yes I see. But that's stupid because then you can end up putting a
> sequential mapping on a page, and cause that to deactivate somebody
> else's references... and the deactivation _only_ happens if the
> sequential mapping pte is young and the page happens not to be
> active, which is totally arbitrary.

Another access would mean another young PTE, which we will catch as a
proper reference sooner or later while walking the mappings, no?

	Hannes

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Saturday 18 October 2008 03:51, Johannes Weiner wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> writes:
> > On Friday 17 October 2008 04:04, Rik van Riel wrote:
> >> Nick Piggin wrote:
> >> > ClearPageReferenced I don't know if it should be cleared like this.
> >> > PageReferenced is more of a bit for the mark_page_accessed state
> >> > machine, rather than the pte_young stuff. Although when unmapping, the
> >> > latter somewhat collapses back to the former, but I don't know if
> >> > there is a very good reason to fiddle with it here.
> >> >
> >> > Ignoring the young bit in the pte for sequential hint maybe is OK (and
> >> > seems to be effective as per the benchmarks). But I would prefer not
> >> > to merge the PageReferenced parts unless they get their own
> >> > justification.
> >>
> >> Unless we clear the PageReferenced bit, we will still activate
> >> the page - even if its only access came through a sequential
> >> mapping.
> >>
> >> Faulting the page into the sequential mapping ends up setting
> >> PageReferenced, IIRC.
> >
> > Yes I see. But that's stupid because then you can end up putting a
> > sequential mapping on a page, and cause that to deactivate somebody
> > else's references... and the deactivation _only_ happens if the
> > sequential mapping pte is young and the page happens not to be
> > active, which is totally arbitrary.
>
> Another access would mean another young PTE, which we will catch as a
> proper reference sooner or later while walking the mappings, no?

No. Another access could come via read/write, or be subsequently unmapped
and put into PG_referenced.

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Johannes Weiner]

Nick Piggin <nickpiggin@yahoo.com.au> writes:

> On Saturday 18 October 2008 03:51, Johannes Weiner wrote:
>> Nick Piggin <nickpiggin@yahoo.com.au> writes:
>> > On Friday 17 October 2008 04:04, Rik van Riel wrote:
>> >> Nick Piggin wrote:
>> >> > ClearPageReferenced I don't know if it should be cleared like this.
>> >> > PageReferenced is more of a bit for the mark_page_accessed state
>> >> > machine, rather than the pte_young stuff. Although when unmapping, the
>> >> > latter somewhat collapses back to the former, but I don't know if
>> >> > there is a very good reason to fiddle with it here.
>> >> >
>> >> > Ignoring the young bit in the pte for sequential hint maybe is OK (and
>> >> > seems to be effective as per the benchmarks). But I would prefer not
>> >> > to merge the PageReferenced parts unless they get their own
>> >> > justification.
>> >>
>> >> Unless we clear the PageReferenced bit, we will still activate
>> >> the page - even if its only access came through a sequential
>> >> mapping.
>> >>
>> >> Faulting the page into the sequential mapping ends up setting
>> >> PageReferenced, IIRC.
>> >
>> > Yes I see. But that's stupid because then you can end up putting a
>> > sequential mapping on a page, and cause that to deactivate somebody
>> > else's references... and the deactivation _only_ happens if the
>> > sequential mapping pte is young and the page happens not to be
>> > active, which is totally arbitrary.
>>
>> Another access would mean another young PTE, which we will catch as a
>> proper reference sooner or later while walking the mappings, no?
>
> No. Another access could come via read/write, or be subsequently unmapped
> and put into PG_referenced.

read/write use mark_page_accessed(), so after having two accesses, the
page is already active.  If it's not and we find an access through a
sequential mapping, we should be safe to clear PG_referenced.

So the combination of young pte, page not active and scanning a
sequential mapping is not an arbitrary condition at all.

The only incorrect behaviour I can see here is what you already pointed
out: zap_pte_range() should mark_page_accessed().

	Hannes

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Saturday 18 October 2008 21:45, Johannes Weiner wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> writes:
> > On Saturday 18 October 2008 03:51, Johannes Weiner wrote:
> >> Nick Piggin <nickpiggin@yahoo.com.au> writes:
> >> > On Friday 17 October 2008 04:04, Rik van Riel wrote:
> >> >> Nick Piggin wrote:
> >> >> > ClearPageReferenced I don't know if it should be cleared like this.
> >> >> > PageReferenced is more of a bit for the mark_page_accessed state
> >> >> > machine, rather than the pte_young stuff. Although when unmapping,
> >> >> > the latter somewhat collapses back to the former, but I don't know
> >> >> > if there is a very good reason to fiddle with it here.
> >> >> >
> >> >> > Ignoring the young bit in the pte for sequential hint maybe is OK
> >> >> > (and seems to be effective as per the benchmarks). But I would
> >> >> > prefer not to merge the PageReferenced parts unless they get their
> >> >> > own justification.
> >> >>
> >> >> Unless we clear the PageReferenced bit, we will still activate
> >> >> the page - even if its only access came through a sequential
> >> >> mapping.
> >> >>
> >> >> Faulting the page into the sequential mapping ends up setting
> >> >> PageReferenced, IIRC.
> >> >
> >> > Yes I see. But that's stupid because then you can end up putting a
> >> > sequential mapping on a page, and cause that to deactivate somebody
> >> > else's references... and the deactivation _only_ happens if the
> >> > sequential mapping pte is young and the page happens not to be
> >> > active, which is totally arbitrary.
> >>
> >> Another access would mean another young PTE, which we will catch as a
> >> proper reference sooner or later while walking the mappings, no?
> >
> > No. Another access could come via read/write, or be subsequently unmapped
> > and put into PG_referenced.
>
> read/write use mark_page_accessed(), so after having two accesses, the
> page is already active.  If it's not and we find an access through a
> sequential mapping, we should be safe to clear PG_referenced.

That's just handwaving. The patch still clears PG_referenced, which
is a shared resource, and it is wrong, conceptually. You can't argue
with that.

What about if mark_page_accessed is only used on the page once? and
it is referenced but not active?


> So the combination of young pte, page not active and scanning a
> sequential mapping is not an arbitrary condition at all.

No, it is a specific condition. And specifically it is wrong.

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Rik van Riel]

Nick Piggin wrote:
>
> That's just handwaving. The patch still clears PG_referenced, which
> is a shared resource, and it is wrong, conceptually. You can't argue
> with that.
>
>   
I don't see an easy way around that.  If the PG_referenced bit is
set and the page is mapped, the code in vmscan.c will move the
page to the active list.

Even if the one pte mapping the page is in an MADV_SEQUENTIAL
VMA, in which case we definately do not want to activate the page.

Of course, if the PG_referenced came from a different access, things
would be a different matter. 

Fixing the page fault code so that it does not set the PG_referenced bit
would take care of 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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Sunday 19 October 2008 13:43, Rik van Riel wrote:
> Nick Piggin wrote:
> > That's just handwaving. The patch still clears PG_referenced, which
> > is a shared resource, and it is wrong, conceptually. You can't argue
> > with that.
>
> I don't see an easy way around that.  If the PG_referenced bit is
> set and the page is mapped, the code in vmscan.c will move the
> page to the active list.
>
> Even if the one pte mapping the page is in an MADV_SEQUENTIAL
> VMA, in which case we definately do not want to activate the page.
>
> Of course, if the PG_referenced came from a different access, things
> would be a different matter.
>
> Fixing the page fault code so that it does not set the PG_referenced bit
> would take care of that.

Yes, I skeched my plan to fix this in a previous mail.

Take the mark_page_accessed out of the page fault handler; put it into
the unmap path in replacement of the SetPageReferenced; then modify
this patch so it doesn't fiddle with references that aren't hinted.

I'm just going to wait for Andrew to do his merges before sending
patches. There is no pressing need to merge this madv patch *right now*,
so it can wait I think.

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Johannes Weiner]

Nick Piggin <nickpiggin@yahoo.com.au> writes:

>> >> Another access would mean another young PTE, which we will catch as a
>> >> proper reference sooner or later while walking the mappings, no?
>> >
>> > No. Another access could come via read/write, or be subsequently unmapped
>> > and put into PG_referenced.
>>
>> read/write use mark_page_accessed(), so after having two accesses, the
>> page is already active.  If it's not and we find an access through a
>> sequential mapping, we should be safe to clear PG_referenced.
>
> That's just handwaving. The patch still clears PG_referenced, which
> is a shared resource, and it is wrong, conceptually. You can't argue
> with that.
>
> What about if mark_page_accessed is only used on the page once? and
> it is referenced but not active?

I see the problem now, thanks for not giving up ;) Fixing up the fault
paths and moving their mark_page_accessed to the unmap side seems like a
good idea.

	Hannes

--
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: mm-more-likely-reclaim-madv_sequential-mappings.patch

[Nick Piggin]

On Monday 20 October 2008 01:39, Johannes Weiner wrote:
> Nick Piggin <nickpiggin@yahoo.com.au> writes:
> >> >> Another access would mean another young PTE, which we will catch as a
> >> >> proper reference sooner or later while walking the mappings, no?
> >> >
> >> > No. Another access could come via read/write, or be subsequently
> >> > unmapped and put into PG_referenced.
> >>
> >> read/write use mark_page_accessed(), so after having two accesses, the
> >> page is already active.  If it's not and we find an access through a
> >> sequential mapping, we should be safe to clear PG_referenced.
> >
> > That's just handwaving. The patch still clears PG_referenced, which
> > is a shared resource, and it is wrong, conceptually. You can't argue
> > with that.
> >
> > What about if mark_page_accessed is only used on the page once? and
> > it is referenced but not active?
>
> I see the problem now, thanks for not giving up ;) Fixing up the fault
> paths and moving their mark_page_accessed to the unmap side seems like a
> good idea.

Thanks. I think I was skeptical of the patch the first time around, but
that could have been because of this other stuff you necessarily had to
hack around to make it work confused me for one reason or another.

With that fixed up, I think your patch should become much more "obviously
correct", and definitely a win for anyone using MADV_SEQUENTIAL.

Andrew's merged up most stuff now, so I'll send over some patches soon.

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