f/madivse(DONTNEED) support

checkpatch reports following problems.
It's a very annoying. This patch fixes it.

total: 1 errors, 8 warnings, 517 lines checked

Signed-off-by Minchan Kim<[email protected]>

checkpatch reports following problems.
It's a very annoying. This patch fixes it.

barrios@barrios-desktop:~/linux-2.6$ ./scripts/checkpatch.pl -f mm/swap.c
WARNING: line over 80 characters

(quoted diff) +1 -0

+		if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable +EXPORT_SYMBOL(mark_page_accessed);

ERROR: code indent should use tabs where possible
+ ^I^I}$

WARNING: please, no space before tabs
+ ^I^I}$

WARNING: please, no spaces at the start of a line
+ ^I^I}$

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable +EXPORT_SYMBOL(__pagevec_release);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable +EXPORT_SYMBOL(____pagevec_lru_add);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable +EXPORT_SYMBOL(pagevec_lookup);

WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable +EXPORT_SYMBOL(pagevec_lookup_tag);

total: 1 errors, 8 warnings, 517 lines checked

Signed-off-by Minchan Kim <[email protected]>

Recently, there are reported problem about thrashing.
(http://marc.info/?l=rsync&m=128885034930933&w=2)
It happens by backup workloads(ex, nightly rsync). That's because the workload makes just use-once pages and touches pages twice. It promotes the page into active list so that it results in working set page eviction.

Some app developer want to support POSIX_FADV_NOREUSE. But other OSes don't support it, either.
(http://marc.info/?l=linux-mm&m=128928979512086&w=2)

By other approach, app developers use POSIX_FADV_DONTNEED. But it has a problem. If kernel meets page is writing during invalidate_mapping_pages, it can't work.
It is very hard for application programmer to use it. Because they always have to sync data before calling fadivse(..POSIX_FADV_DONTNEED) to make sure the pages could be discardable. At last, they can't use deferred write of kernel so that they could see performance loss.
(http://insights.oetiker.ch/linux/fadvise.html)

In fact, invalidation is very big hint to reclaimer. It means we don't use the page any more. So let's move the writing page into inactive list's head.

Why I need the page to head, Dirty/Writeback page would be flushed sooner or later. It can prevent writeout of pageout which is less effective than flusher's writeout.

Originally, I reused lru_demote of Peter with some change so added his Signed-off-by.

Reported-by Ben Gamari <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Signed-off-by Peter Zijlstra <[email protected]>

Acked-by Rik van Riel <[email protected]>

Reviewed-by KOSAKI Motohiro <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Andrew. Before applying this series, please drop below two patches.
mm-deactivate-invalidated-pages.patch mm-deactivate-invalidated-pages-fix.patch

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

Changelog since v2:
- mapped page leaves alone - suggested by Mel
- pass part related PG_reclaim in next patch.

Changelog since v1:
- modify description
- correct typo
- add some comment

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/swap.h |    1 +
 mm/swap.c            |   78 ++++++++++++++++++++++++++++++++++++++++++++++++++
 mm/truncate.c        |   17 ++++++++--
 3 files changed, 92 insertions(+), 4 deletions(-)

include/linux/swap.h +1 -0

index eba53e7..605ab62 100644
@@ -213,6 +213,7 @@ extern void mark_page_accessed(struct page *);
 extern void lru_add_drain(void);
 extern int lru_add_drain_all(void);
 extern void rotate_reclaimable_page(struct page *page);
+extern void deactivate_page(struct page *page);
 extern void swap_setup(void);
 
 extern void add_page_to_unevictable_list(struct page *page);

mm/swap.c +0 -0

index d5822b0..1f36f6f 100644

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

mm/truncate.c +0 -1

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 			if (lock_failed)
 				continue;
 
-			ret += invalidate_inode_page(page);

Quoted 4 lines · Minchan Kim

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

mm/truncate.c +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 			if (lock_failed)
 				continue;
 

mm/truncate.c unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

mm/truncate.c +9 -2

-			ret += invalidate_inode_page(page);
-
+			ret = invalidate_inode_page(page);
+			/*
+			 * If the page is dirty or under writeback, we can not
+			 * invalidate it now.  But we assume that attempted
+			 * invalidation is a hint that the page is no longer
+			 * of interest and try to speed up its reclaim.
+			 */
+			if (!ret && (PageDirty(page) || PageWriteback(page)))
+				deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Can you explain why you felt the need to add these checks?

Thanks!

Hannes

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

mm/truncate.c +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 			if (lock_failed)
 				continue;
 

Quoted 14 lines

mm/truncate.c unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

mm/truncate.c +9 -2

-			ret += invalidate_inode_page(page);
-
+			ret = invalidate_inode_page(page);
+			/*
+			 * If the page is dirty or under writeback, we can not
+			 * invalidate it now.  But we assume that attempted
+			 * invalidation is a hint that the page is no longer
+			 * of interest and try to speed up its reclaim.
+			 */
+			if (!ret && (PageDirty(page) || PageWriteback(page)))
+				deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Quoted 28 lines · Minchan Kim

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

mm/truncate.c +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 			if (lock_failed)
 				continue;
 

Quoted 14 lines

mm/truncate.c unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

mm/truncate.c +9 -2

-			ret += invalidate_inode_page(page);
-
+			ret = invalidate_inode_page(page);
+			/*
+			 * If the page is dirty or under writeback, we can not
+			 * invalidate it now.  But we assume that attempted
+			 * invalidation is a hint that the page is no longer
+			 * of interest and try to speed up its reclaim.
+			 */
+			if (!ret && (PageDirty(page) || PageWriteback(page)))
+				deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Quoted 7 lines

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we try to reclaim it? The reason for deactivating at this location is that truncation is a strong hint for reclaim, not that it failed due to dirty/writeback pages.

What's the problem with deactivating pages where try_to_release_page() failed?

I don't think we should add more logic than necessary. If there is a good reason for it, it needs to get a code comment at least.

Quoted 42 lines · Johannes Weiner

Quoted 28 lines · Minchan Kim

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
- Change function comments - suggested by Johannes
- Change function name - suggested by Johannes
- add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

mm/truncate.c +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 			if (lock_failed)
 				continue;
 

Quoted 14 lines

mm/truncate.c unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

mm/truncate.c +9 -2

-			ret += invalidate_inode_page(page);
-
+			ret = invalidate_inode_page(page);
+			/*
+			 * If the page is dirty or under writeback, we can not
+			 * invalidate it now.  But we assume that attempted
+			 * invalidation is a hint that the page is no longer
+			 * of interest and try to speed up its reclaim.
+			 */
+			if (!ret && (PageDirty(page) || PageWriteback(page)))
+				deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Quoted 7 lines

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we try to reclaim it? The reason for deactivating at this location is that truncation is a strong hint for reclaim, not that it failed due to dirty/writeback pages.

Quoted 2 lines

What's the problem with deactivating pages where try_to_release_page() failed?

If try_to_release_page fails and the such pages stay long time in pagevec, pagevec drain often happens.

I think such pages are rare so skip such pages doesn't hurt goal of this patch.

Quoted 46 lines · Minchan Kim

Quoted 44 lines · Johannes Weiner

Quoted 28 lines · Minchan Kim

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
 - Change function comments - suggested by Johannes
 - Change function name - suggested by Johannes
 - add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

mm/truncate.c +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
                       if (lock_failed)
                               continue;
 

Quoted 14 lines

mm/truncate.c unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

mm/truncate.c +9 -2

-                     ret += invalidate_inode_page(page);
-
+                     ret = invalidate_inode_page(page);
+                     /*
+                      * If the page is dirty or under writeback, we can not
+                      * invalidate it now.  But we assume that attempted
+                      * invalidation is a hint that the page is no longer
+                      * of interest and try to speed up its reclaim.
+                      */
+                     if (!ret && (PageDirty(page) || PageWriteback(page)))
+                             deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Quoted 7 lines

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we try to reclaim it?  The reason for deactivating at this location is that truncation is a strong hint for reclaim, not that it failed due to dirty/writeback pages.

What's the problem with deactivating pages where try_to_release_page() failed?

Quoted 2 lines

If try_to_release_page fails and the such pages stay long time in pagevec, pagevec drain often happens.

You mean because the pagevec becomes full more often?  These are not many pages you get extra without the checks, the race window is very small after all.

Quoted 3 lines

I think such pages are rare so skip such pages doesn't hurt goal of this patch.

Well, you add extra checks, extra detail to this mechanism.  Instead of just saying 'tried to truncate, failed, deactivate the page', you add more ifs and buts.

There should be a real justification for it.  'It can not hurt' is not a good justification for extra code and making a simple model more complex.

'It will hurt without treating these pages differently' is a good justification.  Remember that we have to understand and maintain all this.  The less checks and operations we need to implement a certain idea, the better.

Sorry for being so adamant about this, but I think these random checks are a really sore point of mm code already.

[ For example, we tried discussing lumpy reclaim mode recently and none of us could reliably remember how it actually behaved.  There are so many special conditions in there that we already end up with some of them being dead code and the checks even contradicting each other. ]

Quoted 47 lines · Johannes Weiner

Quoted 46 lines · Minchan Kim

Quoted 44 lines · Johannes Weiner

Quoted 28 lines · Minchan Kim

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
A - Change function comments - suggested by Johannes A - Change function name - suggested by Johannes A - add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

(quoted diff) +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
 A  A  A  A  A  A  A  A  A  A  A  if (lock_failed)
 A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  continue;
 

Quoted 14 lines

Review unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

(quoted diff) +9 -2

- A  A  A  A  A  A  A  A  A  A  ret += invalidate_inode_page(page);
-
+ A  A  A  A  A  A  A  A  A  A  ret = invalidate_inode_page(page);
+ A  A  A  A  A  A  A  A  A  A  /*
+ A  A  A  A  A  A  A  A  A  A  A * If the page is dirty or under writeback, we can not
+ A  A  A  A  A  A  A  A  A  A  A * invalidate it now. A But we assume that attempted
+ A  A  A  A  A  A  A  A  A  A  A * invalidation is a hint that the page is no longer
+ A  A  A  A  A  A  A  A  A  A  A * of interest and try to speed up its reclaim.
+ A  A  A  A  A  A  A  A  A  A  A */
+ A  A  A  A  A  A  A  A  A  A  if (!ret && (PageDirty(page) || PageWriteback(page)))
+ A  A  A  A  A  A  A  A  A  A  A  A  A  A  deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Quoted 7 lines

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we try to reclaim it? A The reason for deactivating at this location is that truncation is a strong hint for reclaim, not that it failed due to dirty/writeback pages.

What's the problem with deactivating pages where try_to_release_page() failed?

Quoted 6 lines

Quoted 2 lines

If try_to_release_page fails and the such pages stay long time in pagevec, pagevec drain often happens.

You mean because the pagevec becomes full more often? A These are not many pages you get extra without the checks, the race window is very small after all.

Right.
It was a totally bad answer. The work in midnight makes my mind to be hurt. :)

Another point is that we can move such pages(!try_to_release_page, someone else holding the ref) into tail of inactive.
We can't expect such pages will be freed sooner or later and it can stir lru pages unnecessary.
On the other hand it's a _really_ rare so couldn't we move the pages into tail? If it can be justified, I will remove the check.
What do you think about it?

Quoted 58 lines · On Wed, 8 Dec 2010 07:51:25 +0900 Minchan Kim <[email protected]> wrote:

Quoted 47 lines · Johannes Weiner

Quoted 46 lines · Minchan Kim

Quoted 44 lines · Johannes Weiner

Quoted 28 lines · Minchan Kim

Quoted 12 lines · Johannes Weiner

Quoted 4 lines · Minchan Kim

Changelog since v3:
 - Change function comments - suggested by Johannes
 - Change function name - suggested by Johannes
 - add only dirty/writeback pages to deactive pagevec

Why the extra check?

Quoted 4 lines

(quoted diff) +0 -0

@@ -359,8 +360,16 @@ unsigned long invalidate_mapping_pages(struct address_space *mapping,
                       if (lock_failed)
                               continue;
 

Quoted 14 lines

Review unsigned long invalidate_mapping_pages(struct address_space *mapping, · line 371

Quoted 11 lines

(quoted diff) +9 -2

-                     ret += invalidate_inode_page(page);
-
+                     ret = invalidate_inode_page(page);
+                     /*
+                      * If the page is dirty or under writeback, we can not
+                      * invalidate it now.  But we assume that attempted
+                      * invalidation is a hint that the page is no longer
+                      * of interest and try to speed up its reclaim.
+                      */
+                     if (!ret && (PageDirty(page) || PageWriteback(page)))
+                             deactivate_page(page);

The writeback completion handler does not take the page lock, so you can still miss pages that finish writeback before this test, no?

Quoted 7 lines

Yes. but I think it's rare and even though it happens, it's not critical.

Quoted 2 lines

Can you explain why you felt the need to add these checks?

invalidate_inode_page can return 0 although the pages is !{dirty|writeback}. Look invalidate_complete_page. As easiest example, if the page has buffer and try_to_release_page can't release the buffer, it could return 0.

Ok, but somebody still tried to truncate the page, so why shouldn't we try to reclaim it?  The reason for deactivating at this location is that truncation is a strong hint for reclaim, not that it failed due to dirty/writeback pages.

What's the problem with deactivating pages where try_to_release_page() failed?

Quoted 6 lines

Quoted 2 lines

If try_to_release_page fails and the such pages stay long time in pagevec, pagevec drain often happens.

You mean because the pagevec becomes full more often?  These are not many pages you get extra without the checks, the race window is very small after all.

Right.
It was a totally bad answer. The work in midnight makes my mind to be hurt. :)

Quoted 8 lines

Another point is that we can move such pages(!try_to_release_page, someone else holding the ref) into tail of inactive.
We can't expect such pages will be freed sooner or later and it can stir lru pages unnecessary.
On the other hand it's a _really_ rare so couldn't we move the pages into tail? If it can be justified, I will remove the check.
What do you think about it?

I wonder ...how about adding "victim" list for "Reclaim" pages ? Then, we don't need extra LRU rotation.

Thanks,
-Kame

Hi Kame,

Quoted 2 lines

I wonder ...how about adding "victim" list for "Reclaim" pages ? Then, we don't need extra LRU rotation.

It can make the code clean.
As far as I think, victim list does following as.

1. select victim pages by strong hint
2. move the page from LRU to victim
3. reclaimer always peeks victim list before diving into LRU list. 4-1. If the victim pages is used by others or dirty, it can be moved into LRU, again or remain the page in victim list.
If the page is remained victim, when do we move it into LRU again if the reclaimer continues to fail the page?

We have to put the new rule.
4-2. If the victim pages isn't used by others and clean, we can reclaim the page asap.

AFAIK, strong hints are just two(invalidation, readahead max window heuristic). I am not sure it's valuable to add new hierarchy(ie, LRU, victim, unevictable) for cleaning the minor codes.
In addition, we have to put the new rule so it would make the LRU code complicated.
I remember how unevictable feature merge is hard.

But I am not against if we have more usecases. In this case, it's valuable to implement it although it's not easy.

Quoted 2 lines · On Wed, 8 Dec 2010 10:43:08 +0900 Minchan Kim <[email protected]> wrote:

Hi Kame,

Hi,

Quoted 6 lines

Quoted 2 lines

I wonder ...how about adding "victim" list for "Reclaim" pages ? Then, we don't need extra LRU rotation.

It can make the code clean.
As far as I think, victim list does following as.

Quoted 7 lines

1. select victim pages by strong hint
2. move the page from LRU to victim
3. reclaimer always peeks victim list before diving into LRU list. 4-1. If the victim pages is used by others or dirty, it can be moved into LRU, again or remain the page in victim list.
If the page is remained victim, when do we move it into LRU again if the reclaimer continues to fail the page?

When sometone touches it.

Quoted 11 lines

We have to put the new rule.
4-2. If the victim pages isn't used by others and clean, we can reclaim the page asap.

AFAIK, strong hints are just two(invalidation, readahead max window heuristic). I am not sure it's valuable to add new hierarchy(ie, LRU, victim, unevictable) for cleaning the minor codes.
In addition, we have to put the new rule so it would make the LRU code complicated.
I remember how unevictable feature merge is hard.

yes, it was hard.

Quoted 3 lines

But I am not against if we have more usecases. In this case, it's valuable to implement it although it's not easy.

I wonder "victim list" can be used for something like Cleancache, when we have very-low-latency backend devices.
And we may able to have page-cache-limit, which Balbir proposed as.

- kvictimed? will move unmappedd page caches to victim list
This may work like a InactiveClean list which we had before and make sizing easy.

Thanks,
-Kame

Quoted 35 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Wed, 8 Dec 2010 10:43:08 +0900 Minchan Kim <[email protected]> wrote:

Hi Kame,

Hi,

Quoted 6 lines

Quoted 2 lines

I wonder ...how about adding "victim" list for "Reclaim" pages ? Then, we don't need extra LRU rotation.

It can make the code clean.
As far as I think, victim list does following as.

Quoted 7 lines

1. select victim pages by strong hint
2. move the page from LRU to victim
3. reclaimer always peeks victim list before diving into LRU list. 4-1. If the victim pages is used by others or dirty, it can be moved into LRU, again or remain the page in victim list.
If the page is remained victim, when do we move it into LRU again if the reclaimer continues to fail the page?

When sometone touches it.

Quoted 11 lines

We have to put the new rule.
4-2. If the victim pages isn't used by others and clean, we can reclaim the page asap.

AFAIK, strong hints are just two(invalidation, readahead max window heuristic). I am not sure it's valuable to add new hierarchy(ie, LRU, victim, unevictable) for cleaning the minor codes.
In addition, we have to put the new rule so it would make the LRU code complicated.
I remember how unevictable feature merge is hard.

yes, it was hard.

Quoted 7 lines

Quoted 3 lines

But I am not against if we have more usecases. In this case, it's valuable to implement it although it's not easy.

I wonder "victim list" can be used for something like Cleancache, when we have very-low-latency backend devices.
And we may able to have page-cache-limit, which Balbir proposed as.

Yes, I thought that, too. I think it would be a good feature in embedded system.

Quoted 5 lines

- kvictimed? will move unmappedd page caches to victim list
This may work like a InactiveClean list which we had before and make sizing easy.

Before further discuss, we need customer's confirm.
We know very well it is very hard to merge if anyone doesn't use.

Balbir, What do think about it?

Quoted 7 lines

Thanks,
-Kame

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 56 lines · MinChan Kim

Quoted 35 lines · KAMEZAWA Hiroyuki

Quoted 2 lines · On Wed, 8 Dec 2010 10:43:08 +0900 Minchan Kim <[email protected]> wrote:

Hi Kame,

Hi,

Quoted 6 lines

Quoted 2 lines

I wonder ...how about adding "victim" list for "Reclaim" pages ? Then, we don't need extra LRU rotation.

It can make the code clean.
As far as I think, victim list does following as.

Quoted 7 lines

1. select victim pages by strong hint
2. move the page from LRU to victim
3. reclaimer always peeks victim list before diving into LRU list. 4-1. If the victim pages is used by others or dirty, it can be moved into LRU, again or remain the page in victim list.
If the page is remained victim, when do we move it into LRU again if the reclaimer continues to fail the page?

When sometone touches it.

Quoted 11 lines

We have to put the new rule.
4-2. If the victim pages isn't used by others and clean, we can reclaim the page asap.

AFAIK, strong hints are just two(invalidation, readahead max window heuristic). I am not sure it's valuable to add new hierarchy(ie, LRU, victim, unevictable) for cleaning the minor codes.
In addition, we have to put the new rule so it would make the LRU code complicated.
I remember how unevictable feature merge is hard.

yes, it was hard.

Quoted 7 lines

Quoted 3 lines

But I am not against if we have more usecases. In this case, it's valuable to implement it although it's not easy.

I wonder "victim list" can be used for something like Cleancache, when we have very-low-latency backend devices.
And we may able to have page-cache-limit, which Balbir proposed as.

Yes, I thought that, too. I think it would be a good feature in embedded system.

Quoted 5 lines

 - kvictimed? will move unmappedd page caches to victim list This may work like a InactiveClean list which we had before and make sizing easy.

Before further discuss, we need customer's confirm.
We know very well it is very hard to merge if anyone doesn't use.

Quoted 2 lines

Balbir, What do think about it?

The idea seems interesting, I am in the process of refreshing my patches for unmapped page cache control. I presume the process of filling the victim list will be similar to what I have or unmapped page cache isolation.

Quoted 11 lines

Quoted 8 lines

Thanks,
-Kame

Quoted 3 lines

--
Kind regards,
Minchan Kim

--
       Three Cheers,
       Balbir

Golbal page reclaim moves reclaimalbe pages into inactive list to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Golbal page reclaim moves reclaimalbe pages into inactive list to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh<[email protected]>

Cc KAMEZAWA Hiroyuki<[email protected]>

Cc Rik van Riel<[email protected]>

Cc KOSAKI Motohiro<[email protected]>

Signed-off-by Minchan Kim<[email protected]>

Quoted 4 lines · Minchan Kim

Golbal page reclaim moves reclaimalbe pages into inactive list to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

The patch is right, but the description is wrong.

The rotate_reclaimable_page function moves just written out pages, which the VM wanted to reclaim, to the end of the inactive list.  That way the VM will find those pages first next time it needs to free memory.

Quoted 6 lines

Cc Balbir Singh<[email protected]>

Cc KAMEZAWA Hiroyuki<[email protected]>

Cc Rik van Riel<[email protected]>

Cc KOSAKI Motohiro<[email protected]>

Signed-off-by Minchan Kim<[email protected]>

Reviewed-by Rik van Riel <[email protected]>

--
All rights reversed

Golbal page reclaim moves reclaimalbe pages into inactive list

to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/memcontrol.h |    6 ++++++
 mm/memcontrol.c            |   27 +++++++++++++++++++++++++++
 mm/swap.c                  |    3 ++-
 3 files changed, 35 insertions(+), 1 deletions(-)

include/linux/memcontrol.h +6 -0

index 067115c..8317f5c 100644
@@ -62,6 +62,7 @@ extern int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
 					gfp_t gfp_mask);
 extern void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru);
 extern void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru);
