respect the referenced bit of KVM guest pages?

Greetings,

Jeff Dike found that many KVM pages are being refaulted in 2.6.29:

"Lots of pages between discarded due to memory pressure only to be faulted back in soon after. These pages are nearly all stack pages. This is not consistent - sometimes there are relatively few such pages and they are spread out between processes."

The refaults can be drastically reduced by the following patch, which respects the referenced bit of all anonymous pages (including the KVM pages).

However it risks reintroducing the problem addressed by commit 7e9cd4842 (fix reclaim scalability problem by ignoring the referenced bit, mainly the pte young bit). I wonder if there are better solutions?

Thanks,
Fengguang

---
 mm/vmscan.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/vmscan.c +0 -4

@@ -1288,12 +1288,12 @@ static void shrink_active_list(unsigned 
 			 * Identify referenced, file-backed active pages and
 			 * give them one more trip around the active list. So
 			 * that executable code get better chances to stay in
-			 * memory under moderate memory pressure.  Anon pages
-			 * are not likely to be evicted by use-once streaming
-			 * IO, plus JVM can create lots of anon VM_EXEC pages,
-			 * so we ignore them here.

Hi

Quoted 4 lines

Greetings,

Jeff Dike found that many KVM pages are being refaulted in 2.6.29:

Quoted 4 lines

"Lots of pages between discarded due to memory pressure only to be faulted back in soon after. These pages are nearly all stack pages. This is not consistent - sometimes there are relatively few such pages and they are spread out between processes."

I suprise this result really.

- Why this issue happened only on kvm?

- Why shrink_inactive_list() can't find pte young bit?

Is this really unused stack?

Quoted 28 lines

The refaults can be drastically reduced by the following patch, which respects the referenced bit of all anonymous pages (including the KVM pages).

However it risks reintroducing the problem addressed by commit 7e9cd4842 (fix reclaim scalability problem by ignoring the referenced bit, mainly the pte young bit). I wonder if there are better solutions?

Thanks,
Fengguang

---
 mm/vmscan.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

mm/vmscan.c +2 -4

@@ -1288,12 +1288,12 @@ static void shrink_active_list(unsigned 
 			 * Identify referenced, file-backed active pages and
 			 * give them one more trip around the active list. So
 			 * that executable code get better chances to stay in
-			 * memory under moderate memory pressure.  Anon pages
-			 * are not likely to be evicted by use-once streaming
-			 * IO, plus JVM can create lots of anon VM_EXEC pages,
-			 * so we ignore them here.
+			 * memory under moderate memory pressure.
+			 *

mm/vmscan.c static void shrink_active_list(unsigned · line 1299

Quoted 8 lines

mm/vmscan.c +3 -1

+			 * Also protect anon pages: swapping could be costly,
+			 * and KVM guest's referenced bit is helpful.
 			 */
-			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
+			if ((vm_flags & VM_EXEC) || PageAnon(page)) {
 				list_add(&page->lru, &l_active);
 				continue;
 			}

Greetings,

Jeff Dike found that many KVM pages are being refaulted in 2.6.29:

"Lots of pages between discarded due to memory pressure only to be faulted back in soon after. These pages are nearly all stack pages. This is not consistent - sometimes there are relatively few such pages and they are spread out between processes."

The refaults can be drastically reduced by the following patch, which respects the referenced bit of all anonymous pages (including the KVM pages).

However it risks reintroducing the problem addressed by commit 7e9cd4842 (fix reclaim scalability problem by ignoring the referenced bit, mainly the pte young bit). I wonder if there are better solutions?

Quoted 9 lines · Wu Fengguang

Greetings,

Jeff Dike found that many KVM pages are being refaulted in 2.6.29:

"Lots of pages between discarded due to memory pressure only to be faulted back in soon after. These pages are nearly all stack pages. This is not consistent - sometimes there are relatively few such pages and they are spread out between processes."

Quoted 7 lines

The refaults can be drastically reduced by the following patch, which respects the referenced bit of all anonymous pages (including the KVM pages).

However it risks reintroducing the problem addressed by commit 7e9cd4842 (fix reclaim scalability problem by ignoring the referenced bit, mainly the pte young bit). I wonder if there are better solutions?

How do you distinguish between kvm pages and non-kvm anonymous pages? More importantly, why should you?

Jeff, do you see the refaults on Nehalem systems? If so, that's likely due to the lack of an accessed bit on EPT pagetables. It would be interesting to compare with Barcelona (which does).

If that's indeed the case, we can have the EPT ageing mechanism give pages a bit more time around by using an available bit in the EPT PTEs to return accessed on the first pass and not-accessed on the second.

The attached patch implements this.

Quoted 1 line · Avi Kivity

The attached patch implements this.

The attached page requires each page to go around twice before it is evicted, but they will still get evicted in the order in which they were made present.

FIFO page replacement was shown to be a bad idea in the 1960's and it is still a terrible idea today.

Quoted 3 lines

However it risks reintroducing the problem addressed by commit 7e9cd4842 (fix reclaim scalability problem by ignoring the referenced bit, mainly the pte young bit). I wonder if there are better solutions?

Jeff, do you see the refaults on Nehalem systems? If so, that's likely due to the lack of an accessed bit on EPT pagetables. It would be interesting to compare with Barcelona (which does).

If that's indeed the case, we can have the EPT ageing mechanism give pages a bit more time around by using an available bit in the EPT PTEs to return accessed on the first pass and not-accessed on the second.

Quoted 1 line

If that's indeed the case, we can have the EPT ageing mechanism give pages a bit more time around by using an available bit in the EPT PTEs to return accessed on the first pass and not-accessed on the second.

Can we find out which pages are EPT pages?

If so, we could unmap them when they get moved from the active to the inactive list, and soft fault them back in on access, emulating the referenced bit for EPT pages and making page replacement on them work like it should.

Your approximation of pretending the page is accessed the first time and pretending it's not the second time sounds like it will just lead to less efficient FIFO replacement, not to anything even vaguely approximating LRU.

Quoted 4 lines

If so, we could unmap them when they get moved from the active to the inactive list, and soft fault them back in on access, emulating the referenced bit for EPT pages and making page replacement on them work like it should.

It should be easy to implement via the mmu notifier callback: when the mm calls clear_flush_young(), mark it as young, and unmap it from the EPT pagetable.

Quoted 4 lines

Your approximation of pretending the page is accessed the first time and pretending it's not the second time sounds like it will just lead to less efficient FIFO replacement, not to anything even vaguely approximating LRU.

Right, it's just a hack that gives EPT pages higher priority, like the original patch suggested. Note that LRU for VMs is not a good algorithm, since the VM will also reference the least recently used page, leading to thrashing.

Quoted 6 lines · Avi Kivity

Quoted 4 lines

If so, we could unmap them when they get moved from the active to the inactive list, and soft fault them back in on access, emulating the referenced bit for EPT pages and making page replacement on them work like it should.

It should be easy to implement via the mmu notifier callback: when the mm calls clear_flush_young(), mark it as young, and unmap it from the EPT pagetable.

You mean "mark it as old"?

Quoted 11 lines · Rik van Riel

Quoted 8 lines · Avi Kivity

Quoted 4 lines

If so, we could unmap them when they get moved from the active to the inactive list, and soft fault them back in on access, emulating the referenced bit for EPT pages and making page replacement on them work like it should.

It should be easy to implement via the mmu notifier callback: when the mm calls clear_flush_young(), mark it as young, and unmap it from the EPT pagetable.

Quoted 1 line

You mean "mark it as old"?

I meant 'return young, and drop it from the EPT pagetable'.

If we use the present bit as a replacement for the accessed bit, present means young, and clear_flush_young means "if present, return young and unmap, otherwise return old'.

Not having a hardware accessed bit would explain why the VM is not reactivating the pages that were accessed while on the inactive list.

Can we find out which pages are EPT pages?

If so, we could unmap them when they get moved from the active to the inactive list, and soft fault them back in on access, emulating the referenced bit for EPT pages and making page replacement on them work like it should.

Your approximation of pretending the page is accessed the first time and pretending it's not the second time sounds like it will just lead to less efficient FIFO replacement, not to anything even vaguely approximating LRU.

Quoted 3 lines · Rik van Riel

Not having a hardware accessed bit would explain why the VM is not reactivating the pages that were accessed while on the inactive list.

Problem is, even with young bit functional the VM isn't reactivating those pages anyway because of that broken check...

I think it'll still better than current situation, as young bit is always set for ptes. Otherwise EPT pages are too penalized, we need them to stay one round more in active list like everything else.

Jeff, do you see the refaults on Nehalem systems?

How do you distinguish between kvm pages and non-kvm anonymous pages? More importantly, why should you?

Jeff, do you see the refaults on Nehalem systems? If so, that's likely due to the lack of an accessed bit on EPT pagetables. It would be interesting to compare with Barcelona (which does).

It can't distinguish. Besides the pages being refaulted (as minor faults) implies they weren't collected yet. So the fact they are allowed to stay on active list or not can't matter or alter the refaulting issue.

Sounds like there's some terminology confusion. A refault is a page being discarded due to memory pressure and subsequently being faulted back in. I was counting the number of faults between the discard and faulting back in for each affected page. For a large number of predominately stack pages, that number was very small.

mm/vmscan.c static void shrink_active_list(unsigned · line 1299

Quoted 7 lines · Wu Fengguang

mm/vmscan.c +1 -1

 			 */
-			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
+			if ((vm_flags & VM_EXEC) || PageAnon(page)) {
 				list_add(&page->lru, &l_active);
 				continue;
 			}

Please nuke the whole check and do an unconditional list_add; continue; there.

mm/vmscan.c static void shrink_active_list(unsigned · line 1299

Quoted 7 lines · Wu Fengguang

mm/vmscan.c +1 -1

 			 */
-			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
+			if ((vm_flags & VM_EXEC) || PageAnon(page)) {
 				list_add(&page->lru, &l_active);
 				continue;
 			}

Please nuke the whole check and do an unconditional list_add; continue; there.

Quoted 11 lines · Andrea Arcangeli

mm/vmscan.c static void shrink_active_list(unsigned · line 1299

Quoted 7 lines · Wu Fengguang

mm/vmscan.c +1 -1

 			 */
-			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
+			if ((vm_flags & VM_EXEC) || PageAnon(page)) {
 				list_add(&page->lru, &l_active);
 				continue;
 			}

Please nuke the whole check and do an unconditional list_add; continue; there.

<riel> aa: so you're saying we should _never_ add pages to the active list at this point in the code
<aa> right
<riel> aa: and remove the list_add and continue completely <aa> yes
<riel> aa: your email says the opposite :)

mm/vmscan.c static void shrink_active_list(unsigned · line 1299

Quoted 7 lines · Wu Fengguang

mm/vmscan.c +1 -1

 			 */
-			if ((vm_flags & VM_EXEC) && !PageAnon(page)) {
+			if ((vm_flags & VM_EXEC) || PageAnon(page)) {
 				list_add(&page->lru, &l_active);
 				continue;
 			}

Please nuke the whole check and do an unconditional list_add; continue; there.

After some conversation it seems reactivating on large systems generates troubles to the VM as young bit have excessive time to be reactivated, giving troubles to shrink active list. I see that, so then the check should be still nuked, but the unconditional deactivation should happen instead. Otherwise it's trivial to put the

VM to its knees and DoS it with a simple mmap of a file with MAP_EXEC as parameter of mmap. My whole point is that deciding if activating or deactivating pages can't be in function of VM_EXEC, and clearly it helps on desktops but then it probably is a signal that the VM isn't good enough by itself to identify the important working set using young bits and stuff on desktop systems, and if there's a good reason to not activate, we shouldn't activate the VM_EXEC either as anything and anybody can generate a file mapping with VM_EXEC set...

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits, as young bits don't provide any meaningful information then and they just hang the VM in preventing it to shrink active list and looping over it endlessy with million pages inside that list. But on small

systems if inactive list is short it may be too quick to just clear the young bit and only giving it time to be re-enabled in inactive list. That may be the source of the problem. Actually I'm speculating here, because I barely understood that this is swapin... not sure exactly what this regression is about but testing the patch posted is good idea and it will tell us if we just need to dynamically differentiating the algorithm between large and small systems and start ignoring young bits only at some point.

Reasonable; if you depend on a hint from userspace, that hint can be used against you.

How about, for every N pages that you scan, evict at least 1 page, regardless of young bit status? That limits overscanning to a N:1 ratio. With N=250 we'll spend at most 25 usec in order to locate one page to evict.

Quoted 2 lines · Avi Kivity

Reasonable; if you depend on a hint from userspace, that hint can be used against you.

Correct, that is my whole point. Also we never know if applications are mmapping huge files with MAP_EXEC just because they might need to trampoline once in a while, or do some little JIT thing once in a while. Sometime people open files with O_RDWR even if they only need O_RDONLY. It's not a bug, but radically altering VM behavior because of a bitflag doesn't sound good to me.

I certainly see this tends to help as it will reactivate all .text. But this signals current VM behavior is not ok for small systems IMHO if such an hack is required. I prefer a dynamic algorithm that when active list grow too much stop reactivating pages and reduces the time for young bit activation only to the time the page sits on the inactive list. And if active list is small (like 128M system) we fully trust young bit and if it set, we don't allow it to go in inactive list as it's quick enough to scan the whole active list, and young bit is meaningful there.

The issue I can see is with huge system and million pages in active list, by the time we can it all, too much time has passed and we don't get any meaningful information out of young bit. Things are radically different on all regular workstations, and frankly regular workstations are very important too, as I suspect there are more users running on <64G systems than on >64G systems.

Quoted 4 lines

How about, for every N pages that you scan, evict at least 1 page, regardless of young bit status? That limits overscanning to a N:1 ratio. With N=250 we'll spend at most 25 usec in order to locate one page to evict.

Yes exactly, something like that I think will be dynamic, and then we can drop VM_EXEC check and solve the issues on large systems while still not almost totally ignoring young bit on small systems.

This is a quick hack to materialize the idea. It remembers roughly the last 32*SWAP_CLUSTER_MAX=1024 active (mapped) pages scanned, and if _all of them_ are referenced, then the referenced bit is probably meaningless and should not be taken seriously.

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

On 08/06/2009 01:59 PM, Wu Fengguang wrote:

Quoted 4 lines

How about, for every N pages that you scan, evict at least 1 page, regardless of young bit status? That limits overscanning to a N:1 ratio. With N=250 we'll spend at most 25 usec in order to locate one page to evict.

Quoted 4 lines

This is a quick hack to materialize the idea. It remembers roughly the last 32*SWAP_CLUSTER_MAX=1024 active (mapped) pages scanned, and if _all of them_ are referenced, then the referenced bit is probably meaningless and should not be taken seriously.

I don't think we should ignore the referenced bit. There could still be a large batch of unreferenced pages later on that we should preferentially swap. If we swap at least 1 page for every 250 scanned, after 4K swaps we will have traversed 1M pages, enough to find them.

Quoted 2 lines

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

Definitely this should be made part of the zone structure, consider the original report where the problem occurs in a 128MB zone (where we can expect many pages to have their referenced bit set).

I guess both schemes have unacceptable flaws.

For JVM/BIGMEM workload, most pages would be found referenced _all the time_. So the KEEP_MOST scheme could increase reclaim overheads by N=250 times; while the DROP_CONTINUOUS scheme is effectively zero cost.

However, the DROP_CONTINUOUS scheme does bring more _indeterminacy_. It can behave vastly different on single active task and multi ones. It is short sighted and can be cheated by bursty activities.

Quoted 5 lines · Wu Fengguang

I guess both schemes have unacceptable flaws.

For JVM/BIGMEM workload, most pages would be found referenced _all the time_. So the KEEP_MOST scheme could increase reclaim overheads by N=250 times; while the DROP_CONTINUOUS scheme is effectively zero cost.

The higher overhead may not be an issue on smaller systems, or inside smaller cgroups inside large systems, when doing cgroup reclaim.

Quoted 3 lines

However, the DROP_CONTINUOUS scheme does bring more _indeterminacy_. It can behave vastly different on single active task and multi ones. It is short sighted and can be cheated by bursty activities.

The split LRU VM tries to avoid the bursty page aging as much as possible, by doing background deactivating of anonymous pages whenever we reclaim page cache pages and the number of anonymous pages in the zone (or cgroup) is low.

Right, but I meant busty page allocations and accesses on them, which can make a large continuous segment of referenced pages in LRU list, say 50MB. They may or may not be valuable as a whole, however a local algorithm may keep the first 4MB and drop the remaining 46MB.

Quoted 4 lines · Wu Fengguang

Right, but I meant busty page allocations and accesses on them, which can make a large continuous segment of referenced pages in LRU list, say 50MB. They may or may not be valuable as a whole, however a local algorithm may keep the first 4MB and drop the remaining 46MB.

I wonder if the problem is that we simply do not keep a large enough inactive list in Jeff's test. If we do not, pages do not have a chance to be referenced again before the reclaim code comes in.

The cgroup stats should show how many active anon and inactive anon pages there are in the cgroup.

Quoted 4 lines · Wu Fengguang

Right, but I meant busty page allocations and accesses on them, which can make a large continuous segment of referenced pages in LRU list, say 50MB. They may or may not be valuable as a whole, however a local algorithm may keep the first 4MB and drop the remaining 46MB.

I wonder if the problem is that we simply do not keep a large enough inactive list in Jeff's test. If we do not, pages do not have a chance to be referenced again before the reclaim code comes in.

The cgroup stats should show how many active anon and inactive anon pages there are in the cgroup.

Quoted 11 lines · Rik van Riel

Quoted 4 lines · Wu Fengguang

Right, but I meant busty page allocations and accesses on them, which can make a large continuous segment of referenced pages in LRU list, say 50MB. They may or may not be valuable as a whole, however a local algorithm may keep the first 4MB and drop the remaining 46MB.

I wonder if the problem is that we simply do not keep a large enough inactive list in Jeff's test. If we do not, pages do not have a chance to be referenced again before the reclaim code comes in.

Exactly, that's the case I call the list FIFO.

Quoted 2 lines

The cgroup stats should show how many active anon and inactive anon pages there are in the cgroup.

Jeff, can you have a look at these stats? Thanks!

Jeff, can you have a look at these stats? Thanks!

Quoted 1 line

Jeff, can you have a look at these stats? Thanks!

Yeah, I just did after adding some tracing which dumped out the same data. It looks pretty much the same. Inactive anon and active anon are pretty similar. Inactive file and active file are smaller and fluctuate more, but doesn't look horribly unbalanced.

Below are the stats from memory.stat - inactive_anon, active_anon, inactive_file, active_file, plus some commentary on what's happening.

What do the stats for / from within kvm look like?

Quoted 1 line

What do the stats for / from within kvm look like?

Interesting - what they look like is inactive_anon is always zero.

This will be because the VM does not start aging pages from the active to the inactive list unless there is some memory pressure.

Quoted 3 lines

This will be because the VM does not start aging pages from the active to the inactive list unless there is some memory pressure.

Which is the reason I gave the VM a puny amount of memory. We know the thing is under memory pressure because I've been complaining about page discards.

Page discards by the host, which are invisible to the guest OS.

Quoted 2 lines

Page discards by the host, which are invisible to the guest OS.

Duh.  Right - I can't keep my VM systems straight...

Quoted 2 lines · Avi Kivity

On 08/06/2009 01:59 PM, Wu Fengguang wrote:

scheme KEEP_MOST:

Quoted 5 lines

Quoted 5 lines

How about, for every N pages that you scan, evict at least 1 page, regardless of young bit status? That limits overscanning to a N:1 ratio. With N=250 we'll spend at most 25 usec in order to locate one page to evict.

scheme DROP_CONTINUOUS:

Quoted 5 lines

Quoted 5 lines

This is a quick hack to materialize the idea. It remembers roughly the last 32*SWAP_CLUSTER_MAX=1024 active (mapped) pages scanned, and if _all of them_ are referenced, then the referenced bit is probably meaningless and should not be taken seriously.

Quoted 5 lines

I don't think we should ignore the referenced bit. There could still be a large batch of unreferenced pages later on that we should preferentially swap. If we swap at least 1 page for every 250 scanned, after 4K swaps we will have traversed 1M pages, enough to find them.

I guess both schemes have unacceptable flaws.

For JVM/BIGMEM workload, most pages would be found referenced _all the time_. So the KEEP_MOST scheme could increase reclaim overheads by N=250 times; while the DROP_CONTINUOUS scheme is effectively zero cost.

Side question -
Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

		if (unlikely(!page_evictable(page, NULL))) {
			putback_lru_page(page);
			continue;
		}

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

But if there are important mlock-heavy workloads, this could make the scan come up empty, or at least emptier than we might like.

Quoted 9 lines · Jeff Dike

Side question -
Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

		if (unlikely(!page_evictable(page, NULL))) {
			putback_lru_page(page);
			continue;
		}

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Quoted 2 lines

But if there are important mlock-heavy workloads, this could make the scan come up empty, or at least emptier than we might like.

Yes, if the above 'if' block is removed, the inactive lists might get more expensive to reclaim.

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

		if (unlikely(!page_evictable(page, NULL))) {
			putback_lru_page(page);
			continue;
		}

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 5 lines

Quoted 2 lines

But if there are important mlock-heavy workloads, this could make the scan come up empty, or at least emptier than we might like.

Yes, if the above 'if' block is removed, the inactive lists might get more expensive to reclaim.

Why?

Quoted 19 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

		if (unlikely(!page_evictable(page, NULL))) {
			putback_lru_page(page);
			continue;
		}

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Quoted 4 lines

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
 Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

         if (unlikely(!page_evictable(page, NULL))) { putback_lru_page(page); continue;
         }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

How about this fix?

---
mm: stop circulating of referenced mlocked pages

Signed-off-by Wu Fengguang <[email protected]>

---

mm/rmap.c     2009-08-16 19:22:46.000000000 +0800 renamed from mm/rmap.c        2009-08-16 19:11:13.000000000 +0800 +3 -1

@@ -358,6 +358,7 @@ static int page_referenced_one(struct pa
         */
        if (vma->vm_flags & VM_LOCKED) {
                *mapcount = 1;  /* break early from loop */
+               *vm_flags |= VM_LOCKED;
                goto out_unmap;
        }
 

@@ -482,6 +483,8 @@ static int page_referenced_file(struct p
        }
 
        spin_unlock(&mapping->i_mmap_lock);
+       if (*vm_flags & VM_LOCKED)
+               referenced = 0;
        return referenced;
  }
 
 
--

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

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

I think it's not a big deal.

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

When next time to scan comes, we can try to move the pages into unevictable list, again.

As I know about mlock pages, we already had some races condition. They will be rescued like above.

Quoted 29 lines

How about this fix?

---
mm: stop circulating of referenced mlocked pages

Signed-off-by Wu Fengguang <[email protected]>

---

mm/rmap.c A A 2009-08-16 19:22:46.000000000 +0800 renamed from mm/rmap.c A A A A 2009-08-16 19:11:13.000000000 +0800 +0 -0

@@ -358,6 +358,7 @@ static int page_referenced_one(struct pa

A  A  A  A  */
A  A  A  A if (vma->vm_flags & VM_LOCKED) {
A  A  A  A  A  A  A  A *mapcount = 1; A /* break early from loop */
+ A  A  A  A  A  A  A  *vm_flags |= VM_LOCKED;
A  A  A  A  A  A  A  A goto out_unmap;
A  A  A  A }

mm/rmap.c A A 2009-08-16 19:22:46.000000000 +0800 renamed from mm/rmap.c A A A A 2009-08-16 19:11:13.000000000 +0800 +2 -0

@@ -482,6 +483,8 @@ static int page_referenced_file(struct p
 A  A  A  A }
 
 A  A  A  A spin_unlock(&mapping->i_mmap_lock);
+ A  A  A  if (*vm_flags & VM_LOCKED)
+ A  A  A  A  A  A  A  referenced = 0;
 A  A  A  A return referenced;
 A }
 

Quoted 6 lines

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

Signature · 2 lines

Kind regards,
Minchan Kim

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

Quoted 50 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Quoted 3 lines

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

----

/*
* called from munlock()/munmap() path with page supposedly on the LRU. *
* Note: unlike mlock_vma_page(), we can't just clear the PageMlocked
* [in try_to_munlock()] and then attempt to isolate the page. We must
* isolate the page to keep others from messing with its unevictable
* and mlocked state while trying to munlock. However, we pre-clear the
* mlocked state anyway as we might lose the isolation race and we might
* not get another chance to clear PageMlocked. If we successfully
* isolate the page and try_to_munlock() detects other VM_LOCKED vmas
* mapping the page, it will restore the PageMlocked state, unless the page
* is mapped in a non-linear vma. So, we go ahead and SetPageMlocked(),
* perhaps redundantly.

* If we lose the isolation race, and the page is mapped by other VM_LOCKED
* vmas, we'll detect this in vmscan--via try_to_munlock() or try_to_unmap()
* either of which will restore the PageMlocked state by calling
* mlock_vma_page() above, if it can grab the vma's mmap sem.
*/
static void munlock_vma_page(struct page *page)
{
...

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 57 lines · Minchan Kim

Quoted 53 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

Quoted 3 lines

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

Then I was worrying about a null problem. Sorry for the confusion!

Thanks,
Fengguang

Quoted 27 lines

----

/*
* called from munlock()/munmap() path with page supposedly on the LRU. *
* Note: unlike mlock_vma_page(), we can't just clear the PageMlocked
* [in try_to_munlock()] and then attempt to isolate the page. We must
* isolate the page to keep others from messing with its unevictable
* and mlocked state while trying to munlock. However, we pre-clear the
* mlocked state anyway as we might lose the isolation race and we might
* not get another chance to clear PageMlocked. If we successfully
* isolate the page and try_to_munlock() detects other VM_LOCKED vmas
* mapping the page, it will restore the PageMlocked state, unless the page
* is mapped in a non-linear vma. So, we go ahead and SetPageMlocked(),
* perhaps redundantly.
* If we lose the isolation race, and the page is mapped by other VM_LOCKED
* vmas, we'll detect this in vmscan--via try_to_munlock() or try_to_unmap()
* either of which will restore the PageMlocked state by calling
* mlock_vma_page() above, if it can grab the vma's mmap sem.
*/
static void munlock_vma_page(struct page *page)
{
...

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 62 lines · On Tue, 18 Aug 2009 17:31:19 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 60 lines · Minchan Kim

Quoted 45 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Quoted 8 lines

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Quoted 2 lines

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

No. I am sorry for making my point not clear.
I meant following as.
When the next time to scan,

shrink_page_list

-> try_to_unmap

	-> try_to_unmap_xxx
		-> if (vma->vm_flags & VM_LOCKED)
		-> try_to_mlock_page
			-> TestSetPageMlocked
			-> putback_lru_page

So at last, the page will be located in unevictable list.

Quoted 29 lines

Then I was worrying about a null problem. Sorry for the confusion!

Thanks,
Fengguang

Quoted 24 lines

----

/*
* called from munlock()/munmap() path with page supposedly on the LRU. *
* Note: unlike mlock_vma_page(), we can't just clear the PageMlocked
* [in try_to_munlock()] and then attempt to isolate the page. We must
* isolate the page to keep others from messing with its unevictable
* and mlocked state while trying to munlock. However, we pre-clear the
* mlocked state anyway as we might lose the isolation race and we might
* not get another chance to clear PageMlocked. If we successfully
* isolate the page and try_to_munlock() detects other VM_LOCKED vmas
* mapping the page, it will restore the PageMlocked state, unless the page
* is mapped in a non-linear vma. So, we go ahead and SetPageMlocked(),
* perhaps redundantly.
* If we lose the isolation race, and the page is mapped by other VM_LOCKED
* vmas, we'll detect this in vmscan--via try_to_munlock() or try_to_unmap()
* either of which will restore the PageMlocked state by calling
* mlock_vma_page() above, if it can grab the vma's mmap sem.
*/
static void munlock_vma_page(struct page *page)
{
...

Quoted 3 lines

Quoted 3 lines

Signature · 2 lines

Kind regards,
Minchan Kim

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 65 lines · Minchan Kim

Quoted 62 lines · On Tue, 18 Aug 2009 17:31:19 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 60 lines · Minchan Kim

Quoted 45 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
 Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

         if (unlikely(!page_evictable(page, NULL))) { putback_lru_page(page); continue;
         }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Quoted 8 lines

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Quoted 8 lines

Quoted 2 lines

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

No. I am sorry for making my point not clear.
I meant following as.
When the next time to scan,

shrink_page_list

 ->
               referenced = page_referenced(page, 1, sc->mem_cgroup, &vm_flags); /* In active use or really unfreeable?  Activate it. */ if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced && page_mapping_inuse(page)) goto activate_locked;

Quoted 1 line

-> try_to_unmap

    ~~~~~~~~~~~~ this line won't be reached if page is found to be referenced in the above lines?

Thanks,
Fengguang

Quoted 45 lines

      -> try_to_unmap_xxx
              -> if (vma->vm_flags & VM_LOCKED)
              -> try_to_mlock_page
                      -> TestSetPageMlocked
                      -> putback_lru_page

So at last, the page will be located in unevictable list.

Quoted 32 lines

Then I was worrying about a null problem. Sorry for the confusion!

Thanks,
Fengguang

Quoted 27 lines

----

/*
 * called from munlock()/munmap() path with page supposedly on the LRU. *
 * Note:  unlike mlock_vma_page(), we can't just clear the PageMlocked
 * [in try_to_munlock()] and then attempt to isolate the page.  We must
 * isolate the page to keep others from messing with its unevictable
 * and mlocked state while trying to munlock.  However, we pre-clear the
 * mlocked state anyway as we might lose the isolation race and we might
 * not get another chance to clear PageMlocked.  If we successfully
 * isolate the page and try_to_munlock() detects other VM_LOCKED vmas
 * mapping the page, it will restore the PageMlocked state, unless the page
 * is mapped in a non-linear vma.  So, we go ahead and SetPageMlocked(),
 * perhaps redundantly.
 * If we lose the isolation race, and the page is mapped by other VM_LOCKED
 * vmas, we'll detect this in vmscan--via try_to_munlock() or try_to_unmap()
 * either of which will restore the PageMlocked state by calling
 * mlock_vma_page() above, if it can grab the vma's mmap sem.
 */
static void munlock_vma_page(struct page *page)
{
...

--
Kind regards,
Minchan Kim

--
Kind regards,
Minchan Kim

Quoted 82 lines · Wu Fengguang

Quoted 65 lines · Minchan Kim

Quoted 62 lines · On Tue, 18 Aug 2009 17:31:19 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 60 lines · Minchan Kim

Quoted 45 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list() as opposed to __isolate_lru_page?

A  A  A  A  A if (unlikely(!page_evictable(page, NULL))) {
A  A  A  A  A  A  A  A  A putback_lru_page(page);
A  A  A  A  A  A  A  A  A continue;
A  A  A  A  A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Quoted 8 lines

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Quoted 8 lines

Quoted 2 lines

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

No. I am sorry for making my point not clear.
I meant following as.
When the next time to scan,

shrink_page_list

A ->

A  A  A  A  A  A  A  A referenced = page_referenced(page, 1,
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A sc->mem_cgroup, &vm_flags);
A  A  A  A  A  A  A  A /* In active use or really unfreeable? A Activate it. */
A  A  A  A  A  A  A  A if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A referenced && page_mapping_inuse(page))
A  A  A  A  A  A  A  A  A  A  A  A goto activate_locked;

Quoted 3 lines

Quoted 1 line

-> try_to_unmap

A  A  ~~~~~~~~~~~~ this line won't be reached if page is found to be
A  A  referenced in the above lines?

Indeed! In fact, I was worry about that.
It looks after live lock problem.
But I think it's very small race window so there isn't any report until now. Let's Cced Lee.

If we have to fix it, how about this ?
This version has small overhead than yours since there is less shrink_page_list call than page_referenced.

mm/rmap.c +1 -0

index ed63894..283266c 100644
@@ -358,6 +358,7 @@ static int page_referenced_one(struct page *page,
         */
        if (vma->vm_flags & VM_LOCKED) {
                *mapcount = 1;  /* break early from loop */
+               *vm_flags |= VM_LOCKED;
                goto out_unmap;
        }
 

mm/vmscan.c +2 -1

index d224b28..d156e1d 100644
@@ -632,7 +632,8 @@ static unsigned long shrink_page_list(struct
 list_head *page_list,
                                                sc->mem_cgroup, &vm_flags);
                /* In active use or really unfreeable?  Activate it. */
                if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
-                                       referenced && page_mapping_inuse(page))
+                                       referenced && page_mapping_inuse(page)
+                                       && !(vm_flags & VM_LOCKED))
                        goto activate_locked;
 

Quoted 48 lines

Thanks,
Fengguang

Quoted 42 lines

A  A  A  -> try_to_unmap_xxx
A  A  A  A  A  A  A  -> if (vma->vm_flags & VM_LOCKED)
A  A  A  A  A  A  A  -> try_to_mlock_page
A  A  A  A  A  A  A  A  A  A  A  -> TestSetPageMlocked
A  A  A  A  A  A  A  A  A  A  A  -> putback_lru_page

So at last, the page will be located in unevictable list.

Quoted 32 lines

Then I was worrying about a null problem. Sorry for the confusion!

Thanks,
Fengguang

Quoted 27 lines

----

/*
A * called from munlock()/munmap() path with page supposedly on the LRU. A *
A * Note: A unlike mlock_vma_page(), we can't just clear the PageMlocked A * [in try_to_munlock()] and then attempt to isolate the page. A We must A * isolate the page to keep others from messing with its unevictable A * and mlocked state while trying to munlock. A However, we pre-clear the A * mlocked state anyway as we might lose the isolation race and we might A * not get another chance to clear PageMlocked. A If we successfully A * isolate the page and try_to_munlock() detects other VM_LOCKED vmas A * mapping the page, it will restore the PageMlocked state, unless the page A * is mapped in a non-linear vma. A So, we go ahead and SetPageMlocked(), A * perhaps redundantly.
A * If we lose the isolation race, and the page is mapped by other VM_LOCKED A * vmas, we'll detect this in vmscan--via try_to_munlock() or try_to_unmap() A * either of which will restore the PageMlocked state by calling A * mlock_vma_page() above, if it can grab the vma's mmap sem.
A */
static void munlock_vma_page(struct page *page)
{
...

--
Kind regards,
Minchan Kim

Quoted 4 lines

Quoted 3 lines

--
Kind regards,
Minchan Kim

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 92 lines · Minchan Kim

Quoted 82 lines · Wu Fengguang

Quoted 65 lines · Minchan Kim

Quoted 62 lines · On Tue, 18 Aug 2009 17:31:19 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 60 lines · Minchan Kim

Quoted 45 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
 Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

         if (unlikely(!page_evictable(page, NULL))) {
                 putback_lru_page(page);
                 continue;
         }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or
avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Quoted 8 lines

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Quoted 8 lines

Quoted 2 lines

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

No. I am sorry for making my point not clear.
I meant following as.
When the next time to scan,

shrink_page_list

 ->
               referenced = page_referenced(page, 1, sc->mem_cgroup, &vm_flags); /* In active use or really unfreeable?  Activate it. */ if (sc->order <= PAGE_ALLOC_COSTLY_ORDER && referenced && page_mapping_inuse(page)) goto activate_locked;

Quoted 3 lines

Quoted 1 line

-> try_to_unmap

    ~~~~~~~~~~~~ this line won't be reached if page is found to be referenced in the above lines?

Indeed! In fact, I was worry about that.
It looks after live lock problem.
But I think  it's very small race window so  there isn't any report until now. Let's Cced Lee.

Quoted 3 lines

If we have to fix it, how about this ?
This version  has small overhead than yours since there is less shrink_page_list call than page_referenced.

Yeah, it looks better. However I still wonder if (VM_LOCKED && !PG_mlocked) is possible and somehow persistent. Does anyone have the answer? Thanks!

Thanks,
Fengguang

Quoted 92 lines · Wu Fengguang

Quoted 82 lines · Minchan Kim

Quoted 65 lines

Quoted 62 lines · On Tue, 18 Aug 2009 17:31:19 +0800 Wu Fengguang <[email protected]> wrote:

Quoted 60 lines · Minchan Kim

Quoted 45 lines · On Tue, 18 Aug 2009 10:34:38 +0800 Wu Fengguang <[email protected]> wrote:

Minchan,

Quoted 35 lines · Minchan Kim

Quoted 26 lines · Wu Fengguang

Quoted 25 lines · Wu Fengguang

Quoted 23 lines · Rik van Riel

Quoted 10 lines · Wu Fengguang

Quoted 9 lines · Jeff Dike

Side question -
A Is there a good reason for this to be in shrink_active_list()
as opposed to __isolate_lru_page?

A A A A A if (unlikely(!page_evictable(page, NULL))) {
A A A A A A A A A putback_lru_page(page);
A A A A A A A A A continue;
A A A A A }

Quoted 7 lines

Quoted 2 lines

Maybe we want to minimize the amount of code under the lru lock or
avoid duplicate logic in the isolate_page functions.

I guess the quick test means to avoid the expensive page_referenced() call that follows it. But that should be mostly one shot cost - the unevictable pages are unlikely to cycle in active/inactive list again and again.

Please read what putback_lru_page does.

It moves the page onto the unevictable list, so that it will not end up in this scan again.

Quoted 7 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

I think it's not a big deal.

Maybe, otherwise I should bring up this issue long time before :)

Quoted 2 lines

As you mentioned, it's rare case so there would be few pages in active list instead of unevictable list.

Quoted 8 lines

Yes.

Quoted 2 lines

When next time to scan comes, we can try to move the pages into unevictable list, again.

Will PG_mlocked be set by then? Otherwise the situation is not likely to change and the VM_LOCKED pages may circulate in active/inactive list for countless times.

PG_mlocked is not important in that case.
Important thing is VM_LOCKED vma.
I think below annotaion can help you to understand my point. :)

Quoted 8 lines

Quoted 2 lines

Hmm, it looks like pages under VM_LOCKED vma is guaranteed to have PG_mlocked set, and so will be caught by page_evictable(). Is it?

No. I am sorry for making my point not clear.
I meant following as.
When the next time to scan,

shrink_page_list

A ->

A  A  A  A  A  A  A  A referenced = page_referenced(page, 1,
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A sc->mem_cgroup, &vm_flags);
A  A  A  A  A  A  A  A /* In active use or really unfreeable? A Activate it. */
A  A  A  A  A  A  A  A if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A referenced && page_mapping_inuse(page))
A  A  A  A  A  A  A  A  A  A  A  A goto activate_locked;

Quoted 3 lines

Quoted 1 line

-> try_to_unmap

A  A  ~~~~~~~~~~~~ this line won't be reached if page is found to be
A  A  referenced in the above lines?

Indeed! In fact, I was worry about that.
It looks after live lock problem.
But I think it's very small race window so there isn't any report until now. Let's Cced Lee.

Quoted 3 lines

If we have to fix it, how about this ?
This version has small overhead than yours since there is less shrink_page_list call than page_referenced.

Yeah, it looks better. However I still wonder if (VM_LOCKED && !PG_mlocked) is possible and somehow persistent. Does anyone have the answer? Thanks!

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

How about this fix?

---
mm: stop circulating of referenced mlocked pages

Signed-off-by Wu Fengguang <[email protected]>

---

mm/rmap.c +1 -0

@@ -358,6 +358,7 @@ static int page_referenced_one(struct pa
 	 */
 	if (vma->vm_flags & VM_LOCKED) {
 		*mapcount = 1;	/* break early from loop */
+		*vm_flags |= VM_LOCKED;
 		goto out_unmap;
 	}
 

Quoted 8 lines

Quoted 7 lines

Yes it does. I said 'mostly' because there is a small hole that an unevictable page may be scanned but still not moved to unevictable
list: when a page is mapped in two places, the first pte has the referenced bit set, the _second_ VMA has VM_LOCKED bit set, then page_referenced() will return 1 and shrink_page_list() will move it into active list instead of unevictable list. Shall we fix this rare case?

Quoted 1 line

How about this fix?

Good spotting.
Yes, this is rare case. but I also don't think your patch introduce performance degression.

However, I think your patch have one bug.

Quoted 20 lines

---
mm: stop circulating of referenced mlocked pages

Signed-off-by Wu Fengguang <[email protected]>

---

mm/rmap.c +1 -0

@@ -358,6 +358,7 @@ static int page_referenced_one(struct pa
 	 */
 	if (vma->vm_flags & VM_LOCKED) {
 		*mapcount = 1;	/* break early from loop */
+		*vm_flags |= VM_LOCKED;
 		goto out_unmap;
 	}
 

@@ -482,6 +483,8 @@ static int page_referenced_file(struct p
 	}
 

mm/rmap.c int page_referenced(struct page *page, · line 541

Quoted 6 lines

mm/rmap.c +2 -0

 	spin_unlock(&mapping->i_mmap_lock);
+	if (*vm_flags & VM_LOCKED)
+		referenced = 0;
 	return referenced;
 }
 

page_referenced_file?
I think we should change page_referenced().

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Otherwise vm stastics show strange number.

This patch does that.

Reported-by Wu Fengguang <[email protected]>

Signed-off-by KOSAKI Motohiro <[email protected]>

---
 mm/internal.h |    5 +++--
 mm/rmap.c     |    8 +++++++-
 mm/vmscan.c   |    2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

Index: b/mm/internal.h
===================================================================

mm/internal.h +3 -2

@@ -91,7 +91,8 @@ static inline void unevictable_migrate_p
  * to determine if it's being mapped into a LOCKED vma.
  * If so, mark page as mlocked.
  */
-static inline int is_mlocked_vma(struct vm_area_struct *vma, struct page *page)
+static inline int try_set_page_mlocked(struct vm_area_struct *vma,
+				       struct page *page)
 {
 	VM_BUG_ON(PageLRU(page));
 

@@ -144,7 +145,7 @@ static inline void mlock_migrate_page(st
 }
 
 #else /* CONFIG_HAVE_MLOCKED_PAGE_BIT */
-static inline int is_mlocked_vma(struct vm_area_struct *v, struct page *p)
+static inline int try_set_page_mlocked(struct vm_area_struct *v, struct page *p)
 {
 	return 0;
 }

Index: b/mm/rmap.c
===================================================================

mm/rmap.c +4 -0

 		goto out_unmap;
 	}

@@ -531,6 +533,9 @@ int page_referenced(struct page *page,
 	if (page_test_and_clear_young(page))
 		referenced++;
 
+	if (unlikely(*vm_flags & VM_LOCKED))
+		referenced = 0;
+
 	return referenced;
 }
 

@@ -784,6 +789,7 @@ static int try_to_unmap_one(struct page 
 	 */
 	if (!(flags & TTU_IGNORE_MLOCK)) {
 		if (vma->vm_flags & VM_LOCKED) {
+			try_set_page_mlocked(vma, page);
 			ret = SWAP_MLOCK;
 			goto out_unmap;
 		}

Index: b/mm/vmscan.c
===================================================================

mm/vmscan.c +1 -1

@@ -2666,7 +2666,7 @@ int page_evictable(struct page *page, st
 	if (mapping_unevictable(page_mapping(page)))
 		return 0;
 
-	if (PageMlocked(page) || (vma && is_mlocked_vma(vma, page)))
+	if (PageMlocked(page) || (vma && try_set_page_mlocked(vma, page)))
 		return 0;
 
 	return 1;

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Quoted 11 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +2 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 static int page_referenced_one(struct pa · line 367

Quoted 1 line

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +1 -0

+             try_set_page_mlocked(vma, page);

That call is not absolutely necessary?

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

A A A A A A A A A A A A A A A A A A A A A A A A A A mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

A A A A A A A A A A A A A PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 12 lines

Quoted 11 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c A A A 2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c A A A 2009-08-18 19:48:14.000000000 +0900 +0 -0

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa

A  A  A  A * unevictable list.
A  A  A  A */
A  A  A  if (vma->vm_flags & VM_LOCKED) {
- A  A  A  A  A  A  *mapcount = 1; A /* break early from loop */
+ A  A  A  A  A  A  *mapcount = 1; A  A  A  A  A /* break early from loop */
+ A  A  A  A  A  A  *vm_flags |= VM_LOCKED; /* for prevent to move active list */

Quoted 3 lines

Quoted 1 line

+ A A A A A A try_set_page_mlocked(vma, page);

That call is not absolutely necessary?

Why? I haven't catch your point.

Quoted 51 lines · KOSAKI Motohiro

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +3 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */
 
+             try_set_page_mlocked(vma, page);

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Thanks,
Fengguang

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

Quoted 52 lines · KOSAKI Motohiro

Quoted 51 lines

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +3 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */
 
+             try_set_page_mlocked(vma, page);

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Yes. we don't have to call it and we can make patch simple. I already sent patch on yesterday.

http://marc.info/?l=linux-mm&m=125059325722370&w=2

I think It's more simple than KOSAKI's idea.
Is any problem in my patch ?

2009/8/19 Minchan Kim <[email protected]>:

Quoted 53 lines · Wu Fengguang

Quoted 52 lines · KOSAKI Motohiro

Quoted 51 lines

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +3 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */
 
+             try_set_page_mlocked(vma, page);

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Correct.

Quoted 3 lines

Yes. we don't have to call it and we can make patch simple. I already sent patch on yesterday.

Quoted 4 lines

http://marc.info/?l=linux-mm&m=125059325722370&w=2

I think It's more simple than KOSAKI's idea.
Is any problem in my patch ?

Hmm, I think

1. Anyway, we need turn on PG_mlock.

2. PG_mlock prevent livelock because page_evictable() check is called at very early in shrink_page_list().

Quoted 3 lines

Hmm, I think

1. Anyway, we need turn on PG_mlock.

I add my patch again to explain.

mm/rmap.c +1 -0

index ed63894..283266c 100644
@@ -358,6 +358,7 @@ static int page_referenced_one(struct page *page,
         */
        if (vma->vm_flags & VM_LOCKED) {
                *mapcount = 1;  /* break early from loop */
+               *vm_flags |= VM_LOCKED;
                goto out_unmap;
        }
 

mm/vmscan.c +0 -0

index d224b28..d156e1d 100644
@@ -632,7 +632,8 @@ static unsigned long shrink_page_list(struct
 list_head *page_list,
                                                sc->mem_cgroup, &vm_flags);
                /* In active use or really unfreeable?  Activate it. */

               if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
-                                       referenced && page_mapping_inuse(page))
+                                       referenced && page_mapping_inuse(page)
+                                       && !(vm_flags & VM_LOCKED)) goto activate_locked;

By this check, the page can be reached at try_to_unmap after page_referenced in shrink_page_list. At that time PG_mlocked will be set.

Quoted 52 lines · Wu Fengguang

Quoted 51 lines · KOSAKI Motohiro

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +3 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */
 
+             try_set_page_mlocked(vma, page);

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Yes. we don't have to call it and we can make patch simple. I already sent patch on yesterday.

http://marc.info/?l=linux-mm&m=125059325722370&w=2

I think It's more simple than KOSAKI's idea.
Is any problem in my patch ?

Quoted 66 lines · Minchan Kim

Quoted 52 lines · Wu Fengguang

Quoted 51 lines · KOSAKI Motohiro

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

                                                   mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

                          PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c       2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c       2009-08-18 19:48:14.000000000 +0900 +3 -1

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa
        * unevictable list.
        */
       if (vma->vm_flags & VM_LOCKED) {
-             *mapcount = 1;  /* break early from loop */
+             *mapcount = 1;          /* break early from loop */
+             *vm_flags |= VM_LOCKED; /* for prevent to move active list */
 
+             try_set_page_mlocked(vma, page);

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Yes. we don't have to call it and we can make patch simple. I already sent patch on yesterday.

http://marc.info/?l=linux-mm&m=125059325722370&w=2

Quoted 2 lines

I think It's more simple than KOSAKI's idea.
Is any problem in my patch ?

No, IMHO your patch is simple and good, while KOSAKI's is more complete :)

- the try_set_page_mlocked() rename is suitable
- the call to try_set_page_mlocked() is necessary on try_to_unmap()

- the "if (VM_LOCKED) referenced = 0" in page_referenced() could cover both active/inactive vmscan

I did like your proposed

               if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&
-                                       referenced && page_mapping_inuse(page))
+                                       referenced && page_mapping_inuse(page)
+                                       && !(vm_flags & VM_LOCKED)) goto activate_locked;

which looks more intuitive and less confusing.

Thanks,
Fengguang

Quoted 73 lines · Wu Fengguang

Quoted 61 lines · Minchan Kim

Quoted 52 lines · Wu Fengguang

Quoted 51 lines · KOSAKI Motohiro

Quoted 25 lines

Quoted 2 lines

page_referenced_file?
I think we should change page_referenced().

Yeah, good catch.

Quoted 7 lines

Instead, How about this?
==============================================

Subject: [PATCH] mm: stop circulating of referenced mlocked pages

Currently, mlock() systemcall doesn't gurantee to mark the page PG_Mlocked

A A A A A A A A A A A A A A A A A A A A A A A A A A mark PG_mlocked

Quoted 9 lines

because some race prevent page grabbing.
In that case, instead vmscan move the page to unevictable lru.

However, Recently Wu Fengguang pointed out current vmscan logic isn't so efficient.
mlocked page can move circulatly active and inactive list because vmscan check the page is referenced _before_ cull mlocked page.

Plus, vmscan should mark PG_Mlocked when cull mlocked page.

Quoted 7 lines

A A A A A A A A A A A A A PG_mlocked

Quoted 3 lines

Otherwise vm stastics show strange number.

This patch does that.

Reviewed-by Wu Fengguang <[email protected]>

Thanks.

Quoted 14 lines

Quoted 13 lines

Index: b/mm/rmap.c
===================================================================

mm/rmap.c A A A 2009-08-18 23:47:34.000000000 +0900 renamed from mm/rmap.c A A A 2009-08-18 19:48:14.000000000 +0900 +0 -0

@@ -362,7 +362,9 @@ static int page_referenced_one(struct pa

A  A  A  A * unevictable list.
A  A  A  A */
A  A  A  if (vma->vm_flags & VM_LOCKED) {
- A  A  A  A  A  A  *mapcount = 1; A /* break early from loop */
+ A  A  A  A  A  A  *mapcount = 1; A  A  A  A  A /* break early from loop */
+ A  A  A  A  A  A  *vm_flags |= VM_LOCKED; /* for prevent to move active list */

+ A A A A A A try_set_page_mlocked(vma, page);

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

That call is not absolutely necessary?

Why? I haven't catch your point.

Because we'll eventually hit another try_set_page_mlocked() when trying to unmap the page. Ie. duplicated with another call you added in this patch.

Quoted 7 lines

Yes. we don't have to call it and we can make patch simple. I already sent patch on yesterday.

http://marc.info/?l=linux-mm&m=125059325722370&w=2

I think It's more simple than KOSAKI's idea.
Is any problem in my patch ?

No, IMHO your patch is simple and good, while KOSAKI's is more complete :)

Quoted 2 lines

- the try_set_page_mlocked() rename is suitable
- the call to try_set_page_mlocked() is necessary on try_to_unmap()

We don't need try_set_page_mlocked call in try_to_unmap. That's because try_to_unmap_xxx will call try_to_mlock_page if the page is included in any VM_LOCKED vma. Eventually, It can move unevictable list.

Quoted 2 lines

- the "if (VM_LOCKED) referenced = 0" in page_referenced() could A cover both active/inactive vmscan

ASAP we set PG_mlocked in page, we can save unnecessary vmscan cost from active list to inactive list. But I think it's rare case so that there would be few pages.
So I think that will be not big overhead.

As I know, Rescue by vmscan page losing the isolation race was the Lee's design.
But as you pointed out, it have a bug that vmscan can't rescue the page due to reach try_to_unmap.

So I think this approach is proper. :)

Quoted 8 lines

I did like your proposed

A A A A A A A A if (sc->order <= PAGE_ALLOC_COSTLY_ORDER &&

- A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  referenced && page_mapping_inuse(page))
+ A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  referenced && page_mapping_inuse(page)
+ A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  A  && !(vm_flags & VM_LOCKED))

A A A A A A A A A A A A goto activate_locked;

Quoted 5 lines

which looks more intuitive and less confusing.

Thanks,
Fengguang

Signature · 2 lines

Kind regards,
Minchan Kim

Quoted 2 lines · Wu Fengguang

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

Definitely this should be made part of the zone structure, consider the original report where the problem occurs in a 128MB zone (where we can expect many pages to have their referenced bit set).

Quoted 5 lines · Avi Kivity

Quoted 2 lines · Wu Fengguang

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

Quoted 1 line

Definitely this should be made part of the zone structure, consider the original report where the problem occurs in a 128MB zone (where we can expect many pages to have their referenced bit set).

The problem did not occur in a 128MB zone, but in a 128MB cgroup.

Putting it in the zone means that the cgroup, which may have different behaviour from the rest of the zone, due to excessive memory pressure inside the cgroup, does not get the right statistics.

Quoted 8 lines · Avi Kivity

Quoted 2 lines · Wu Fengguang

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

Definitely this should be made part of the zone structure, consider the original report where the problem occurs in a 128MB zone (where we can expect many pages to have their referenced bit set).

The problem did not occur in a 128MB zone, but in a 128MB cgroup.

Putting it in the zone means that the cgroup, which may have different behaviour from the rest of the zone, due to excessive memory pressure inside the cgroup, does not get the right statistics.

(cc to memcg folks)

Quoted 10 lines

Quoted 8 lines · Avi Kivity

Quoted 2 lines · Wu Fengguang

As a refinement, the static variable 'recent_all_referenced' could be moved to struct zone or made a per-cpu variable.

Definitely this should be made part of the zone structure, consider the original report where the problem occurs in a 128MB zone (where we can expect many pages to have their referenced bit set).

Quoted 6 lines

The problem did not occur in a 128MB zone, but in a 128MB cgroup.

Putting it in the zone means that the cgroup, which may have different behaviour from the rest of the zone, due to excessive memory pressure inside the cgroup, does not get the right statistics.

maybe, I heven't catch your point.

Current memcgroup logic also use recent_scan/recent_rotate statistics. Isn't it enought?

Quoted 2 lines · KOSAKI Motohiro

Current memcgroup logic also use recent_scan/recent_rotate statistics. Isn't it enought?

I don't understand the context, I'll look at the problem when I am back (I am away from work for the next few days).

This is a quick hack to materialize the idea. It remembers roughly the last 32*SWAP_CLUSTER_MAX=1024 active (mapped) pages scanned, and if _all of them_ are referenced, then the referenced bit is probably meaningless and should not be taken seriously.

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits,

Quoted 2 lines · Andrea Arcangeli

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits,

We could just make this depend on the calculated inactive_ratio, which depends on the size of the list.

For small systems, it may make sense to make every accessed bit count, because the working set will often approach the size of memory.

On very large systems, the working set may also approach the size of memory, but the inactive list only contains a small percentage of the pages, so there is enough space for everything.

Say, if the inactive_ratio is 3 or less, make the accessed bit on the active lists count.

Quoted 12 lines

Quoted 2 lines · Andrea Arcangeli

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits,

We could just make this depend on the calculated inactive_ratio, which depends on the size of the list.

For small systems, it may make sense to make every accessed bit count, because the working set will often approach the size of memory.

Quoted 6 lines

On very large systems, the working set may also approach the size of memory, but the inactive list only contains a small percentage of the pages, so there is enough space for everything.

Say, if the inactive_ratio is 3 or less, make the accessed bit on the active lists count.

Sound reasonable.

How do we confirm the idea correctness?

Wu, your X focus switching benchmark is sufficient test?

Quoted 11 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 2 lines · Andrea Arcangeli

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits,

We could just make this depend on the calculated inactive_ratio, which depends on the size of the list.

For small systems, it may make sense to make every accessed bit count, because the working set will often approach the size of memory.

Quoted 7 lines

Quoted 6 lines

On very large systems, the working set may also approach the size of memory, but the inactive list only contains a small percentage of the pages, so there is enough space for everything.

Say, if the inactive_ratio is 3 or less, make the accessed bit on the active lists count.

Sound reasonable.

Yes, such kind of global measurements would be much better.

Quoted 1 line

How do we confirm the idea correctness?

In general the active list tends to grow large on under-scanned LRU. I guess Rik is pretty familiar with typical inactive_ratio values of the large memory systems and may even have some real numbers :)

Quoted 1 line

Wu, your X focus switching benchmark is sufficient test?

It is a major test case for memory tight desktop. Jeff presents another interesting one for KVM, hehe.

Anyway I collected the active/inactive list sizes, and the numbers show that the inactive_ratio is roughly 1 when the LRU is scanned actively and may go very high when it is under-scanned.

Quoted 30 lines · Wu Fengguang

Quoted 11 lines · KOSAKI Motohiro

Quoted 11 lines

Quoted 2 lines · Andrea Arcangeli

Likely we need a cut-off point, if we detect it takes more than X seconds to scan the whole active list, we start ignoring young bits,

We could just make this depend on the calculated inactive_ratio, which depends on the size of the list.

For small systems, it may make sense to make every accessed bit count, because the working set will often approach the size of memory.

Quoted 7 lines

Quoted 6 lines

On very large systems, the working set may also approach the size of memory, but the inactive list only contains a small percentage of the pages, so there is enough space for everything.

Say, if the inactive_ratio is 3 or less, make the accessed bit on the active lists count.

Sound reasonable.

Yes, such kind of global measurements would be much better.

Quoted 1 line

How do we confirm the idea correctness?

In general the active list tends to grow large on under-scanned LRU. I guess Rik is pretty familiar with typical inactive_ratio values of the large memory systems and may even have some real numbers :)

Quoted 1 line

Wu, your X focus switching benchmark is sufficient test?

Quoted 6 lines

It is a major test case for memory tight desktop. Jeff presents another interesting one for KVM, hehe.

Anyway I collected the active/inactive list sizes, and the numbers show that the inactive_ratio is roughly 1 when the LRU is scanned actively and may go very high when it is under-scanned.

inactive_ratio is based on the zone (or cgroup) size.

For zones it is a fixed value, which is available in /proc/zoneinfo

Quoted 2 lines · Rik van Riel

For zones it is a fixed value, which is available in /proc/zoneinfo

On my 64bit desktop with 4GB memory:

        DMA     inactive_ratio:    1
        DMA32   inactive_ratio:    4
        Normal  inactive_ratio:    1

The biggest zone DMA32 has inactive_ratio=4. But I guess the referenced bit should not be ignored on this typical desktop configuration?

We need to ignore the referenced bit on active anon pages on very large systems, but it could indeed be helpful to respect the referenced bit on smaller systems.

I have no idea where the cut-off between them would be.

Maybe at inactive_ratio <= 4 ?

Quoted 7 lines · Rik van Riel

We need to ignore the referenced bit on active anon pages on very large systems, but it could indeed be helpful to respect the referenced bit on smaller systems.

I have no idea where the cut-off between them would be.

Maybe at inactive_ratio <= 4 ?

Why do we need to ignore the referenced bit in such cases? To avoid overscanning?

Quoted 1 line

Why do we need to ignore the referenced bit in such cases? To avoid overscanning?

Because swapping out anonymous pages tends to be a relatively rare operation, we'll have many gigabytes of anonymous pages that all have the referenced bit set (because there was lots of time between swapout bursts).

Ignoring the referenced bit on active anon pages makes no difference on these systems, because all active anon pages have the referenced bit set, anyway.

All we need to do is put the pages on the inactive list and give them a chance to get referenced.

However, on smaller systems (and cgroups!), the speed at which we can do pageout IO is larger, compared to the amount of memory. This means we can cycle through the pages more quickly and we may want to count references on the active list, too.

Yes, on smaller systems we'll also often end up with bursty swapout loads and all pages referenced - but since we have fewer pages to begin with, it won't hurt as much.

I suspect that an inactive_ratio of 3 or 4 might make a good cutoff value.

Quoted 22 lines · Rik van Riel

Quoted 2 lines

Why do we need to ignore the referenced bit in such cases? To avoid overscanning?

Because swapping out anonymous pages tends to be a relatively rare operation, we'll have many gigabytes of anonymous pages that all have the referenced bit set (because there was lots of time between swapout bursts).

Ignoring the referenced bit on active anon pages makes no difference on these systems, because all active anon pages have the referenced bit set, anyway.

All we need to do is put the pages on the inactive list and give them a chance to get referenced.

However, on smaller systems (and cgroups!), the speed at which we can do pageout IO is larger, compared to the amount of memory. This means we can cycle through the pages more quickly and we may want to count references on the active list, too.

Quoted 7 lines

Yes, on smaller systems we'll also often end up with bursty swapout loads and all pages referenced - but since we have fewer pages to begin with, it won't hurt as much.

I suspect that an inactive_ratio of 3 or 4 might make a good cutoff value.

Thanks for the explanation. I think my earlier idea of

- do not ignore the referenced bit
- if you see a run of N pages which all have the referenced bit set, do swap one

has merit. It means we cycle more quickly (by a factor of N) through the list, looking for unreferenced pages. If we don't find any we've spent a some more cpu, but if we do find an unreferenced page, we win by swapping a truly unneeded page.

Quoted 6 lines

- do not ignore the referenced bit
- if you see a run of N pages which all have the referenced bit set, do swap one

But it also means destroying the LRU order.

Quoted 7 lines · Johannes Weiner

Quoted 6 lines

- do not ignore the referenced bit
- if you see a run of N pages which all have the referenced bit set, do swap one

Quoted 3 lines

But it also means destroying the LRU order.

True, it would, but if we ignore the referenced bit, LRU order is really FIFO.

Quoted 8 lines · Avi Kivity

Quoted 7 lines · Johannes Weiner

Quoted 6 lines

- do not ignore the referenced bit
- if you see a run of N pages which all have the referenced bit set, do swap one

Quoted 6 lines

Quoted 3 lines

But it also means destroying the LRU order.

True, it would, but if we ignore the referenced bit, LRU order is really FIFO.

For the active list, yes. But it's not that we degrade to First Fault First Out in a global scope, we still update the order from mark_page_accessed() and by activating referenced pages in shrink_page_list() etc.

So even with the active list being a FIFO, we keep usage information gathered from the inactive list. If we deactivate pages in arbitrary list intervals, we throw this away.

And while global FIFO-based reclaim does not work too well, initial fault order is a valuable hint in the aspect of referential locality as the pages get used in groups and thus move around the lists in groups.

Our granularity for regrouping decisions is pretty coarse, for non-filecache pages it's basically 'referenced or not refrenced in the last list round-trip', so it will take quite some time to regroup pages that are used in truly similar intervals.

Quoted 3 lines · Johannes Weiner

So even with the active list being a FIFO, we keep usage information gathered from the inactive list. If we deactivate pages in arbitrary list intervals, we throw this away.

We do have the danger of FIFO, if inactive list is small enough, so that (unconditionally) deactivated pages quickly get reclaimed and their life window in inactive list is too small to be useful.

Quoted 9 lines · Wu Fengguang

Quoted 3 lines · Johannes Weiner

So even with the active list being a FIFO, we keep usage information gathered from the inactive list. If we deactivate pages in arbitrary list intervals, we throw this away.

We do have the danger of FIFO, if inactive list is small enough, so that (unconditionally) deactivated pages quickly get reclaimed and their life window in inactive list is too small to be useful.

This one of the reasons why we unconditionally deactivate the active anon pages, and do background scanning of the active anon list when reclaiming page cache pages.

We want to always move some pages to the inactive anon list, so it does not get too small.

Quoted 11 lines · Rik van Riel

Quoted 9 lines · Wu Fengguang

Quoted 3 lines · Johannes Weiner

So even with the active list being a FIFO, we keep usage information gathered from the inactive list. If we deactivate pages in arbitrary list intervals, we throw this away.

We do have the danger of FIFO, if inactive list is small enough, so that (unconditionally) deactivated pages quickly get reclaimed and their life window in inactive list is too small to be useful.

Quoted 6 lines

This one of the reasons why we unconditionally deactivate the active anon pages, and do background scanning of the active anon list when reclaiming page cache pages.

We want to always move some pages to the inactive anon list, so it does not get too small.

Right, the current code tries to pull inactive list out of smallish-size state as long as there are vmscan activities.

However there is a possible (and tricky) hole: mem cgroups don't do batched vmscan. shrink_zone() may call shrink_list() with nr_to_scan=1, in which case shrink_list() _still_ calls isolate_pages() with the much larger SWAP_CLUSTER_MAX.

It effectively scales up the inactive list scan rate by 10 times when it is still small, and may thus prevent it from growing up for ever.

In that case, LRU becomes FIFO.

Jeff, can you confirm if the mem cgroup's inactive list is small? If so, this patch should help.

Thanks,
Fengguang
---

mm: do batched scans for mem_cgroup

Signed-off-by Wu Fengguang <[email protected]>

---
 include/linux/memcontrol.h |    3 +++
 mm/memcontrol.c            |   12 ++++++++++++
 mm/vmscan.c                |    9 +++++----
 3 files changed, 20 insertions(+), 4 deletions(-)

include/linux/memcontrol.h +3 -0

@@ -98,6 +98,9 @@ int mem_cgroup_inactive_file_is_low(stru
 unsigned long mem_cgroup_zone_nr_pages(struct mem_cgroup *memcg,
 				       struct zone *zone,
 				       enum lru_list lru);
+unsigned long *mem_cgroup_get_saved_scan(struct mem_cgroup *memcg,
+					 struct zone *zone,
+					 enum lru_list lru);
 struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
 						      struct zone *zone);
 struct zone_reclaim_stat*

mm/memcontrol.c +12 -0

@@ -115,6 +115,7 @@ struct mem_cgroup_per_zone {
 	 */
 	struct list_head	lists[NR_LRU_LISTS];
 	unsigned long		count[NR_LRU_LISTS];
+	unsigned long		nr_saved_scan[NR_LRU_LISTS];
 
 	struct zone_reclaim_stat reclaim_stat;
 };

@@ -597,6 +598,17 @@ unsigned long mem_cgroup_zone_nr_pages(s
 	return MEM_CGROUP_ZSTAT(mz, lru);
 }
 
+unsigned long *mem_cgroup_get_saved_scan(struct mem_cgroup *memcg,
+					 struct zone *zone,
+					 enum lru_list lru)
+{
+	int nid = zone->zone_pgdat->node_id;
+	int zid = zone_idx(zone);
+	struct mem_cgroup_per_zone *mz = mem_cgroup_zoneinfo(memcg, nid, zid);
+
+	return &mz->nr_saved_scan[lru];
+}
+
 struct zone_reclaim_stat *mem_cgroup_get_reclaim_stat(struct mem_cgroup *memcg,
 						      struct zone *zone)
 {

mm/vmscan.c +1 -1

@@ -1534,6 +1534,7 @@ static void shrink_zone(int priority, st
 	for_each_evictable_lru(l) {
 		int file = is_file_lru(l);
 		unsigned long scan;
+		unsigned long *saved_scan;
 
 		scan = zone_nr_pages(zone, sc, l);
 		if (priority || noswap) {

@@ -1541,11 +1542,11 @@ static void shrink_zone(int priority, st
 			scan = (scan * percent[file]) / 100;
 		}
 		if (scanning_global_lru(sc))
-			nr[l] = nr_scan_try_batch(scan,

Quoted 27 lines · Wu Fengguang

Quoted 11 lines · Rik van Riel

Quoted 9 lines · Wu Fengguang

Quoted 3 lines · Johannes Weiner

So even with the active list being a FIFO, we keep usage information gathered from the inactive list. If we deactivate pages in arbitrary list intervals, we throw this away.

We do have the danger of FIFO, if inactive list is small enough, so that (unconditionally) deactivated pages quickly get reclaimed and their life window in inactive list is too small to be useful.

Quoted 6 lines

This one of the reasons why we unconditionally deactivate the active anon pages, and do background scanning of the active anon list when reclaiming page cache pages.

We want to always move some pages to the inactive anon list, so it does not get too small.

Right, the current code tries to pull inactive list out of smallish-size state as long as there are vmscan activities.

However there is a possible (and tricky) hole: mem cgroups don't do batched vmscan. shrink_zone() may call shrink_list() with nr_to_scan=1, in which case shrink_list() _still_ calls isolate_pages() with the much larger SWAP_CLUSTER_MAX.

Quoted 3 lines

It effectively scales up the inactive list scan rate by 10 times when it is still small, and may thus prevent it from growing up for ever.

I think we need to possibly export some scanning data under DEBUG_VM to cross verify.

Quoted 17 lines · Wu Fengguang

Quoted 3 lines · Rik van Riel

Quoted 1 line · Wu Fengguang

On Fri, Aug 14, 2009 at 05:10:55PM +0800, Johannes Weiner wrote:

mm/vmscan.c +4 -4

@@ -1541,11 +1542,11 @@ static void shrink_zone(int priority, st
 			scan = (scan * percent[file]) / 100;
 		}
 		if (scanning_global_lru(sc))
-			nr[l] = nr_scan_try_batch(scan,
-						  &zone->lru[l].nr_saved_scan,
-						  swap_cluster_max);
+			saved_scan = &zone->lru[l].nr_saved_scan;
 		else
-			nr[l] = scan;
+			saved_scan = mem_cgroup_get_saved_scan(sc->mem_cgroup,
+							       zone, l);
+		nr[l] = nr_scan_try_batch(scan, saved_scan, swap_cluster_max);