mm: rmap: don't try to add an unevictable page to lru list

PATCH In review Bob Liu 3 participants 7 messages Mar 31, 2014 – Apr 24, 2014

Subsystems (3)

Memory management MM migration MM rmap

lore

Share & tools

lore

Bob Liu

author

2014-03-31 03:07 UTC #0 lore raw

PATCH 1/1 mm: rmap: don't try to add an unevictable page to lru list

VM_BUG_ON_PAGE(PageActive(page) && PageUnevictable(page), page) in lru_cache_add() was triggered during migrate_misplaced_transhuge_page.

[  477.301955] kernel BUG at mm/swap.c:609!
[  477.302564] invalid opcode: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
[  477.303590] Dumping ftrace buffer:
[  477.305022]    (ftrace buffer empty)
[  477.305899] Modules linked in:
[  477.306397] CPU: 35 PID: 10092 Comm: trinity-c374 Tainted: G        W
3.14.0-rc5-next-20140307-sasha-00010-g1f812cb #142
[  477.307644] task: ffff8800a7f80000 ti: ffff8800a7f6a000 task.ti:
ffff8800a7f6a000
[  477.309124] RIP: 0010:[<ffffffff8127f311>]  [<ffffffff8127f311>]
lru_cache_add+0x21/0x60
[  477.310301] RSP: 0000:ffff8800a7f6bbc8  EFLAGS: 00010292
[  477.311110] RAX: 000000000000003f RBX: ffffea0013d68000 RCX:
0000000000000006
[  477.311110] RDX: 0000000000000006 RSI: ffff8800a7f80d60 RDI:
0000000000000282
[  477.311110] RBP: ffff8800a7f6bbc8 R08: 0000000000000001 R09:
0000000000000001
[  477.311110] R10: 0000000000000001 R11: 0000000000000001 R12:
ffff8800ab9b0c00
[  477.311110] R13: 0000000002400000 R14: ffff8800ab9b0c00 R15:
0000000000000001
[  477.311110] FS:  00007ff2c047c700(0000) GS:ffff88042bc00000(0000)
knlGS:0000000000000000
[  477.311110] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  477.311110] CR2: 0000000003788a68 CR3: 00000000a7f68000 CR4:
00000000000006a0
[  477.311110] DR0: 000000000069b000 DR1: 0000000000000000 DR2:
0000000000000000
[  477.311110] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7:
0000000000000600
[  477.311110] Stack:
[  477.311110]  ffff8800a7f6bbf8 ffffffff812adaec ffffea0013d68000
ffffea002bdb8000
[  477.311110]  ffffea0013d68000 ffff8800a7f7c090 ffff8800a7f6bca8
ffffffff812db8ec
[  477.311110]  0000000000000001 ffffffff812e1321 ffff8800a7f6bc48
ffffffff811ad632
[  477.311110] Call Trace:
[  477.311110]  [<ffffffff812adaec>] page_add_new_anon_rmap+0x1ec/0x210
[  477.311110]  [<ffffffff812db8ec>]
migrate_misplaced_transhuge_page+0x55c/0x830
[  477.311110]  [<ffffffff812e1321>] ? do_huge_pmd_numa_page+0x311/0x460
[  477.311110]  [<ffffffff811ad632>] ? __lock_release+0x1e2/0x200
[  477.311110]  [<ffffffff812e133f>] do_huge_pmd_numa_page+0x32f/0x460
[  477.311110]  [<ffffffff81af6aca>] ? delay_tsc+0xfa/0x120
[  477.311110]  [<ffffffff812a31f4>] __handle_mm_fault+0x244/0x3a0
[  477.311110]  [<ffffffff812e37ed>] ? rcu_read_unlock+0x5d/0x60
[  477.311110]  [<ffffffff812a3463>] handle_mm_fault+0x113/0x1c0
[  477.311110]  [<ffffffff844abd42>] ? __do_page_fault+0x302/0x5d0
[  477.311110]  [<ffffffff844abfd1>] __do_page_fault+0x591/0x5d0
[  477.311110]  [<ffffffff8118ab46>] ? vtime_account_user+0x96/0xb0
[  477.311110]  [<ffffffff844ac492>] ? preempt_count_sub+0xe2/0x120
[  477.311110]  [<ffffffff81269567>] ? context_tracking_user_exit+0x187/0x1d0
[  477.311110]  [<ffffffff844ac0d5>] do_page_fault+0x45/0x70
[  477.311110]  [<ffffffff844ab386>] do_async_page_fault+0x36/0x100
[  477.311110]  [<ffffffff844a7f18>] async_page_fault+0x28/0x30
[  477.311110] Code: 65 f0 4c 8b 6d f8 c9 c3 66 90 55 48 89 e5 66 66 66 66 90
48 8b 07 a8 40 74 18 48 8b 07 a9 00 00 10 00 74 0e 31 f6 e8 2f 20 ff ff <0f>
0b eb fe 0f 1f 00 48 8b 07 a8 20 74 19 31 f6 e8 1a 20 ff ff
[  477.311110] RIP  [<ffffffff8127f311>] lru_cache_add+0x21/0x60
[  477.311110]  RSP <ffff8800a7f6bbc8>