+extern void mem_cgroup_rotate_reclaimable_page(struct page *page);
 extern void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru);
 extern void mem_cgroup_del_lru(struct page *page);
 extern void mem_cgroup_move_lists(struct page *page,

@@ -207,6 +208,11 @@ static inline void mem_cgroup_del_lru_list(struct page *page, int lru)
 	return ;
 }
 
+static inline inline void mem_cgroup_rotate_reclaimable_page(struct page *page)
+{
+	return ;
+}
+
 static inline void mem_cgroup_rotate_lru_list(struct page *page, int lru)
 {
 	return ;

mm/memcontrol.c +27 -0

index 729beb7..f9435be 100644
@@ -829,6 +829,33 @@ void mem_cgroup_del_lru(struct page *page)
 	mem_cgroup_del_lru_list(page, page_lru(page));
 }
 
+/*
+ * Writeback is about to end against a page which has been marked for immediate
+ * reclaim.  If it still appears to be reclaimable, move it to the tail of the
+ * inactive list.
+ */
+void mem_cgroup_rotate_reclaimable_page(struct page *page)
+{
+	struct mem_cgroup_per_zone *mz;
+	struct page_cgroup *pc;
+	enum lru_list lru = page_lru_base_type(page);
+
+	if (mem_cgroup_disabled())
+		return;
+
+	pc = lookup_page_cgroup(page);
+	/*
+	 * Used bit is set without atomic ops but after smp_wmb().
+	 * For making pc->mem_cgroup visible, insert smp_rmb() here.
+	 */
+	smp_rmb();
+	/* unused or root page is not rotated. */
+	if (!PageCgroupUsed(pc) || mem_cgroup_is_root(pc->mem_cgroup))
+		return;
+	mz = page_cgroup_zoneinfo(pc);
+	list_move_tail(&pc->lru, &mz->lists[lru]);
+}
+
 void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru)
 {
 	struct mem_cgroup_per_zone *mz;

mm/swap.c +0 -0

index 1f36f6f..0fe98e7 100644

Quoted 1 line · MinChan Kim

Golbal page reclaim moves reclaimalbe pages into inactive list

Some typos here and Rik already pointed out some other changes.

Quoted 86 lines

to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

---
 include/linux/memcontrol.h |    6 ++++++ mm/memcontrol.c            |   27 +++++++++++++++++++++++++++ mm/swap.c                  |    3 ++-

 3 files changed, 35 insertions(+), 1 deletions(-)

include/linux/memcontrol.h +6 -0

index 067115c..8317f5c 100644
@@ -62,6 +62,7 @@ extern int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
                                       gfp_t gfp_mask);
  extern void mem_cgroup_add_lru_list(struct page *page, enum lru_list lru);
  extern void mem_cgroup_del_lru_list(struct page *page, enum lru_list lru);
