Read/Write migration entries: Implement correct behavior in copy_one_pte

Read/Write migration entries: Implement correct behavior in copy_one_pte

[Christoph Lameter]

Note that this is again only a partial solution. mprotect() also has the
potential of changing the write status to read. Are there any additional
occurrences? Would you check and fix this one as well?

If we cannot get to all the locations or if these fixes get too extensive
then I think we better drop the preservation of write permissions and
tolerate the occurrence of some useless COW after migration.



Migration entries with write permission must become SWP_MIGRATION_READ
entries if a COW mapping is processed. The migration entries from which
the copy is being made must also become SWP_MIGRATION_READ. This mimicks
the copying of pte for an anonymous page.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6.17-rc1-mm3/mm/memory.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/mm/memory.c	2006-04-18 10:58:33.000000000 -0700
+++ linux-2.6.17-rc1-mm3/mm/memory.c	2006-04-18 11:09:23.000000000 -0700
@@ -434,7 +434,9 @@ copy_one_pte(struct mm_struct *dst_mm, s
 	/* pte contains position in swap or file, so copy. */
 	if (unlikely(!pte_present(pte))) {
 		if (!pte_file(pte)) {
-			swap_duplicate(pte_to_swp_entry(pte));
+			swp_entry_t entry = pte_to_swp_entry(pte);
+
+			swap_duplicate(entry);
 			/* make sure dst_mm is on swapoff's mmlist. */
 			if (unlikely(list_empty(&dst_mm->mmlist))) {
 				spin_lock(&mmlist_lock);
@@ -443,6 +445,19 @@ copy_one_pte(struct mm_struct *dst_mm, s
 						 &src_mm->mmlist);
 				spin_unlock(&mmlist_lock);
 			}
+			if (is_migration_entry(entry) &&
+					is_cow_mapping(vm_flags)) {
+				page = migration_entry_to_page(entry);
+
+				/*
+				 * COW mappings require pages in both parent
+				*  and child to be set to read.
+				 */
+				entry = make_migration_entry(page,
+	`					SWP_MIGRATION_READ);
+				pte = swp_entry_to_pte(entry);
+				set_pte_at(src_mm, addr, src_pte, pte);
+			}
 		}
 		goto out_set_pte;
 	}

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

Re: Read/Write migration entries: Implement correct behavior in copy_one_pte

[KAMEZAWA Hiroyuki]

On Tue, 18 Apr 2006 11:21:25 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> Note that this is again only a partial solution. mprotect() also has the
> potential of changing the write status to read. 
yes. in change_pte_range(). 

Note:
fork() and mprotect() both requires mm->mmap_sem.
So both of them is not problem when migration holds mm->mmap_sem.
If we does lazy migration or memory hot removing or allows migration from
another process, this will be problem.



> Are there any additional occurrences? Would you check and fix this one as well?
> 
pte_modify() looks to be called only by mprotect(). I checked all mk_pte() but
not found no occurrences now. But I'll have to do more.


> If we cannot get to all the locations or if these fixes get too extensive
> then I think we better drop the preservation of write permissions and
> tolerate the occurrence of some useless COW after migration.
> 
yes. I agree.

-Kame
> 
> 
> Migration entries with write permission must become SWP_MIGRATION_READ
> entries if a COW mapping is processed. The migration entries from which
> the copy is being made must also become SWP_MIGRATION_READ. This mimicks
> the copying of pte for an anonymous page.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Signed-off-by: Christoph Lameter <clameter@sgi.com>
> 
> Index: linux-2.6.17-rc1-mm3/mm/memory.c
> ===================================================================
> --- linux-2.6.17-rc1-mm3.orig/mm/memory.c	2006-04-18 10:58:33.000000000 -0700
> +++ linux-2.6.17-rc1-mm3/mm/memory.c	2006-04-18 11:09:23.000000000 -0700
> @@ -434,7 +434,9 @@ copy_one_pte(struct mm_struct *dst_mm, s
>  	/* pte contains position in swap or file, so copy. */
>  	if (unlikely(!pte_present(pte))) {
>  		if (!pte_file(pte)) {
> -			swap_duplicate(pte_to_swp_entry(pte));
> +			swp_entry_t entry = pte_to_swp_entry(pte);
> +
> +			swap_duplicate(entry);
>  			/* make sure dst_mm is on swapoff's mmlist. */
>  			if (unlikely(list_empty(&dst_mm->mmlist))) {
>  				spin_lock(&mmlist_lock);
> @@ -443,6 +445,19 @@ copy_one_pte(struct mm_struct *dst_mm, s
>  						 &src_mm->mmlist);
>  				spin_unlock(&mmlist_lock);
>  			}
> +			if (is_migration_entry(entry) &&
> +					is_cow_mapping(vm_flags)) {
> +				page = migration_entry_to_page(entry);
> +
> +				/*
> +				 * COW mappings require pages in both parent
> +				*  and child to be set to read.
> +				 */
> +				entry = make_migration_entry(page,
> +	`					SWP_MIGRATION_READ);
> +				pte = swp_entry_to_pte(entry);
> +				set_pte_at(src_mm, addr, src_pte, pte);
> +			}
>  		}
>  		goto out_set_pte;
>  	}
> 

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

Re: Read/Write migration entries: Implement correct behavior in copy_one_pte

[Christoph Lameter]

On Wed, 19 Apr 2006, KAMEZAWA Hiroyuki wrote:

> > Note that this is again only a partial solution. mprotect() also has the
> > potential of changing the write status to read. 
> yes. in change_pte_range(). 
> 
> Note:
> fork() and mprotect() both requires mm->mmap_sem.
> So both of them is not problem when migration holds mm->mmap_sem.
> If we does lazy migration or memory hot removing or allows migration from
> another process, this will be problem.

Oh. We already allow migration from another process since the page may 
be mapped by multiple mm's. Page migration will then replace the ptes in 
*all* mm_structs that map this page with migration entries.

So we need a fix here.

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

Re: Read/Write migration entries: Implement correct behavior in copy_one_pte

[KAMEZAWA Hiroyuki]

On Tue, 18 Apr 2006 18:27:28 -0700 (PDT)
Christoph Lameter <clameter@sgi.com> wrote:

> On Wed, 19 Apr 2006, KAMEZAWA Hiroyuki wrote:
> 
> > > Note that this is again only a partial solution. mprotect() also has the
> > > potential of changing the write status to read. 
> > yes. in change_pte_range(). 
> > 
> > Note:
> > fork() and mprotect() both requires mm->mmap_sem.
> > So both of them is not problem when migration holds mm->mmap_sem.
> > If we does lazy migration or memory hot removing or allows migration from
> > another process, this will be problem.
> 
> Oh. We already allow migration from another process since the page may 
> be mapped by multiple mm's. Page migration will then replace the ptes in 
> *all* mm_structs that map this page with migration entries.
> 
> So we need a fix here.
> 
Ah.....yes. sorry.

In my understanding (and grep), read/write protection for anon pages 
can be changed under

- fork()
- mprotect()

all are known.

BTW, do we manage page table under move_vma() in right way ?

-Kame

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

Re: Read/Write migration entries: Implement correct behavior in copy_one_pte

[Christoph Lameter]

On Wed, 19 Apr 2006, KAMEZAWA Hiroyuki wrote:

> BTW, do we manage page table under move_vma() in right way ?

I had a look at it and it seems to be done the right way. The ptl locks
are taken and the vma information is setup before the move. 
remove_migration_ptes() will find the page both in the old and the new 
vma.

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

Read/Write migration entries: Make mprotect() convert write migration entries to read

[Christoph Lameter]

1. Introduce a new function make_migration_entry() to
   isolate common code between copy_pte_range and change_pte_range.

2. Modify change_pte_range() to check for a migration entry.
   If a write migration entry is found and there is a request for
   a READ permissions then change the migration entry.

I am a bit concerned about the check of newprot. Are there other
values than PAGE_READONLY that indicate read only access?

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

Index: linux-2.6.17-rc1-mm3/mm/memory.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/mm/memory.c	2006-04-18 11:09:23.252982000 -0700
+++ linux-2.6.17-rc1-mm3/mm/memory.c	2006-04-20 12:22:50.626800376 -0700
@@ -447,14 +447,11 @@
 			}
 			if (is_migration_entry(entry) &&
 					is_cow_mapping(vm_flags)) {
-				page = migration_entry_to_page(entry);
-
 				/*
 				 * COW mappings require pages in both parent
-				*  and child to be set to read.
+				 * and child to be set to read.
 				 */
-				entry = make_migration_entry(page,
-						SWP_MIGRATION_READ);
+				make_migration_entry_read(&entry);
 				pte = swp_entry_to_pte(entry);
 				set_pte_at(src_mm, addr, src_pte, pte);
 			}