The root cause is the checking mlocked_vma_newpage() in page_add_new_anon_rmap() is not enough to decide whether a page is unevictable.

migrate_misplaced_transhuge_page():

	=> migrate_page_copy()
		=> SetPageUnevictable(newpage)

	=> page_add_new_anon_rmap(newpage)
		=> mlocked_vma_newpage(vma, newpage) <--This check is not enough
			=> SetPageActive(newpage)
			=> lru_cache_add(newpage)
				=> VM_BUG_ON_PAGE()

Quoted 1 line

From vmscan.c:

* Reasons page might not be evictable:
* (1) page's mapping marked unevictable
* (2) page is part of an mlocked VMA

But page_add_new_anon_rmap() only checks reason (2), we may hit this VM_BUG_ON_PAGE() if PageUnevictable(old_page) was originally set by reason (1).

Reported-by Sasha Levin <[email protected]>

Signed-off-by Bob Liu <[email protected]>

---
 mm/rmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/rmap.c +1 -1

index 43d429b..39458c5 100644
@@ -1024,7 +1024,7 @@ void page_add_new_anon_rmap(struct page *page,
 	__mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
 			hpage_nr_pages(page));
 	__page_set_anon_rmap(page, vma, address, 1);
-	if (!mlocked_vma_newpage(vma, page)) {
+	if (!mlocked_vma_newpage(vma, page) && !PageUnevictable(page)) {
 		SetPageActive(page);
 		lru_cache_add(page);
 	} else

--
1.7.10.4

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

Hugh Dickins

2014-04-05 09:06 UTC #1 lore raw

Quoted 21 lines · Bob Liu

VM_BUG_ON_PAGE(PageActive(page) && PageUnevictable(page), page) in lru_cache_add() was triggered during migrate_misplaced_transhuge_page.
kernel BUG at mm/swap.c:609!
[<ffffffff8127f311>] lru_cache_add+0x21/0x60
[<ffffffff812adaec>] page_add_new_anon_rmap+0x1ec/0x210
[<ffffffff812db8ec>] migrate_misplaced_transhuge_page+0x55c/0x830
The root cause is the checking mlocked_vma_newpage() in page_add_new_anon_rmap() is not enough to decide whether a page is unevictable.
migrate_misplaced_transhuge_page():
	=> migrate_page_copy()
		=> SetPageUnevictable(newpage)

	=> page_add_new_anon_rmap(newpage)
		=> mlocked_vma_newpage(vma, newpage) <--This check is not enough
			=> SetPageActive(newpage)
			=> lru_cache_add(newpage)
				=> VM_BUG_ON_PAGE()

Quoted 7 lines

From vmscan.c:
* Reasons page might not be evictable:
* (1) page's mapping marked unevictable
* (2) page is part of an mlocked VMA
But page_add_new_anon_rmap() only checks reason (2), we may hit this VM_BUG_ON_PAGE() if PageUnevictable(old_page) was originally set by reason (1).

But (1) always reports evictable on an anon page, doesn't it?

Quoted 3 lines

Reported-by Sasha Levin <[email protected]>
Signed-off-by Bob Liu <[email protected]>

I can't quite assert NAK, but I suspect this is not the proper fix.

Initially I was uncomfortable with it for largely aesthetic reasons. page_add_new_anon_rmap() is a cut-some-corners fast-path collection of rmap and lru stuff for the common case, the first time a page is added.

If what it does is not suitable for the unusual case of page migration, then we should not clutter it up with additional tests, but adjust migration to use the slower page_add_anon_rmap() instead.

Or, if there turns out to be some really good reason to stick with page_add_new_anon_rmap(), add an inline comment to explain why this additional !PageUnevictable test (never needed before) is needed now.

Note that the call from migrate_misplaced_transhuge_page() is the only use of page_add_new_anon_rmap() in mm/migrate.c: I think it's a mistake, and should use page_add_anon_rmap() plus putback_lru_page() like elsewhere in migrate.c.

Beware, I've not written, let alone tested, a patch to do so: maybe more is needed. In particular, it's unclear whether Mel intended the SetPageActive that comes bundled up in page_add_new_anon_rmap(), when normally migration just transfers PageActive state from old to new.

I went through a phase of thinking your patch is downright wrong, that in the racy case it puts a recently-become-evictable page back to the unevictable lru. Currently I believe I was wrong about that, the page lock (on old page) or mmap_sem preventing that possibility.

(Yet now I'm wavering again: if down_write mmap_sem is needed to munlock() the vma, and migrate_misplaced_transhuge_page() is only migrating a singly-mapped THP under down_read mmap_sem, how could VM_LOCKED have changed during the migration? I've lost sight of how we got to hit the BUG altogether, maybe I'm just too tired.)

Even so, I'd be much more comfortable with a page_add_anon_rmap() plus putback_lru_page() approach; but we probably need Mel to explain why he chose not to do it that way (my guess is it just seemed simpler this way, relying on the singly-mapped aspect), and someone to explain again how we hit the BUG.

Hugh

Quoted 18 lines

---
 mm/rmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

mm/rmap.c +1 -2

index 43d429b..39458c5 100644
@@ -1024,7 +1024,7 @@ void page_add_new_anon_rmap(struct page *page,
 	__mod_zone_page_state(page_zone(page), NR_ANON_PAGES,
 			hpage_nr_pages(page));
 	__page_set_anon_rmap(page, vma, address, 1);
-	if (!mlocked_vma_newpage(vma, page)) {
+	if (!mlocked_vma_newpage(vma, page) && !PageUnevictable(page)) {
 		SetPageActive(page);
 		lru_cache_add(page);
 	} else
--

Quoted 1 line

1.7.10.4

Bob Liu

author

2014-04-08 08:58 UTC #2 lore raw

Quoted 66 lines · Hugh Dickins

Quoted 21 lines · Bob Liu
VM_BUG_ON_PAGE(PageActive(page) && PageUnevictable(page), page) in lru_cache_add() was triggered during migrate_misplaced_transhuge_page.
kernel BUG at mm/swap.c:609!
[<ffffffff8127f311>] lru_cache_add+0x21/0x60
[<ffffffff812adaec>] page_add_new_anon_rmap+0x1ec/0x210
[<ffffffff812db8ec>] migrate_misplaced_transhuge_page+0x55c/0x830
The root cause is the checking mlocked_vma_newpage() in page_add_new_anon_rmap() is not enough to decide whether a page is unevictable.
migrate_misplaced_transhuge_page():
      => migrate_page_copy()
              => SetPageUnevictable(newpage)

      => page_add_new_anon_rmap(newpage)
              => mlocked_vma_newpage(vma, newpage) <--This check is not enough
                      => SetPageActive(newpage)
                      => lru_cache_add(newpage)
                              => VM_BUG_ON_PAGE()
Quoted 7 lines
From vmscan.c:
* Reasons page might not be evictable:
* (1) page's mapping marked unevictable
* (2) page is part of an mlocked VMA
But page_add_new_anon_rmap() only checks reason (2), we may hit this VM_BUG_ON_PAGE() if PageUnevictable(old_page) was originally set by reason (1).
But (1) always reports evictable on an anon page, doesn't it?
Quoted 3 lines
Reported-by Sasha Levin <[email protected]>
Signed-off-by Bob Liu <[email protected]>
I can't quite assert NAK, but I suspect this is not the proper fix.
Initially I was uncomfortable with it for largely aesthetic reasons. page_add_new_anon_rmap() is a cut-some-corners fast-path collection of rmap and lru stuff for the common case, the first time a page is added.
If what it does is not suitable for the unusual case of page migration, then we should not clutter it up with additional tests, but adjust migration to use the slower page_add_anon_rmap() instead.
Or, if there turns out to be some really good reason to stick with page_add_new_anon_rmap(), add an inline comment to explain why this additional !PageUnevictable test (never needed before) is needed now.
Note that the call from migrate_misplaced_transhuge_page() is the only use of page_add_new_anon_rmap() in mm/migrate.c: I think it's a mistake, and should use page_add_anon_rmap() plus putback_lru_page() like elsewhere in migrate.c.
Beware, I've not written, let alone tested, a patch to do so: maybe more is needed. In particular, it's unclear whether Mel intended the SetPageActive that comes bundled up in page_add_new_anon_rmap(), when normally migration just transfers PageActive state from old to new.
I went through a phase of thinking your patch is downright wrong, that in the racy case it puts a recently-become-evictable page back to the unevictable lru. Currently I believe I was wrong about that, the page lock (on old page) or mmap_sem preventing that possibility.

Quoted 4 lines

(Yet now I'm wavering again: if down_write mmap_sem is needed to munlock() the vma, and migrate_misplaced_transhuge_page() is only migrating a singly-mapped THP under down_read mmap_sem, how could VM_LOCKED have changed during the migration? I've lost sight of

I think you are right, I'll do more investigation about why this BUG was triggered.

Signature · 8 lines

Regards,
--Bob

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

Hugh Dickins

2014-04-24 03:10 UTC #3 lore raw

Quoted 21 lines · Bob Liu

Quoted 20 lines · Hugh Dickins
Quoted 10 lines · Bob Liu
VM_BUG_ON_PAGE(PageActive(page) && PageUnevictable(page), page) in lru_cache_add() was triggered during migrate_misplaced_transhuge_page.
...
From vmscan.c:
* Reasons page might not be evictable:
* (1) page's mapping marked unevictable
* (2) page is part of an mlocked VMA
But page_add_new_anon_rmap() only checks reason (2), we may hit this VM_BUG_ON_PAGE() if PageUnevictable(old_page) was originally set by reason (1).
But (1) always reports evictable on an anon page, doesn't it?
Quoted 3 lines
Reported-by Sasha Levin <[email protected]>
Signed-off-by Bob Liu <[email protected]>
I can't quite assert NAK, but I suspect this is not the proper fix.

...

Quoted 8 lines

Quoted 5 lines
(Yet now I'm wavering again: if down_write mmap_sem is needed to munlock() the vma, and migrate_misplaced_transhuge_page() is only migrating a singly-mapped THP under down_read mmap_sem, how could VM_LOCKED have changed during the migration? I've lost sight of
I think you are right, I'll do more investigation about why this BUG was triggered.

Andrew, if Bob agrees, please drop

mm-rmap-dont-try-to-add-an-unevictable-page-to-lru-list.patch

from mmotm now. We have not heard any such report yet on 3.15-rc, and neither Bob nor I have yet come up with a convincing explanation for how it came about. It's tempting to suppose it was a side-effect of something temporarily wrong on a 3.14-next, and now okay; but we'll learn more quickly whether that's so if mmotm stops working around it.

Thanks,
Hugh

Bob Liu

author

2014-04-24 03:23 UTC #4 lore raw

Quoted 29 lines · Hugh Dickins

Quoted 21 lines · Bob Liu
Quoted 20 lines · Hugh Dickins
Quoted 10 lines · Bob Liu
VM_BUG_ON_PAGE(PageActive(page) && PageUnevictable(page), page) in lru_cache_add() was triggered during migrate_misplaced_transhuge_page.
...
From vmscan.c:
* Reasons page might not be evictable:
* (1) page's mapping marked unevictable
* (2) page is part of an mlocked VMA
But page_add_new_anon_rmap() only checks reason (2), we may hit this VM_BUG_ON_PAGE() if PageUnevictable(old_page) was originally set by reason (1).
But (1) always reports evictable on an anon page, doesn't it?
Quoted 3 lines
Reported-by Sasha Levin <[email protected]>
Signed-off-by Bob Liu <[email protected]>
I can't quite assert NAK, but I suspect this is not the proper fix.
...
Quoted 6 lines
Quoted 5 lines
(Yet now I'm wavering again: if down_write mmap_sem is needed to munlock() the vma, and migrate_misplaced_transhuge_page() is only migrating a singly-mapped THP under down_read mmap_sem, how could VM_LOCKED have changed during the migration? I've lost sight of

Quoted 7 lines

Quoted 2 lines
I think you are right, I'll do more investigation about why this BUG was triggered.
Andrew, if Bob agrees, please drop
mm-rmap-dont-try-to-add-an-unevictable-page-to-lru-list.patch

I agree!

Quoted 1 line

from mmotm now. We have not heard any such report yet on 3.15-rc,

Yes, I can't reproduce it on 3.15-rc1 neither.

Quoted 5 lines

and neither Bob nor I have yet come up with a convincing explanation for how it came about. It's tempting to suppose it was a side-effect of something temporarily wrong on a 3.14-next, and now okay; but we'll learn more quickly whether that's so if mmotm stops working around it.

Agree! Thank you for taking look at this issue.

Signature · 8 lines

Regards,
--Bob

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

Mel Gorman

2014-04-24 15:39 UTC #5 lore raw

PATCH 1/1 mm: numa: Add migrated transhuge pages to LRU the same way as base pages

In reply to Hugh Dickins

Migration of misplaced transhuge pages uses page_add_new_anon_rmap() when putting the page back as it avoided an atomic operations and added the new page to the correct LRU. A side-effect is that the page gets marked activated as part of the migration meaning that transhuge and base pages are treated differently from an aging perspective than base page migration.

This patch uses page_add_anon_rmap() and putback_lru_page() on completion of a transhuge migration similar to base page migration. It would fewer atomic operations to use lru_cache_add without taking an additional reference to the page. The downside would be that it's still different to base page migration and unevictable pages may be added to the wrong LRU for cleaning up later. Testing of the usual workloads did not show any adverse impact to the change.

Signed-off-by Mel Gorman <[email protected]>

---
 mm/migrate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/migrate.c +5 -1

index bed4880..6247be7 100644
@@ -1852,7 +1852,7 @@ fail_putback:
 	 * guarantee the copy is visible before the pagetable update.
 	 */
 	flush_cache_range(vma, mmun_start, mmun_end);
-	page_add_new_anon_rmap(new_page, vma, mmun_start);
+	page_add_anon_rmap(new_page, vma, mmun_start);
 	pmdp_clear_flush(vma, mmun_start, pmd);
 	set_pmd_at(mm, mmun_start, pmd, entry);
 	flush_tlb_range(vma, mmun_start, mmun_end);

@@ -1877,6 +1877,10 @@ fail_putback:
 	spin_unlock(ptl);
 	mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end);
 
+	/* Take an "isolate" reference and put new page on the LRU. */
+	get_page(new_page);
+	putback_lru_page(new_page);
+
 	unlock_page(new_page);
 	unlock_page(page);
 	put_page(page);			/* Drop the rmap reference */

Hugh Dickins

Acked-by on v1 1/1

2014-04-24 19:50 UTC #6 lore raw

Re: [PATCH] mm: numa: Add migrated transhuge pages to LRU the same way as base pages

Quoted 14 lines · Mel Gorman

Migration of misplaced transhuge pages uses page_add_new_anon_rmap() when putting the page back as it avoided an atomic operations and added the new page to the correct LRU. A side-effect is that the page gets marked activated as part of the migration meaning that transhuge and base pages are treated differently from an aging perspective than base page migration.
This patch uses page_add_anon_rmap() and putback_lru_page() on completion of a transhuge migration similar to base page migration. It would fewer atomic operations to use lru_cache_add without taking an additional reference to the page. The downside would be that it's still different to base page migration and unevictable pages may be added to the wrong LRU for cleaning up later. Testing of the usual workloads did not show any adverse impact to the change.

Quoted 1 line

Signed-off-by Mel Gorman <[email protected]>

Acked-by Hugh Dickins <[email protected]>

Thanks, Mel: I do feel more comfortable doing it this way, as elsewhere in migrate.c, whatever the slight drawbacks (I hadn't realized that I was asking for another get_page, but it's far from the only place where isolate/putback forces us into that additional superfluous reference:
something to sort out "some other day"). I worried for a bit that you are doing the putback while holding page lock; but this is not the only place where that's done (page_remove_rmap may clear_page_mlock though called under page lock), so no good reason to ask for a change there.

Quoted 19 lines

---
 mm/migrate.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

mm/migrate.c +1 -1

index bed4880..6247be7 100644
@@ -1852,7 +1852,7 @@ fail_putback:
 	 * guarantee the copy is visible before the pagetable update.
 	 */
 	flush_cache_range(vma, mmun_start, mmun_end);
-	page_add_new_anon_rmap(new_page, vma, mmun_start);
+	page_add_anon_rmap(new_page, vma, mmun_start);
 	pmdp_clear_flush(vma, mmun_start, pmd);
 	set_pmd_at(mm, mmun_start, pmd, entry);
 	flush_tlb_range(vma, mmun_start, mmun_end);

mm/migrate.c fail_putback: · line 1886

Quoted 10 lines

mm/migrate.c +4 -0

@@ -1877,6 +1877,10 @@ fail_putback:
 	spin_unlock(ptl);
 	mmu_notifier_invalidate_range_end(mm, mmun_start, mmun_end);
 
+	/* Take an "isolate" reference and put new page on the LRU. */
+	get_page(new_page);
+	putback_lru_page(new_page);
+
 	unlock_page(new_page);
 	unlock_page(page);
 	put_page(page);			/* Drop the rmap reference */