+extern void mem_cgroup_rotate_reclaimable_page(struct page *page);
  extern void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru);
  extern void mem_cgroup_del_lru(struct page *page);
  extern void mem_cgroup_move_lists(struct page *page,

@@ -207,6 +208,11 @@ static inline void mem_cgroup_del_lru_list(struct page *page, int lru)
       return ;
  }
 
+static inline inline void mem_cgroup_rotate_reclaimable_page(struct page *page)
+{
+     return ;
+}
+
  static inline void mem_cgroup_rotate_lru_list(struct page *page, int lru)
  {
       return ;

mm/memcontrol.c +27 -0

index 729beb7..f9435be 100644
@@ -829,6 +829,33 @@ void mem_cgroup_del_lru(struct page *page)
       mem_cgroup_del_lru_list(page, page_lru(page));
  }
 
+/*
+ * Writeback is about to end against a page which has been marked for immediate
+ * reclaim.  If it still appears to be reclaimable, move it to the tail of the
+ * inactive list.
+ */
+void mem_cgroup_rotate_reclaimable_page(struct page *page)
+{
+     struct mem_cgroup_per_zone *mz;
+     struct page_cgroup *pc;
+     enum lru_list lru = page_lru_base_type(page);
+
+     if (mem_cgroup_disabled())
+             return;
+
+     pc = lookup_page_cgroup(page);
+     /*
+      * Used bit is set without atomic ops but after smp_wmb().
+      * For making pc->mem_cgroup visible, insert smp_rmb() here.
+      */
+     smp_rmb();
+     /* unused or root page is not rotated. */
+     if (!PageCgroupUsed(pc) || mem_cgroup_is_root(pc->mem_cgroup))
+             return;
+     mz = page_cgroup_zoneinfo(pc);
+     list_move_tail(&pc->lru, &mz->lists[lru]);
+}
+
  void mem_cgroup_rotate_lru_list(struct page *page, enum lru_list lru)
  {
       struct mem_cgroup_per_zone *mz;

mm/swap.c +0 -0

index 1f36f6f..0fe98e7 100644
@@ -122,8 +122,9 @@ static void pagevec_move_tail(struct pagevec *pvec)
               }

              if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