Index: linux-2.6.17-rc1-mm3/mm/mprotect.c
===================================================================
--- linux-2.6.17-rc1-mm3.orig/mm/mprotect.c	2006-04-18 11:12:30.614603000 -0700
+++ linux-2.6.17-rc1-mm3/mm/mprotect.c	2006-04-20 12:17:03.771210036 -0700
@@ -19,6 +19,8 @@
 #include <linux/mempolicy.h>
 #include <linux/personality.h>
 #include <linux/syscalls.h>
+#include <linux/swap.h>
+#include <linux/swapops.h>
 
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
@@ -28,22 +30,35 @@
 static void change_pte_range(struct mm_struct *mm, pmd_t *pmd,
 		unsigned long addr, unsigned long end, pgprot_t newprot)
 {
-	pte_t *pte;
+	pte_t *pte, oldpte;
 	spinlock_t *ptl;
 
 	pte = pte_offset_map_lock(mm, pmd, addr, &ptl);
 	do {
-		if (pte_present(*pte)) {
+		oldpte = *pte;
+		if (pte_present(oldpte)) {
 			pte_t ptent;
 
 			/* Avoid an SMP race with hardware updated dirty/clean
 			 * bits by wiping the pte and then setting the new pte
 			 * into place.
 			 */
-			ptent = pte_modify(ptep_get_and_clear(mm, addr, pte), newprot);
+			ptent = pte_modify(ptep_get_and_clear(mm, addr, pte),
+								newprot);
 			set_pte_at(mm, addr, pte, ptent);
 			lazy_mmu_prot_update(ptent);
+		} else
+		if (!pte_file(oldpte) && pgprot_val(newprot) ==
+						 pgprot_val(PAGE_READONLY)) {
+			swp_entry_t entry = pte_to_swp_entry(oldpte);
+
+			if (is_write_migration_entry(entry)) {
+				make_migration_entry_read(&entry);
+				set_pte_at(mm, addr, pte,
+					swp_entry_to_pte(entry));
+			}
 		}
+
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 	pte_unmap_unlock(pte - 1, ptl);
 }
Index: linux-2.6.17-rc1-mm3/include/linux/swapops.h
===================================================================
--- linux-2.6.17-rc1-mm3.orig/include/linux/swapops.h	2006-04-18 10:58:33.675573000 -0700
+++ linux-2.6.17-rc1-mm3/include/linux/swapops.h	2006-04-20 12:00:29.279539838 -0700
@@ -98,6 +98,11 @@
 	return p;
 }
 
+static inline void make_migration_entry_read(swp_entry_t *entry)
+{
+	*entry = swp_entry(SWP_MIGRATION_READ, swp_offset(*entry));
+}
+
 extern void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
 					unsigned long address);
 #else
@@ -105,6 +110,7 @@
 #define make_migration_entry(page, write) swp_entry(0, 0)
 #define is_migration_entry(swp) 0
 #define migration_entry_to_page(swp) NULL
+static inline void make_migration_entry_read(entryp) { }
 static inline void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
 					 unsigned long address) { }
 

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