Re: Race in new page migration code?

[Christoph Lameter <clameter@engr.sgi.com>]

On Mon, 16 Jan 2006, Hugh Dickins wrote:

> Hmm, that battery of unusual tests at the start of migrate_page_add
> is odd: the tests don't quite match the comment, and it isn't clear
> what reasoning lies behind the comment anyway.

Here is patch to clarify the test. I'd be glad if someone could make
the tests more accurate. This ultimately comes down to a concept of
ownership of page by a process / mm_struct that we have to approximate.

===

Explain the complicated check in migrate_page_add by putting the logic
into a separate function migration_check. This way any enhancements can
be easily added.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.15/mm/mempolicy.c
===================================================================
--- linux-2.6.15.orig/mm/mempolicy.c	2006-01-14 10:56:28.000000000 -0800
+++ linux-2.6.15/mm/mempolicy.c	2006-01-17 09:24:20.000000000 -0800
@@ -551,6 +551,37 @@ out:
 	return rc;
 }
 
+static inline int migration_check(struct mm_struct *mm, struct page *page)
+{
+	/*
+	 * If the page has no mapping then we do not track reverse mappings.
+	 * Thus the page is not mapped by other mms, so its safe to move.
+	 */
+	if (page->mapping)
+		return 1;
+
+	/*
+	 * We cannot determine "ownership" of anonymous pages.
+	 * However, this is the primary set of pages a user would like
+	 * to move. So move the page regardless of sharing.
+	 */
+	if (PageAnon(page))
+		return 1;
+
+	/*
+	 * If the mapping is writable then its reasonable to assume that
+	 * it is okay to move the page.
+	 */
+	if (mapping_writably_mapped(page->mapping))
+		return 1;
+
+	/*
+	 * Its a read only file backed mapping. Only migrate the page
+	 * if we are the only process mapping that file.
+	 */
+	return single_mm_mapping(mm, page->mapping);
+}
+
 /*
  * Add a page to be migrated to the pagelist
  */
@@ -558,11 +589,17 @@ static void migrate_page_add(struct vm_a
 	struct page *page, struct list_head *pagelist, unsigned long flags)
 {
 	/*
-	 * Avoid migrating a page that is shared by others and not writable.
+	 * MPOL_MF_MOVE_ALL migrates all pages. However, migrating all
+	 * pages may also move commonly shared pages (like for example glibc
+	 * pages referenced by all processes). If these are included in
+	 * migration then these pages may be uselessly moved back and
+	 * forth. Migration may also affect the performance of other
+	 * processes.
+	 *
+	 * If MPOL_MF_MOVE_ALL is not set then we try to avoid migrating
+	 * these shared pages.
 	 */
-	if ((flags & MPOL_MF_MOVE_ALL) || !page->mapping || PageAnon(page) ||
-	    mapping_writably_mapped(page->mapping) ||
-	    single_mm_mapping(vma->vm_mm, page->mapping))
+	if ((flags & MPOL_MF_MOVE_ALL) || migration_check(vma->vm_mm, page))
 		if (isolate_lru_page(page) == 1)
 			list_add(&page->lru, pagelist);
 }
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/