-                     int lru = page_lru_base_type(page);
+                     enum lru_list lru = page_lru_base_type(page);

Quoted 5 lines

                      list_move_tail(&page->lru, &zone->lru[lru].list);
+                     mem_cgroup_rotate_reclaimable_page(page); pgmoved++;
              }
      }

Looks good, do you have any numbers, workloads that benefit? I agree that keeping both global and memcg reclaim in sync is a good idea.

Acked-by Balbir Singh <[email protected]>

--
       Three Cheers,
       Balbir

Golbal page reclaim moves reclaimalbe pages into inactive list to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

Golbal page reclaim moves reclaimalbe pages into inactive list to reclaim asap. This patch apply the rule in memcg.
It can help to prevent unnecessary working page eviction of memcg.

Cc Balbir Singh <[email protected]>

Cc KAMEZAWA Hiroyuki <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Signed-off-by Minchan Kim <[email protected]>

mm/swap.c +0 -0

@@ -275,26 +275,59 @@ void add_page_to_unevictable_list(struct page *page)
  * head of the list, rather than the tail, to give the flusher

mm/swap.c +14 -3

+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be write it out by flusher threads as this is much more effective
+ * than the single-page writeout from reclaim.
  */
 static void lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
+	bool active;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
 
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
 
+	active = PageActive(page);
+
 	file = page_is_file_cache(page);
 	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
+	del_page_from_lru_list(zone, page, lru + active);
 	ClearPageActive(page);
 	ClearPageReferenced(page);
 	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
 
+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window

Quoted 4 lines · Minchan Kim

mm/swap.c +0 -0

@@ -275,26 +275,59 @@ void add_page_to_unevictable_list(struct page *page)
  * head of the list, rather than the tail, to give the flusher

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 285

Quoted 10 lines

mm/swap.c +8 -0

  * threads some time to write it out, as this is much more
  * effective than the single-page writeout from reclaim.
+ *
+ * If the page isn't page_mapped and dirty/writeback, the page
+ * could reclaim asap using PG_reclaim.
+ *
+ * 1. active, mapped page -> none
+ * 2. active, dirty/writeback page -> inactive, head, PG_reclaim
+ * 3. inactive, mapped page -> none
+ * 4. inactive, dirty/writeback page -> inactive, head, PG_reclaim

         inactive, clean -> inactive, tail

Quoted 31 lines

mm/swap.c +10 -3

