[PATCH mm] swapless page migration: fix swapops.h:97 BUG

[PATCH mm] swapless page migration: fix swapops.h:97 BUG

[Hugh Dickins]

Several times while testing swapless page migration (I've no NUMA, just
hacking it up to migrate recklessly while running load), I've hit the
BUG_ON(!PageLocked(p)) in migration_entry_to_page.

This comes from an orphaned migration entry, unrelated to the current
correctly locked migration, but hit by remove_anon_migration_ptes as
it checks an address in each vma of the anon_vma list.

Such an orphan may be left behind if an earlier migration raced with fork:
copy_one_pte can duplicate a migration entry from parent to child, after
remove_anon_migration_ptes has checked the child vma, but before it has
removed it from the parent vma.  (If the process were later to fault on
this orphaned entry, it would hit the same BUG from migration_entry_wait.)

This could be fixed by locking anon_vma in copy_one_pte, but we'd rather
not.  There's no such problem with file pages, because vma_prio_tree_add
adds child vma after parent vma, and the page table locking at each end
is enough to serialize.  Follow that example with anon_vma: add new vmas
to the tail instead of the head.

(There's no corresponding problem when inserting migration entries,
because a missed pte will leave the page count and mapcount high,
which is allowed for.  And there's no corresponding problem when
migrating via swap, because a leftover swap entry will be correctly
faulted.  But the swapless method has no refcounting of its entries.)

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/rmap.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- 2.6.17-rc4-mm3/mm/rmap.c	2006-05-22 12:19:04.000000000 +0100
+++ linux/mm/rmap.c	2006-05-22 12:51:02.000000000 +0100
@@ -103,7 +103,7 @@ int anon_vma_prepare(struct vm_area_stru
 		spin_lock(&mm->page_table_lock);
 		if (likely(!vma->anon_vma)) {
 			vma->anon_vma = anon_vma;
-			list_add(&vma->anon_vma_node, &anon_vma->head);
+			list_add_tail(&vma->anon_vma_node, &anon_vma->head);
 			allocated = NULL;
 		}
 		spin_unlock(&mm->page_table_lock);
@@ -127,7 +127,7 @@ void __anon_vma_link(struct vm_area_stru
 	struct anon_vma *anon_vma = vma->anon_vma;
 
 	if (anon_vma) {
-		list_add(&vma->anon_vma_node, &anon_vma->head);
+		list_add_tail(&vma->anon_vma_node, &anon_vma->head);
 		validate_anon_vma(vma);
 	}
 }
@@ -138,7 +138,7 @@ void anon_vma_link(struct vm_area_struct
 
 	if (anon_vma) {
 		spin_lock(&anon_vma->lock);
-		list_add(&vma->anon_vma_node, &anon_vma->head);
+		list_add_tail(&vma->anon_vma_node, &anon_vma->head);
 		validate_anon_vma(vma);
 		spin_unlock(&anon_vma->lock);
 	}

Re: [PATCH mm] swapless page migration: fix swapops.h:97 BUG

[Christoph Lameter]

On Wed, 24 May 2006, Hugh Dickins wrote:

> Such an orphan may be left behind if an earlier migration raced with fork:
> copy_one_pte can duplicate a migration entry from parent to child, after
> remove_anon_migration_ptes has checked the child vma, but before it has
> removed it from the parent vma.  (If the process were later to fault on
> this orphaned entry, it would hit the same BUG from migration_entry_wait.)

Right the pte lock does not help us there. sigh.

> This could be fixed by locking anon_vma in copy_one_pte, but we'd rather
> not.  There's no such problem with file pages, because vma_prio_tree_add
> adds child vma after parent vma, and the page table locking at each end
> is enough to serialize.  Follow that example with anon_vma: add new vmas
> to the tail instead of the head.

Thats is very subtle. I hope there are no other areas where the child vma 
has to be processed before the parent vma? An alternativbe would be to 
make remove_anon_migration_ptes walk the list in reverse and add a big 
comment.

Thank you.

Re: [PATCH mm] swapless page migration: fix swapops.h:97 BUG

[Hugh Dickins]

On Wed, 24 May 2006, Christoph Lameter wrote:
> 
> Thats is very subtle. I hope there are no other areas where the child vma 
> has to be processed before the parent vma?

I've considered, and very much doubt it; and if there are, they'd
have to be peculiar to anonymous, since they'd already have been
wrong for the vma_prio_tree cases.

> An alternative would be to 
> make remove_anon_migration_ptes walk the list in reverse and add a big 
> comment.

No, even without this migration issue, I'd much rather have anon_vmas
inserted in the new list_add_tail way, matching the prio_tree direction.
It's asking for trouble to have opposite conventions there (and much
less surprising to insert child after parent than before).

Hugh