+ * 5. Others -> none
+ *
+ * In 4, why it moves inactive's head, the VM expects the page would
+ * be write it out by flusher threads as this is much more effective
+ * than the single-page writeout from reclaim.
  */
 static void lru_deactivate(struct page *page, struct zone *zone)
 {
 	int lru, file;
+	bool active;
 
-	if (!PageLRU(page) || !PageActive(page))
+	if (!PageLRU(page))
 		return;
 
 	/* Some processes are using the page */
 	if (page_mapped(page))
 		return;
 
+	active = PageActive(page);
+
 	file = page_is_file_cache(page);
 	lru = page_lru_base_type(page);
-	del_page_from_lru_list(zone, page, lru + LRU_ACTIVE);
+	del_page_from_lru_list(zone, page, lru + active);
 	ClearPageActive(page);
 	ClearPageReferenced(page);
 	add_page_to_lru_list(zone, page, lru);
-	__count_vm_event(PGDEACTIVATE);
 

Quoted 4 lines

mm/swap.c +4 -0

+	if (PageWriteback(page) || PageDirty(page)) {
+		/*
+		 * PG_reclaim could be raced with end_page_writeback
+		 * It can make readahead confusing.  But race window

mm/swap.c void add_page_to_unevictable_list(struct page *page) · line 326

Quoted 10 lines

mm/swap.c +10 -0

+		 * is _really_ small and  it's non-critical problem.
+		 */
+		SetPageReclaim(page);
+	} else {
+		/*
+		 * The page's writeback ends up during pagevec
+		 * We moves tha page into tail of inactive.
+		 */
+		list_move_tail(&page->lru, &zone->lru[lru].list);
+		mem_cgroup_rotate_reclaimable_page(page);

I think you also need to increase PGROTATED here.

Other than that,

Reviewed-by Johannes Weiner <[email protected]>

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Acked-by Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Quoted 23 lines

invalidate_mapping_pages is very big hint to reclaimer. It means user doesn't want to use the page any more. So in order to prevent working set page eviction, this patch move the page into tail of inactive list by PG_reclaim.

Please, remember that pages in inactive list are working set as well as active list. If we don't move pages into inactive list's tail, pages near by tail of inactive list can be evicted although we have a big clue about useless pages. It's totally bad.

Now PG_readahead/PG_reclaim is shared.
fe3cba17 added ClearPageReclaim into clear_page_dirty_for_io for preventing fast reclaiming readahead marker page.

In this series, PG_reclaim is used by invalidated page, too. If VM find the page is invalidated and it's dirty, it sets PG_reclaim to reclaim asap. Then, when the dirty page will be writeback, clear_page_dirty_for_io will clear PG_reclaim unconditionally. It disturbs this serie's goal.

I think it's okay to clear PG_readahead when the page is dirty, not writeback time. So this patch moves ClearPageReadahead.

Quoted 7 lines

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Acked-by Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Until anyone should data, I will not ack this. This patch increase VM state, but benefit is doubious.

Make sense to me. If Ben is busy, I will measure it and send the result.

Quoted 1 line

Make sense to me. If Ben is busy, I will measure it and send the result.

I've done measurements on the patched kernel. All that remains is to do measurements on the baseline unpached case. To summarize the results thusfar,

Times:
=======
                      user    sys     %cpu    inputs           outputs Patched, drop          142     64      46      13557744         14052744 Patched, nodrop        55      57      33      13557936         13556680

vmstat:
========
                       free_pages      inact_anon      act_anon        inact_file      act_file        dirtied      written  reclaim Patched, drop, pre      306043          37541           185463          276266          153955          3689674      3604959  1550641 Patched, drop, post     13233           38462           175252          536346          178792          5527564      5371563  3169155

Patched, nodrop, pre    475211          38602           175242          81979           178820          5527592      5371554  3169155 Patched, nodrop, post   7697            38959           176986          547984          180855          7324836      7132158  3169155

Altogether, it seems that something is horribly wrong, most likely with my test (or rsync patch). I'll do the baseline benchmarks today.

Thoughts?

Thanks,

- Ben

Quoted 18 lines · Ben Gamari

Quoted 1 line

Make sense to me. If Ben is busy, I will measure it and send the result.

I've done measurements on the patched kernel. All that remains is to do measurements on the baseline unpached case. To summarize the results thusfar,

Times:
=======
? ? ? ? ? ? ? ? ? ? ? user ? ?sys ? ? %cpu ? ?inputs ? ? ? ? ? outputs Patched, drop ? ? ? ? ?142 ? ? 64 ? ? ?46 ? ? ?13557744 ? ? ? ? 14052744 Patched, nodrop ? ? ? ?55 ? ? ?57 ? ? ?33 ? ? ?13557936 ? ? ? ? 13556680

vmstat:
========
? ? ? ? ? ? ? ? ? ? ? ?free_pages ? ? ?inact_anon ? ? ?act_anon ? ? ? ?inact_file ? ? ?act_file ? ? ? ?dirtied ? ? ?written ?reclaim Patched, drop, pre ? ? ?306043 ? ? ? ? ?37541 ? ? ? ? ? 185463 ? ? ? ? ?276266 ? ? ? ? ?153955 ? ? ? ? ?3689674 ? ? ?3604959 ?1550641 Patched, drop, post ? ? 13233 ? ? ? ? ? 38462 ? ? ? ? ? 175252 ? ? ? ? ?536346 ? ? ? ? ?178792 ? ? ? ? ?5527564 ? ? ?5371563 ?3169155

Quoted 7 lines

Patched, nodrop, pre ? ?475211 ? ? ? ? ?38602 ? ? ? ? ? 175242 ? ? ? ? ?81979 ? ? ? ? ? 178820 ? ? ? ? ?5527592 ? ? ?5371554 ?3169155 Patched, nodrop, post ? 7697 ? ? ? ? ? ?38959 ? ? ? ? ? 176986 ? ? ? ? ?547984 ? ? ? ? ?180855 ? ? ? ? ?7324836 ? ? ?7132158 ?3169155

Altogether, it seems that something is horribly wrong, most likely with my test (or rsync patch). I'll do the baseline benchmarks today.

Thoughts?

How do you test it?
I think patch's effect would be good in big memory pressure environment.

Quickly I did it on my desktop environment.(2G DRAM)
So it's not completed result. I will test more when out of office.

Used kernel : mmotm-12-02 + my patch series
Used rsync :
1. rsync_normal : v3.0.7 vanilla
2. rsync_patch : v3.0.7 + Ben's patch(fadvise)

Test scenario :
* kernel full compile
* git clone linux-kernel
* rsync local host directory to local host dst directory

1) rsync_normal : 89.08user 127.48system 33:22.24elapsed
2) rsync_patch : 88.42user 135.26system 31:30.56elapsed

1) rsync_normal vmstat :
pgfault : 45538203
pgmajfault : 4181

pgactivate 377416
pgdeactivate 34183
pginvalidate 0
pgreclaim 0

pgsteal_dma 0
pgsteal_normal 2144469
pgsteal_high 2884412
pgsteal_movable 0
pgscan_kswapd_dma 0
pgscan_kswapd_normal 2149739
pgscan_kswapd_high 2909140
pgscan_kswapd_movable 0
pgscan_direct_dma 0
pgscan_direct_normal 647
pgscan_direct_high 716
pgscan_direct_movable 0
pginodesteal 0
slabs_scanned 1737344
kswapd_steal 5028353
kswapd_inodesteal 438910
pageoutrun 81208
allocstall 9
pgrotated 1642

2) rsync_patch vmstat:

pgfault : 47570231
pgmajfault : 2669

pgactivate 391806
pgdeactivate 36861
pginvalidate 1685065
pgreclaim 1685065

pgrefill_dma 0
pgrefill_normal 32025
pgrefill_high 9619
pgrefill_movable 0
pgsteal_dma 0
pgsteal_normal 744904
pgsteal_high 1079709
pgsteal_movable 0
pgscan_kswapd_dma 0
pgscan_kswapd_normal 745017
pgscan_kswapd_high 1096660
pgscan_kswapd_movable 0
pgscan_direct_dma 0
pgscan_direct_normal 0
pgscan_direct_high 0
pgscan_direct_movable 0
pginodesteal 0
slabs_scanned 1896960
kswapd_steal 1824613
kswapd_inodesteal 703499
pageoutrun 26828
allocstall 0
pgrotated 1681570

In summary,
Unfortunately, the number of fault is increased (47570231 - 45538203) but pgmajfault is reduced (4181 - 2669).

The number of scanning is much reduced. 2149739 -> 745017, 2909140 -> 1096660 and even no direct reclaim in patched rsync.

The number of steal is much reduced. 2144469 -> 744904, 2884412 -> 1079709, 5028353 -> 1824613.

The elapsed time is reduced 2 minutes.

I think result is good. Reduced the steal number could imply prevent eviction of working set pages.

It has a good result with small effort(small scanning).

I will resend with more exact measurement after repeated test.

Quoted 5 lines

Thanks,

- Ben

Signature · 3 lines

-- 
Kind regards,
Minchan Kim

In summary, my patch enhances a littie bit about elapsed time in memory pressure environment and enhance reclaim effectivness(reclaim/reclaim) with x2. It means reclaim latency is short and doesn't evict working set pages due to invalidated pages.

Look at reclaim effectivness. Patched rsync enhances x2 about reclaim effectiveness and compared to mmotm-12-03, mmotm-12-03-fadvise enhances 3 minute about elapsed time in stress environment.
I think it's due to reduce scanning, reclaim overhead.

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Quoted 5 lines · Minchan Kim

In summary, my patch enhances a littie bit about elapsed time in memory pressure environment and enhance reclaim effectivness(reclaim/reclaim) with x2. It means reclaim latency is short and doesn't evict working set pages due to invalidated pages.

Thank you very much for this testing! I'm very sorry I've been unable to contribute more recently. My last exam is on Wednesday and besides some grading that is the end of the semester.  Is there anything you would

like me to do? Perhaps reproducing these results on my setup would be useful?

Quoted 6 lines

Look at reclaim effectivness. Patched rsync enhances x2 about reclaim effectiveness and compared to mmotm-12-03, mmotm-12-03-fadvise enhances 3 minute about elapsed time in stress environment.
I think it's due to reduce scanning, reclaim overhead.

Good good. This looks quite promising.

Quoted 5 lines

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Hmm, nothing comes to mind. As I've said in the past, rsync should require each page only once. Perhaps perf might offer some insight into where this time is being spent?

- Ben

Hi Ben,

[snipped]

Quoted 3 lines · Minchan Kim

Hi Ben,

[snipped]

What exactly happened to this set? It seems it dropped off my radar and otherwise never was merged. Was it not tested enough, the benefit not great enough, or did it simply slip through the cracks?

Test Environment :
DRAM : 2G, CPU : Intel(R) Core(TM)2 CPU
Rsync backup directory size : 16G

rsync version is 3.0.7.
rsync patch is Ben's fadivse.
stress scenario do following jobs with parallel.

1. make all -j4 linux, git clone linux-kernel
2. git clone linux-kernel
3. rsync src dst

nrns : no-patched rsync + no stress
prns : patched rsync + no stress
nrs : no-patched rsync + stress
prs : patched rsync + stress

pginvalidate : the number of dirty/writeback pages which is invalidated by fadvise pgreclaim : pages moved PG_reclaim trick in inactive's tail

In summary, my patch enhances a littie bit about elapsed time in memory pressure environment and enhance reclaim effectivness(reclaim/reclaim) with x2. It means reclaim latency is short and doesn't evict working set pages due to invalidated pages.

Look at reclaim effectivness. Patched rsync enhances x2 about reclaim effectiveness and compared to mmotm-12-03, mmotm-12-03-fadvise enhances 3 minute about elapsed time in stress environment.
I think it's due to reduce scanning, reclaim overhead.

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Before futher work, I hope listen opinions.
Any comment is welcome.

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Quoted 34 lines · On Tue, 14 Dec 2010 00:31:05 +0900 Minchan Kim <[email protected]> wrote:

Test Environment :
DRAM : 2G, CPU : Intel(R) Core(TM)2 CPU
Rsync backup directory size : 16G

rsync version is 3.0.7.
rsync patch is Ben's fadivse.
stress scenario do following jobs with parallel.

1. make all -j4 linux, git clone linux-kernel
2. git clone linux-kernel
3. rsync src dst

nrns : no-patched rsync + no stress
prns : patched rsync + no stress
nrs  : no-patched rsync + stress
prs  : patched rsync + stress

pginvalidate : the number of dirty/writeback pages which is invalidated by fadvise pgreclaim : pages moved PG_reclaim trick in inactive's tail

In summary, my patch enhances a littie bit about elapsed time in memory pressure environment and enhance reclaim effectivness(reclaim/reclaim) with x2. It means reclaim latency is short and doesn't evict working set pages due to invalidated pages.

Look at reclaim effectivness. Patched rsync enhances x2 about reclaim effectiveness and compared to mmotm-12-03, mmotm-12-03-fadvise enhances 3 minute about elapsed time in stress environment.
I think it's due to reduce scanning, reclaim overhead.

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Quoted 3 lines

Before futher work, I hope listen opinions.
Any comment is welcome.

At first, the improvement seems great. Thank you for your effort.

Quoted 5 lines

In no-stress enviroment, fadivse makes program little bit slow. I think because there are many pgfault. I don't know why it happens. Could you guess why it happens?

Are there no program which accesses a directory rsync'ed ?

Thanks,
-Kame

This patch adds profile information about invalidated page reclaim. It's just for profiling for test so it would be discard when the series are merged.

Signed-off-by Minchan Kim<[email protected]>

Cc Rik van Riel<[email protected]>

Cc KOSAKI Motohiro<[email protected]>

Cc Wu Fengguang<[email protected]>

Cc Johannes Weiner<[email protected]>

Cc Nick Piggin<[email protected]>

Cc Mel Gorman<[email protected]>

This patch adds profile information about invalidated page reclaim. It's just for profiling for test so it would be discard when the series are merged.

Signed-off-by Minchan Kim <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

---
 include/linux/vmstat.h |    4 ++--
 mm/swap.c              |    3 +++
 mm/vmstat.c            |    3 +++
 3 files changed, 8 insertions(+), 2 deletions(-)

include/linux/vmstat.h +2 -2

index 833e676..c38ad95 100644
@@ -30,8 +30,8 @@
 
 enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		FOR_ALL_ZONES(PGALLOC),
-		PGFREE, PGACTIVATE, PGDEACTIVATE,
-		PGFAULT, PGMAJFAULT,
+		PGFREE, PGACTIVATE, PGDEACTIVATE, PGINVALIDATE,
+		PGRECLAIM, PGFAULT, PGMAJFAULT,
 		FOR_ALL_ZONES(PGREFILL),
 		FOR_ALL_ZONES(PGSTEAL),
 		FOR_ALL_ZONES(PGSCAN_KSWAPD),

mm/swap.c +0 -0

index 0f23998..2f21e6e 100644

mm/swap.c +0 -0

 }
 

mm/vmstat.c +3 -0

index 3555636..ef6102d 100644
@@ -818,6 +818,9 @@ static const char * const vmstat_text[] = {
 	"pgactivate",
 	"pgdeactivate",
 
+	"pginvalidate",
+	"pgreclaim",
+
 	"pgfault",
 	"pgmajfault",
 

Signature · 1 line

1.7.0.4

Quoted 11 lines

This patch adds profile information about invalidated page reclaim. It's just for profiling for test so it would be discard when the series are merged.

Signed-off-by Minchan Kim <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Quoted 5 lines

---
 include/linux/vmstat.h |    4 ++--
 mm/swap.c              |    3 +++
 mm/vmstat.c            |    3 +++

 3 files changed, 8 insertions(+), 2 deletions(-)

Today, we have tracepoint. tracepoint has no overhead if it's unused. but vmstat has a overhead even if unused.

Then, all new vmstat proposal should be described why you think it is frequently used from administrators.

Hi KOSAKI,

Quoted 17 lines · KOSAKI Motohiro

Quoted 16 lines

This patch adds profile information about invalidated page reclaim. It's just for profiling for test so it would be discard when the series are merged.

Signed-off-by Minchan Kim <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

---
 include/linux/vmstat.h |    4 ++--
 mm/swap.c              |    3 +++
 mm/vmstat.c            |    3 +++

 3 files changed, 8 insertions(+), 2 deletions(-)

Quoted 5 lines

Today, we have tracepoint. tracepoint has no overhead if it's unused. but vmstat has a overhead even if unused.

Then, all new vmstat proposal should be described why you think it is frequently used from administrators.

It's just for easy gathering the data when Ben will test. I never want to merge it in upstream and even mmtom.

If you don't like it for just testing, I am happy to change it with tracepoint.

Some functions used zap_details depends on assumption that zap_details parameter should be NULLed if some fields are 0.

This patch removes that dependency for next patch easy review/merge. It should not chanage behavior.

Signed-off-by Minchan Kim<[email protected]>

Cc Rik van Riel<[email protected]>

Cc KOSAKI Motohiro<[email protected]>

Cc Johannes Weiner<[email protected]>

Cc Nick Piggin<[email protected]>

Cc Mel Gorman<[email protected]>

Cc Wu Fengguang<[email protected]>

Cc Hugh Dickins<[email protected]>

Some functions used zap_details depends on assumption that zap_details parameter should be NULLed if some fields are 0.

This patch removes that dependency for next patch easy review/merge. It should not chanage behavior.

Signed-off-by Minchan Kim <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Hugh Dickins <[email protected]>

---
 include/linux/mm.h |    8 ++++++++
 mm/madvise.c       |   15 +++++++++------
 mm/memory.c        |   14 ++++++++------
 mm/mmap.c          |    6 ++++--
 4 files changed, 29 insertions(+), 14 deletions(-)

include/linux/mm.h +0 -0

index e097df6..6522ae4 100644

include/linux/mm.h +1 -0

+
 struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
 		pte_t pte);
 

mm/madvise.c +2 -4

index 319528b..bfa17aa 100644
@@ -162,18 +162,21 @@ static long madvise_dontneed(struct vm_area_struct * vma,
 			     struct vm_area_struct ** prev,
 			     unsigned long start, unsigned long end)
 {
+	DEFINE_ZAP_DETAILS(details);
+
 	*prev = vma;
 	if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
 		return -EINVAL;
 
 	if (unlikely(vma->vm_flags & VM_NONLINEAR)) {
-		struct zap_details details = {
-			.nonlinear_vma = vma,
-			.last_index = ULONG_MAX,
-		};

mm/madvise.c +0 -0

 	return 0;
 }
 

mm/memory.c +0 -0

index ebfeedf..c0879bb 100644

mm/memory.c +3 -2

 	if (address < vma->vm_start || address + size > vma->vm_end ||
 	    		!(vma->vm_flags & VM_PFNMAP))
 		return -1;
-	zap_page_range(vma, address, size, NULL);
+	zap_page_range(vma, address, size, &details);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(zap_vma_ptes);

@@ -2577,7 +2578,8 @@ restart:
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows)
 {
-	struct zap_details details;
+	DEFINE_ZAP_DETAILS(details);
+
 	pgoff_t hba = holebegin >> PAGE_SHIFT;
 	pgoff_t hlen = (holelen + PAGE_SIZE - 1) >> PAGE_SHIFT;
 

mm/mmap.c +0 -0

index b179abb..31d2594 100644

mm/mmap.c +1 -1

 	lru_add_drain();
 	tlb = tlb_gather_mmu(mm, 0);
 	update_hiwater_rss(mm);
-	unmap_vmas(&tlb, vma, start, end, &nr_accounted, NULL);
+	unmap_vmas(&tlb, vma, start, end, &nr_accounted, &details);
 	vm_unacct_memory(nr_accounted);
 	free_pgtables(tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
 				 next? next->vm_start: 0);

Quoted 6 lines · Minchan Kim

Some functions used zap_details depends on assumption that zap_details parameter should be NULLed if some fields are 0.

This patch removes that dependency for next patch easy review/merge. It should not chanage behavior.

Quoted 8 lines

Signed-off-by Minchan Kim <[email protected]>

Cc Rik van Riel <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Hugh Dickins <[email protected]>

Sorry, while I do like that you're now using the details block, you seem to be adding overhead in various places without actually simplifying anything - you insist that everything passes down an initialized details block, and then in the end force the pointer to NULL again in all the common cases.

Which seems odd.  I could understand if you were going to scrap the NULL details optimization altogether; but I think that (for the original optimization reasons) you're right to force it to NULL in the end, so then why initialize the block at all those call sites?

Quoted 15 lines

---
 include/linux/mm.h |    8 ++++++++
 mm/madvise.c       |   15 +++++++++------
 mm/memory.c        |   14 ++++++++------
 mm/mmap.c          |    6 ++++--

 4 files changed, 29 insertions(+), 14 deletions(-)

include/linux/mm.h +0 -0

index e097df6..6522ae4 100644
@@ -773,6 +773,14 @@ struct zap_details {

      unsigned long truncate_count;           /* Compare vm_truncate_count */ };

include/linux/mm.h struct zap_details { · line 782

Quoted 7 lines

include/linux/mm.h +7 -0

+#define __ZAP_DETAILS_INITIALIZER(name) \
+             { .nonlinear_vma = NULL \
+             , .check_mapping = NULL \
+             , .i_mmap_lock = NULL }
+
+#define DEFINE_ZAP_DETAILS(name)             \
+     struct zap_details name = __ZAP_DETAILS_INITIALIZER(name)

Okay.

Quoted 28 lines

+
 struct page *vm_normal_page(struct vm_area_struct *vma, unsigned long addr, pte_t pte);

mm/madvise.c +5 -5

index 319528b..bfa17aa 100644
@@ -162,18 +162,21 @@ static long madvise_dontneed(struct vm_area_struct * vma,
                            struct vm_area_struct ** prev,
                            unsigned long start, unsigned long end)
  {
+     DEFINE_ZAP_DETAILS(details);
+
       *prev = vma;
       if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))
               return -EINVAL;
 
       if (unlikely(vma->vm_flags & VM_NONLINEAR)) {
-             struct zap_details details = {
-                     .nonlinear_vma = vma,
-                     .last_index = ULONG_MAX,
-             };
+             details.nonlinear_vma = vma;
+             details.last_index = ULONG_MAX;
+
               zap_page_range(vma, start, end - start, &details);
-     } else

mm/madvise.c static long madvise_dontneed(struct vm_area_struct * vma, · line 179

Quoted 5 lines

mm/madvise.c +4 -1

-             zap_page_range(vma, start, end - start, NULL);
+     } else {
+
+             zap_page_range(vma, start, end - start, &details);
+     }

You end up with two identical zap_page_range() lines:
better have one after the if {} without an else.

Quoted 11 lines

mm/madvise.c +0 -0

       return 0;
  }
 

mm/memory.c +0 -0

index ebfeedf..c0879bb 100644
@@ -900,6 +900,9 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 
       init_rss_vec(rss);
 

mm/memory.c static unsigned long zap_pte_range(struct mmu_gather *tlb, · line 905

Quoted 3 lines

mm/memory.c +3 -0

+     if (!details->check_mapping && !details->nonlinear_vma)
+             details = NULL;
+

Aside from its necessity in the next patch, I thoroughly approve of your moving this optimization here: it is confusing, and better that it be done near where the fields are used, than off at the higher level.

mm/memory.c static unsigned long unmap_page_range(struct mmu_gather *tlb, · line 1043

Quoted 11 lines

mm/memory.c +0 -3

       pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
       arch_enter_lazy_mmu_mode();
       do {

@@ -1038,9 +1041,6 @@ static unsigned long unmap_page_range(struct mmu_gather *tlb,
       pgd_t *pgd;
       unsigned long next;
 
-     if (details && !details->check_mapping && !details->nonlinear_vma)
-             details = NULL;
-

Yes, I put it there because that was the highest point at which it could then be done, so it was optimal from a do-it-fewest-times point of view; but not at all helpful in understanding what's going on, much better as you have it.

Quoted 9 lines

mm/memory.c +0 -0

       BUG_ON(addr >= end);
       mem_cgroup_uncharge_start();
       tlb_start_vma(tlb, vma);

@@ -1102,7 +1102,7 @@ unsigned long unmap_vmas(struct mmu_gather **tlbp,

      unsigned long tlb_start = 0;    /* For tlb_finish_mmu */ int tlb_start_valid = 0;
      unsigned long start = start_addr;
-     spinlock_t *i_mmap_lock = details? details->i_mmap_lock: NULL;
+     spinlock_t *i_mmap_lock = details->i_mmap_lock;

This appears to be the sole improvement from insisting that everywhere sets up an initialized details block.  I don't think this is worth it.

mm/memory.c unsigned long zap_page_range(struct vm_area_struct *vma, unsigned long address, · line 1220

Quoted 8 lines

      int fullmm = (*tlbp)->fullmm;
      struct mm_struct *mm = vma->vm_mm;

mm/memory.c +1 -0

@@ -1217,10 +1217,11 @@ unsigned long zap_page_range(struct vm_area_struct *vma, unsigned long address,
  int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
               unsigned long size)
  {
+     DEFINE_ZAP_DETAILS(details);

Overhead.

Quoted 23 lines

      if (address < vma->vm_start || address + size > vma->vm_end || !(vma->vm_flags & VM_PFNMAP)) return -1;
-     zap_page_range(vma, address, size, NULL);
+     zap_page_range(vma, address, size, &details); return 0;
 }
 EXPORT_SYMBOL_GPL(zap_vma_ptes);

mm/memory.c +2 -1

@@ -2577,7 +2578,8 @@ restart:
  void unmap_mapping_range(struct address_space *mapping,
               loff_t const holebegin, loff_t const holelen, int even_cows)
  {
-     struct zap_details details;
+     DEFINE_ZAP_DETAILS(details);
+
       pgoff_t hba = holebegin >> PAGE_SHIFT;
       pgoff_t hlen = (holelen + PAGE_SIZE - 1) >> PAGE_SHIFT;
 

mm/mmap.c +0 -0

index b179abb..31d2594 100644

mm/mmap.c static void unmap_region(struct mm_struct *mm, · line 1903

Quoted 4 lines

mm/mmap.c +1 -0

@@ -1900,11 +1900,12 @@ static void unmap_region(struct mm_struct *mm,
       struct vm_area_struct *next = prev? prev->vm_next: mm->mmap;
       struct mmu_gather *tlb;
       unsigned long nr_accounted = 0;
+     DEFINE_ZAP_DETAILS(details);

Overhead.

Quoted 10 lines

mm/mmap.c +1 -1

       lru_add_drain();
       tlb = tlb_gather_mmu(mm, 0);
       update_hiwater_rss(mm);
-     unmap_vmas(&tlb, vma, start, end, &nr_accounted, NULL);
+     unmap_vmas(&tlb, vma, start, end, &nr_accounted, &details);
       vm_unacct_memory(nr_accounted);
       free_pgtables(tlb, vma, prev? prev->vm_end: FIRST_USER_ADDRESS,
                                next? next->vm_start: 0);

mm/mmap.c void exit_mmap(struct mm_struct *mm) · line 2258

Quoted 4 lines

mm/mmap.c +1 -0

@@ -2254,6 +2255,7 @@ void exit_mmap(struct mm_struct *mm)
       struct vm_area_struct *vma;
       unsigned long nr_accounted = 0;
       unsigned long end;
+     DEFINE_ZAP_DETAILS(details);

Overhead.

Quoted 11 lines

mm/mmap.c +1 -1

       /* mm's last user has gone, and its about to be pulled down */
       mmu_notifier_release(mm);

@@ -2278,7 +2280,7 @@ void exit_mmap(struct mm_struct *mm)
       tlb = tlb_gather_mmu(mm, 1);
       /* update_hiwater_rss(mm) here? but nobody should be looking */
       /* Use -1 here to ensure all VMAs in the mm are unmapped */
-     end = unmap_vmas(&tlb, vma, 0, -1, &nr_accounted, NULL);
+     end = unmap_vmas(&tlb, vma, 0, -1, &nr_accounted, &details);
       vm_unacct_memory(nr_accounted);
 

mm/mmap.c void exit_mmap(struct mm_struct *mm)

Quoted 2 lines

mm/mmap.c +0 -1

       free_pgtables(tlb, vma, FIRST_USER_ADDRESS, 0);
--

Am I being too fussy?

Hugh

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Reviewed-by KOSAKI Motohiro <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Hugh Dickins <[email protected]>

Changelog since v3:
- Change variable name - suggested by Johannes
- Union ignore_references with zap_details - suggested by Hugh

Changelog since v2:
- remove unnecessary description

Changelog since v1:
- change word from promote to activate
- add activate argument to zap_pte_range and family function

---
 include/linux/mm.h |    4 +++-
 mm/madvise.c       |    6 +++---
 mm/memory.c        |    5 ++++-
 3 files changed, 10 insertions(+), 5 deletions(-)

include/linux/mm.h +0 -0

index 6522ae4..e57190f 100644
@@ -771,12 +771,14 @@ struct zap_details {
 	pgoff_t last_index;			/* Highest page->index to unmap */
 	spinlock_t *i_mmap_lock;		/* For unmap_mapping_range: */

include/linux/mm.h +0 -0

 #define DEFINE_ZAP_DETAILS(name)		\
 	struct zap_details name = __ZAP_DETAILS_INITIALIZER(name)

mm/madvise.c +1 -0

index bfa17aa..8e7aba3 100644
@@ -163,6 +163,7 @@ static long madvise_dontneed(struct vm_area_struct * vma,
 			     unsigned long start, unsigned long end)
 {
 	DEFINE_ZAP_DETAILS(details);
+	details.ignore_references = true;
 
 	*prev = vma;
 	if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))

mm/madvise.c +0 -0

 	return 0;
 }
 

mm/memory.c +1 -0

index c0879bb..44d87e1 100644
@@ -897,6 +897,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 	pte_t *pte;
 	spinlock_t *ptl;
 	int rss[NR_MM_COUNTERS];
+	bool ignore_references = details->ignore_references;
 
 	init_rss_vec(rss);
 

Quoted 41 lines · Minchan Kim

Now zap_pte_range alwayas activates pages which are pte_young && !VM_SequentialReadHint(vma). But in case of calling MADV_DONTNEED, it's unnecessary since the page wouldn't use any more.

Signed-off-by Minchan Kim <[email protected]>

Acked-by Rik van Riel <[email protected]>

Reviewed-by KOSAKI Motohiro <[email protected]>

Cc KOSAKI Motohiro <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nick Piggin <[email protected]>

Cc Mel Gorman <[email protected]>

Cc Wu Fengguang <[email protected]>

Cc Hugh Dickins <[email protected]>

Changelog since v3:
 - Change variable name - suggested by Johannes
 - Union ignore_references with zap_details - suggested by Hugh

Changelog since v2:
 - remove unnecessary description

Changelog since v1:
 - change word from promote to activate
 - add activate argument to zap_pte_range and family function
---
 include/linux/mm.h |    4 +++-
 mm/madvise.c       |    6 +++---
 mm/memory.c        |    5 ++++-

 3 files changed, 10 insertions(+), 5 deletions(-)

include/linux/mm.h +0 -0

index 6522ae4..e57190f 100644
@@ -771,12 +771,14 @@ struct zap_details {

      pgoff_t last_index;                     /* Highest page->index to unmap */ spinlock_t *i_mmap_lock;                /* For unmap_mapping_range: */ unsigned long truncate_count;           /* Compare vm_truncate_count */
+     bool ignore_references;                 /* For page activation */ };

Quoted 6 lines

 #define __ZAP_DETAILS_INITIALIZER(name) \
                 { .nonlinear_vma = NULL \
              , .check_mapping = NULL \
-             , .i_mmap_lock = NULL }
+             , .i_mmap_lock = NULL   \
+             , .ignore_references = false }

Okay.

Quoted 16 lines

 #define DEFINE_ZAP_DETAILS(name)             \
      struct zap_details name = __ZAP_DETAILS_INITIALIZER(name)

mm/madvise.c +1 -0

index bfa17aa..8e7aba3 100644
@@ -163,6 +163,7 @@ static long madvise_dontneed(struct vm_area_struct * vma,
                            unsigned long start, unsigned long end)
  {
       DEFINE_ZAP_DETAILS(details);
+     details.ignore_references = true;
 
       *prev = vma;
       if (vma->vm_flags & (VM_LOCKED|VM_HUGETLB|VM_PFNMAP))

mm/madvise.c static long madvise_dontneed(struct vm_area_struct * vma, · line 179

Quoted 9 lines

mm/madvise.c +2 -3

@@ -173,10 +174,9 @@ static long madvise_dontneed(struct vm_area_struct * vma,
               details.last_index = ULONG_MAX;
 
               zap_page_range(vma, start, end - start, &details);
-     } else {
-
+     } else
               zap_page_range(vma, start, end - start, &details);
-     }
+

As in the previous patch, you have the same in the if {} and the else.

Quoted 18 lines

mm/madvise.c +0 -0

       return 0;
  }
 

mm/memory.c +1 -0

index c0879bb..44d87e1 100644
@@ -897,6 +897,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
       pte_t *pte;
       spinlock_t *ptl;
       int rss[NR_MM_COUNTERS];
+     bool ignore_references = details->ignore_references;
 
       init_rss_vec(rss);
 

@@ -952,7 +953,8 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
                               if (pte_dirty(ptent))
                                       set_page_dirty(page);

Quoted 4 lines

                              if (pte_young(ptent) &&
-                                 likely(!VM_SequentialReadHint(vma)))
+                                 likely(!VM_SequentialReadHint(vma)) &&
+                                     !ignore_references)

I think ignore_references is about as likely as VM_SequentialReadHint:
I'd probably just omit that "likely()" nowadays, but you might prefer to put your "|| !ignore_references" inside.

Hmm, actually it would probably be better to say something like

       mark_accessed = true;
       if (VM_SequentialReadHint(vma) || (details && details->ignore_references)) mark_accessed = false;

on entry to zap_pte_range().

Quoted 9 lines

                                      mark_page_accessed(page); rss[MM_FILEPAGES]--;
                      }

mm/memory.c +1 -0

@@ -1218,6 +1220,7 @@ int zap_vma_ptes(struct vm_area_struct *vma, unsigned long address,
               unsigned long size)
  {
       DEFINE_ZAP_DETAILS(details);
+     details.ignore_references = true;

      if (address < vma->vm_start || address + size > vma->vm_end ||

Quoted 3 lines

                      !(vma->vm_flags & VM_PFNMAP)) return -1;
--

Unnecessary here (would make more sense in the truncation case, but not necessary there either): zap_vma_ptes() is only being used on GRU's un-cowable VM_PFNMAP area, so vm_normal_page() won't even give you a non-NULL page to mark.

Hugh