mm: tracking dirty pages -v9

The latest version of the tracking dirty pages patch-set. On request against -mm.

This version handles VM_PFNMAP vmas and the COW case of shared RO mappings.

follow_page() got a comment for being weird, but in the light of the set_page_dirty() call that can not yet be removed does something sane.

copy_one_pte() also does the right thing, although I wonder why it clears the dirty bit for children?

f_op->open() - sets a backing_dev_info
f_op->mmap() - modifies both vma->vm_flags and vma->vm_page_prot

Since our condition depends on both the backing_dev_info and vma->vm_flags it cannot set vma->vm_page_prot before f_op->mmap().

However this means that !VM_PFNMAP vmas that are shared writable but do not provide a f_op->nopage() and whos backing_dev_info does not have BDI_CAP_NO_ACCT_DIRTY, are left writable.

Peter

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

From: Peter Zijlstra <[email protected]>

People expressed the need to track dirty pages in shared mappings.

Linus outlined the general idea of doing that through making clean writable pages write-protected and taking the write fault.

This patch does exactly that, it makes pages in a shared writable mapping write-protected. On write-fault the pages are marked dirty and made writable. When the pages get synced with their backing store, the write-protection is re-instated.

It survives a simple test and shows the dirty pages in /proc/vmstat.

Changes in -v9

- respin against latest -mm.

Changes in -v8

- access_process_vm() and other force users of get_user_pages() can induce COW of read-only shared mappings.

Changes in -v7

- changed is_shared_writable() to exclude VM_PFNMAP'ed regions.
- Hugh's tiresome problem wasn't fully solved, now using the ugly duckling method.

Changes in -v6

- make page_mkclean_one() modify the pte more like change_pte_range() (suggested by Christoph Lameter)
- made is_shared_writable() take vm_flags, it now resembles is_cow_mapping().
- fixed the mprotect() bug (spotted by Hugh Dickins)
- hopefully fixed the tiresome issue of do_mmap_pgoff() trampling on driver specific vm_page_prot settings (spotted by Hugh Dickins)
- made a new version of the page_mkwrite() patch to go on top of all this. This so that Linus could merge this very early on in 2.6.18.

Changes in -v5

- rename page_wrprotect() to page_mkclean() (suggested by Nick Piggin)
- added comment to test_clear_page_dirty() (Andrew Morton)
- cleanup page_wrprotect() (Andrew Morton)
- renamed VM_SharedWritable() to is_shared_writable()
- fs/buffers.c try_to_free_buffers(): remove clear_page_dirty() from under ->private_lock. This seems to be save, since ->private_lock is used to serialize access to the buffers, not the page itself.
- rebased on top of David Howells' page_mkwrite() patch.

Changes in -v4:

- small cleanup as suggested by Christoph Lameter.

Changes in -v3:

- move set_page_dirty() outside pte lock (suggested by Christoph Lameter)

Changes in -v2:

- only wrprotect pages from dirty capable mappings. (Nick Piggin)
- move the writefault handling from do_wp_page() into handle_pte_fault(). (Nick Piggin)
- revert to the old install_page interface. (Nick Piggin)
- also clear the pte dirty bit when we make pages read-only again. (spotted by Rik van Riel)
- make page_wrprotect() return the number of reprotected ptes.

---

 fs/buffer.c          |    2 -
 include/linux/mm.h   |    6 ++++
 include/linux/rmap.h |    8 ++++++
 mm/memory.c          |   29 ++++++++++++++++++---
 mm/mmap.c            |   34 +++++++++++++++++++++----
 mm/mprotect.c        |    7 ++++-
 mm/page-writeback.c  |    9 ++++++
 mm/rmap.c            |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 149 insertions(+), 14 deletions(-)

Index: 2.6-mm/include/linux/mm.h
===================================================================
@@ -182,6 +182,12 @@ extern unsigned int kobjsize(const void 
 #define VM_SequentialReadHint(v)	((v)->vm_flags & VM_SEQ_READ)
 #define VM_RandomReadHint(v)		((v)->vm_flags & VM_RAND_READ)
 
+static inline int is_shared_writable(unsigned int flags)
+{
+	return (flags & (VM_SHARED|VM_WRITE|VM_PFNMAP)) ==
+		(VM_SHARED|VM_WRITE);
+}
+
 /*
  * mapping from the currently active vm_flags protection bits (the
  * low four bits) to a page protection mask..

Index: 2.6-mm/mm/memory.c
===================================================================
@@ -938,6 +938,12 @@ struct page *follow_page(struct vm_area_
 	pte = *ptep;
 	if (!pte_present(pte))
 		goto unlock;
+	/*
+	 * This is not fully correct in the light of trapping write faults
+	 * for writable shared mappings. However since we're going to mark
+	 * the page dirty anyway some few lines downward, we might as well
+	 * take the write fault now.
+	 */
 	if ((flags & FOLL_WRITE) && !pte_write(pte))
 		goto unlock;
 	page = vm_normal_page(vma, address, pte);

@@ -1458,13 +1464,14 @@ static int do_wp_page(struct mm_struct *
 {
 	struct page *old_page, *new_page;
 	pte_t entry;
-	int reuse, ret = VM_FAULT_MINOR;
+	int reuse = 0, ret = VM_FAULT_MINOR;
+	struct page *dirty_page = NULL;
 
 	old_page = vm_normal_page(vma, address, orig_pte);
 	if (!old_page)
 		goto gotten;
 
-	if (unlikely(vma->vm_flags & VM_SHARED)) {
+	if (unlikely(is_shared_writable(vma->vm_flags))) {
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to

@@ -1493,13 +1500,12 @@ static int do_wp_page(struct mm_struct *
 			if (!pte_same(*page_table, orig_pte))
 				goto unlock;
 		}
-
+		dirty_page = old_page;
+		get_page(dirty_page);
 		reuse = 1;
 	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
 		reuse = can_share_swap_page(old_page);
 		unlock_page(old_page);
-	} else {
-		reuse = 0;
 	}
 
 	if (reuse) {

@@ -1565,6 +1571,10 @@ gotten:
 		page_cache_release(old_page);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	if (old_page)

@@ -2097,6 +2107,7 @@ static int do_no_page(struct mm_struct *
 	unsigned int sequence = 0;
 	int ret = VM_FAULT_MINOR;
 	int anon = 0;
+	struct page *dirty_page = NULL;
 
 	pte_unmap(page_table);
 	BUG_ON(vma->vm_flags & VM_PFNMAP);

@@ -2191,6 +2202,10 @@ retry:
 		} else {
 			inc_mm_counter(mm, file_rss);
 			page_add_file_rmap(new_page);
+			if (write_access) {
+				dirty_page = new_page;
+				get_page(dirty_page);
+			}
 		}
 	} else {
 		/* One of our sibling threads was faster, back out. */

@@ -2203,6 +2218,10 @@ retry:
 	lazy_mmu_prot_update(entry);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	page_cache_release(new_page);

Index: 2.6-mm/mm/mmap.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/mount.h>
 #include <linux/mempolicy.h>
 #include <linux/rmap.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -888,6 +889,7 @@ unsigned long do_mmap_pgoff(struct file 
 	struct rb_node ** rb_link, * rb_parent;
 	int accountable = 1;
 	unsigned long charged = 0, reqprot = prot;
+	struct address_space *mapping = NULL;
 
 	if (file) {
 		if (is_file_hugepages(file))

@@ -1084,18 +1086,13 @@ munmap_back:
 		error = file->f_op->mmap(file, vma);
 		if (error)
 			goto unmap_and_free_vma;
+
 	} else if (vm_flags & VM_SHARED) {
 		error = shmem_zero_setup(vma);
 		if (error)
 			goto free_vma;
 	}
 
-	/* Don't make the VMA automatically writable if it's shared, but the
-	 * backer wishes to know when pages are first written to */
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
-		vma->vm_page_prot =
-			protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];
-
 	/* We set VM_ACCOUNT in a shared mapping's vm_flags, to inform
 	 * shmem_zero_setup (perhaps called through /dev/zero's ->mmap)
 	 * that memory reservation must be checked; but that reservation

@@ -1113,6 +1110,31 @@ munmap_back:
 	pgoff = vma->vm_pgoff;
 	vm_flags = vma->vm_flags;
 
+	/*
+	 * Tracking of dirty pages for shared writable mappings. Do this by
+	 * write protecting writable pages, and mark dirty in the write fault.
+	 *
+	 * Modify vma->vm_page_prot (the default protection for new pages)
+	 * to this effect.
+	 *
+	 * Cannot do before because the condition depends on:
+	 *  - backing_dev_info having the right capabilities
+	 *    (set by f_op->open())
+	 *  - vma->vm_flags being fully set
+	 *    (finished in f_op->mmap(), which could call remap_pfn_range())
+	 *
+	 *  Also, cannot reset vma->vm_page_prot from vma->vm_flags because
+	 *  f_op->mmap() can modify it.
+	 */
+	if (is_shared_writable(vm_flags) && vma->vm_file)
+		mapping = vma->vm_file->f_mapping;
+	if ((mapping && mapping_cap_account_dirty(mapping)) ||
+			(vma->vm_ops && vma->vm_ops->page_mkwrite))
+		vma->vm_page_prot =
+			__pgprot(pte_val
+				(pte_wrprotect
+				 (__pte(pgprot_val(vma->vm_page_prot)))));
+
 	if (!file || !vma_merge(mm, prev, addr, vma->vm_end,
 			vma->vm_flags, NULL, file, pgoff, vma_policy(vma))) {
 		file = vma->vm_file;

Index: 2.6-mm/mm/mprotect.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/syscalls.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+#include <linux/backing-dev.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>

@@ -124,6 +125,7 @@ mprotect_fixup(struct vm_area_struct *vm
 	long nrpages = (end - start) >> PAGE_SHIFT;
 	unsigned long charged = 0;
 	unsigned int mask;
+	struct address_space *mapping = NULL;
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;

@@ -179,7 +181,10 @@ success:
 	/* Don't make the VMA automatically writable if it's shared, but the
 	 * backer wishes to know when pages are first written to */
 	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+	if (is_shared_writable(newflags) && vma->vm_file)
+		mapping = vma->vm_file->f_mapping;
+	if ((mapping && mapping_cap_account_dirty(mapping)) ||
+			(vma->vm_ops && vma->vm_ops->page_mkwrite))
 		mask &= ~VM_SHARED;
 
 	newprot = protection_map[newflags & mask];

Index: 2.6-mm/mm/page-writeback.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/sysctl.h>
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
+#include <linux/rmap.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -540,7 +541,7 @@ int do_writepages(struct address_space *
 		return 0;
 	wbc->for_writepages = 1;
 	if (mapping->a_ops->writepages)
-		ret =  mapping->a_ops->writepages(mapping, wbc);
+		ret = mapping->a_ops->writepages(mapping, wbc);
 	else
 		ret = generic_writepages(mapping, wbc);
 	wbc->for_writepages = 0;

@@ -704,6 +705,11 @@ int test_clear_page_dirty(struct page *p
 			if (mapping_cap_account_dirty(mapping))
 				__dec_zone_page_state(page, NR_DIRTY);
 			write_unlock_irqrestore(&mapping->tree_lock, flags);
+			/*
+			 * We can continue to use `mapping' here because the
+			 * page is locked, which pins the address_space
+			 */
+			page_mkclean(page);
 			return 1;
 		}
 		write_unlock_irqrestore(&mapping->tree_lock, flags);

@@ -733,6 +739,7 @@ int clear_page_dirty_for_io(struct page 
 
 	if (mapping) {
 		if (TestClearPageDirty(page)) {
+			page_mkclean(page);
 			if (mapping_cap_account_dirty(mapping))
 				dec_zone_page_state(page, NR_DIRTY);
 			return 1;

Index: 2.6-mm/include/linux/rmap.h
===================================================================
@@ -103,6 +103,14 @@ pte_t *page_check_address(struct page *,
  */
 unsigned long page_address_in_vma(struct page *, struct vm_area_struct *);
 
+/*
+ * Cleans the PTEs of shared mappings.
+ * (and since clean PTEs should also be readonly, write protects them too)
+ *
+ * returns the number of cleaned PTEs.
+ */
+int page_mkclean(struct page *);
+
 #else	/* !CONFIG_MMU */
 
 #define anon_vma_init()		do {} while (0)

Index: 2.6-mm/fs/buffer.c
===================================================================
@@ -2985,6 +2985,7 @@ int try_to_free_buffers(struct page *pag
 
 	spin_lock(&mapping->private_lock);
 	ret = drop_buffers(page, &buffers_to_free);
+	spin_unlock(&mapping->private_lock);
 	if (ret) {
 		/*
 		 * If the filesystem writes its buffers by hand (eg ext3)

@@ -2996,7 +2997,6 @@ int try_to_free_buffers(struct page *pag
 		 */
 		clear_page_dirty(page);
 	}
-	spin_unlock(&mapping->private_lock);
 out:
 	if (buffers_to_free) {
 		struct buffer_head *bh = buffers_to_free;

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

On Mon, 19 Jun 2006 19:52:53 +0200
Peter Zijlstra <[email protected]> wrote:

Could we have a changelog for this patch? A list of what-i-changed-since-last-time bullet points is uninteresting and unuseful for the permanent record. Think in terms of some random person in Barcelona who is trying to work upon your code two years hence.

The patch should be accompanied by a standalone description of what it does, why it does it and how it does it, thanks.

	if (unlikely((vma->vm_flags & (VM_SHARED|VM_WRITE)) ==
			(VM_SHARED|VM_WRITE))) {

Tests did show that there was no performance regression for the usual tests. That is to be expected since the patch should only modify the behavior of shared writable mapping. The use of those is rare in typical benchmarks.

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

Of course. In this case one should prepare an artificial microbenchmark so we can understand the worst case.

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

	pgprot_t prot_shared = protection_map[vm_flags & 
		(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
	pgprot_t prot_priv = protection_map[vm_flags & 
		(VM_READ|VM_WRITE|VM_EXEC)];

	typeof(pgprot_val(prot_shared)) mask = 
		~(pgprot_val(prot_shared) ^ pgprot_val(prot_priv));

	pgprot_val(vma->vm_page_prot) &= mask;
	pgprot_val(vma->vm_page_prot) |= 
		(pgprot_val(prot_priv) & mask);

its more readable, but barely so.

BTW, is there a difference between:
(VM_READ|VM_WRITE|VM_EXEC)
and
(VM_READ|VM_EXEC|VM_SHARED)
in this context?

Or I can make it a generic arch specific function and override for i386 and x86-64. That way I can also cleanup drivers/char/drm/drm_vm.c where I found this thing.

include/asm-generic/pgtable.h

#ifndef __HAVE_ARCH_PGPROT_WRPROTECT
#define pgprot_wrprotect(prot) \

({ (prot) = __pgprot(pte_val \
		(pte_wrprotect	\
		(__pte(pgprot_val(prot))))) \
})

#endif

include/asm-{i386,x86-64}/pgtable.h

#define pgprot_wrprotect(prot) ({ pgprot_val(prot) &= ~_PAGE_RW; }) #define __HAVE_ARCH_PGPROT_WRPROTECT

I can go through some other archs and see what I can do.

Hmm, now that I look at this, might give a include-dependency problem. Awell, thoughts, comments?

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

Just note vma->vm_page_prot before calling the ->mmap and check after. If the driver has messed with it at all, it's not a mapping we want to apply dirty capping to anyway. If it's unchanged, and the other tests pass, go ahead and reset readonly vm_page_prot via protection_map[].

This is of course a hack, as is the pgprotting code from drm. The correct solution would be to set the proper backing_dev_info in a number of drivers - we were too lazy when setting the default in the first place; but even if we caught all the intree drivers, we'd miss out-of-tree ones and suffer unnecessary pain from them. So for now a hack is necessary, and I haven't thought of a better.

Other comments to follow...

Hugh

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

*sight*, thats it, explicit it will be :-)

I have no idea either, I reread this part earlier today and found it one big brainfart. It does indeed seem to do the right thing.

I don't think the VM_PFNMAP is needed here, but it doesn't hurt either. Like said, I'll do explicits from now on.

:-) uhu..

useless information indeed, a remnant from old times when I placed the vm_page_prot modification between the two calls, shall remove.

Made it one big if stmt, perhaps too big, we'll see.

Yes, that was a very good suggestion and has already been incorporated, thanks.

VM_INSERTPAGE thou shall have.

Sad situation but true.

Well, no, not anymore. I thought to make it actually do what its name said it does: clean the page's PTEs (I am even pondering about implementing the anonymous branch).

In that light, its now called for each page.

New patch will follow shortly since I can't seem to sleep anyway...

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

Preview of the goodness,

I'll repost the whole thing tomorrow after I've updated the other patches, esp. the msync one. I seem to be too tired to make any sense out of that atm.

(PS, 2.6.17-mm1 UML doesn't seem to boot)

-------------------------------------------------------

From: Peter Zijlstra <[email protected]>

Tracking of dirty pages in shared writeable mmap()s.

The idea is simple: write protect clean shared writeable pages, catch the write-fault, make writeable and set dirty. On page write-back clean all the PTE dirty bits and write protect them once again.

The implementation is a tad harder, mainly because the default backing_dev_info capabilities were too loosely maintained. Hence it is not enough to test the backing_dev_info for cap_account_dirty.

The current heuristic is as follows, a VMA is eligible when:
- its shared writeable

    (vm_flags & (VM_WRITE|VM_SHARED)) == (VM_WRITE|VM_SHARED)
 - it is not a PFN mapping
    (vm_flags & VM_PFNMAP) == 0
 - the backing_dev_info is cap_account_dirty
    mapping_cap_account_dirty(vma->vm_file->f_mapping)

- f_op->mmap() didn't change the default page protection

NOTE: the last rule is only checked in do_mmap_pgoff(), other code- paths assume they will not be reached in that case.

Page from remap_pfn_range() are explicitly excluded because their COW semantics are already horrid enough (see vm_normal_page() in do_wp_page()) and because they don't have a backing store anyway.

copy_one_pte() cleans child PTEs, not wrong in the current context, but why?

mprotect() is taught about the new behaviour as well.

Cleaning the pages on write-back is done with page_mkclean() a new rmap call. It can be called on any page, but is currently only implemented for mapped pages (does it make sense to also implement anon pages?), if the page is found the be of a trackable VMA it will also wrprotect the PTE.

Finally, in fs/buffers.c:try_to_free_buffers(); remove clear_page_dirty() from under ->private_lock. This seems to be save, since ->private_lock is used to serialize access to the buffers, not the page itself. This is needed because clear_page_dirty() will call into page_mkclean() and would thereby violate locking order.

---
Changes in -v10

- 2.6.17-mm1
- Drop the ugly duckling pgprotting, Hugh suggested resetting the vma->vm_page_prot when f_op->mmap() didn't modify it. If it were modified we're not interested anyway.
- abandon is_shared_writable() because its actually spelled writeable and it didn't actually mean that any more.
- Comments all round.

Changes in -v9

- respin against latest -mm.

Changes in -v8

- access_process_vm() and other force users of get_user_pages() can induce COW of read-only shared mappings.

Changes in -v7

- changed is_shared_writable() to exclude VM_PFNMAP'ed regions.
- Hugh's tiresome problem wasn't fully solved, now using the ugly duckling method.

Changes in -v6

- make page_mkclean_one() modify the pte more like change_pte_range() (suggested by Christoph Lameter)
- made is_shared_writable() take vm_flags, it now resembles is_cow_mapping().
- fixed the mprotect() bug (spotted by Hugh Dickins)
- hopefully fixed the tiresome issue of do_mmap_pgoff() trampling on driver specific vm_page_prot settings (spotted by Hugh Dickins)
- made a new version of the page_mkwrite() patch to go on top of all this. This so that Linus could merge this very early on in 2.6.18.

Changes in -v5

- rename page_wrprotect() to page_mkclean() (suggested by Nick Piggin)
- added comment to test_clear_page_dirty() (Andrew Morton)
- cleanup page_wrprotect() (Andrew Morton)
- renamed VM_SharedWritable() to is_shared_writable()
- fs/buffers.c try_to_free_buffers(): remove clear_page_dirty() from under ->private_lock. This seems to be save, since ->private_lock is used to serialize access to the buffers, not the page itself.
- rebased on top of David Howells' page_mkwrite() patch.

Changes in -v4:

- small cleanup as suggested by Christoph Lameter.

Changes in -v3:

- move set_page_dirty() outside pte lock (suggested by Christoph Lameter)

Changes in -v2:

- only wrprotect pages from dirty capable mappings. (Nick Piggin)
- move the writefault handling from do_wp_page() into handle_pte_fault(). (Nick Piggin)
- revert to the old install_page interface. (Nick Piggin)
- also clear the pte dirty bit when we make pages read-only again. (spotted by Rik van Riel)
- make page_wrprotect() return the number of reprotected ptes.

 fs/buffer.c                 |    2 -
 include/linux/backing-dev.h |   13 +++++---
 include/linux/rmap.h        |    8 +++++
 mm/memory.c                 |   29 ++++++++++++++----
 mm/mmap.c                   |   40 ++++++++++++++++++++-----
 mm/mprotect.c               |   17 ++++++++--
 mm/page-writeback.c         |    9 +++++
 mm/rmap.c                   |   69 ++++++++++++++++++++++++++++++++++++++++++++
 8 files changed, 163 insertions(+), 24 deletions(-)

Index: 2.6-mm/mm/memory.c
===================================================================
@@ -1458,14 +1458,19 @@ static int do_wp_page(struct mm_struct *
 {
 	struct page *old_page, *new_page;
 	pte_t entry;
-	int reuse, ret = VM_FAULT_MINOR;
+	int reuse = 0, ret = VM_FAULT_MINOR;
+	struct page *dirty_page = NULL;
 
 	old_page = vm_normal_page(vma, address, orig_pte);
 	if (!old_page)
 		goto gotten;
 
-	if (unlikely((vma->vm_flags & (VM_SHARED|VM_WRITE)) ==
-				(VM_SHARED|VM_WRITE))) {
+	/*
+	 * Only catch write-faults on shared writable pages, read-only
+	 * shared pages can get COWed by get_user_pages(.write=1, .force=1).
+	 */
+	if (unlikely(((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
+					(VM_WRITE|VM_SHARED)))) {
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to

@@ -1494,13 +1499,12 @@ static int do_wp_page(struct mm_struct *
 			if (!pte_same(*page_table, orig_pte))
 				goto unlock;
 		}
-
+		dirty_page = old_page;
+		get_page(dirty_page);
 		reuse = 1;
 	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
 		reuse = can_share_swap_page(old_page);
 		unlock_page(old_page);
-	} else {
-		reuse = 0;
 	}
 
 	if (reuse) {

@@ -1566,6 +1570,10 @@ gotten:
 		page_cache_release(old_page);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	if (old_page)

@@ -2098,6 +2106,7 @@ static int do_no_page(struct mm_struct *
 	unsigned int sequence = 0;
 	int ret = VM_FAULT_MINOR;
 	int anon = 0;
+	struct page *dirty_page = NULL;
 
 	pte_unmap(page_table);
 	BUG_ON(vma->vm_flags & VM_PFNMAP);

@@ -2192,6 +2201,10 @@ retry:
 		} else {
 			inc_mm_counter(mm, file_rss);
 			page_add_file_rmap(new_page);
+			if (write_access) {
+				dirty_page = new_page;
+				get_page(dirty_page);
+			}
 		}
 	} else {
 		/* One of our sibling threads was faster, back out. */

@@ -2204,6 +2217,10 @@ retry:
 	lazy_mmu_prot_update(entry);
 unlock:
 	pte_unmap_unlock(page_table, ptl);
+	if (dirty_page) {
+		set_page_dirty(dirty_page);
+		put_page(dirty_page);
+	}
 	return ret;
 oom:
 	page_cache_release(new_page);

Index: 2.6-mm/mm/mmap.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/mount.h>
 #include <linux/mempolicy.h>
 #include <linux/rmap.h>
+#include <linux/backing-dev.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -888,6 +889,7 @@ unsigned long do_mmap_pgoff(struct file 
 	struct rb_node ** rb_link, * rb_parent;
 	int accountable = 1;
 	unsigned long charged = 0, reqprot = prot;
+	pgprot_t vm_page_prot;
 
 	if (file) {
 		if (is_file_hugepages(file))

@@ -1065,8 +1067,8 @@ munmap_back:
 	vma->vm_start = addr;
 	vma->vm_end = addr + len;
 	vma->vm_flags = vm_flags;
-	vma->vm_page_prot = protection_map[vm_flags &
-				(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
+	vma->vm_page_prot = vm_page_prot = protection_map[vm_flags &
+		(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)];
 	vma->vm_pgoff = pgoff;
 
 	if (file) {

@@ -1090,12 +1092,6 @@ munmap_back:
 			goto free_vma;
 	}
 
-	/* Don't make the VMA automatically writable if it's shared, but the
-	 * backer wishes to know when pages are first written to */
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
-		vma->vm_page_prot =
-			protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];
-
 	/* We set VM_ACCOUNT in a shared mapping's vm_flags, to inform
 	 * shmem_zero_setup (perhaps called through /dev/zero's ->mmap)
 	 * that memory reservation must be checked; but that reservation

@@ -1113,6 +1109,34 @@ munmap_back:
 	pgoff = vma->vm_pgoff;
 	vm_flags = vma->vm_flags;
 
+	/*
+	 * Tracking of dirty pages for shared writable mappings. Do this by
+	 * write protecting writable pages, and mark dirty in the write fault.
+	 *
+	 * Cannot do before because the condition depends on:
+	 *  - backing_dev_info having the right capabilities
+	 *  - vma->vm_flags being fully set
+	 *    (finished in f_op->mmap(), which could call remap_pfn_range())
+	 *
+	 * If f_op->mmap() changed vma->vm_page_prot its a funny mapping
+	 * and we won't touch it.
+	 * NOTE: in a perfect world backing_dev_info would have the proper
+	 * capabilities.
+	 *
+	 * OR
+	 *
+	 * Don't make the VMA automatically writable if it's shared, but the
+	 * backer wishes to know when pages are first written to.
+	 */
+	if ((pgprot_val(vma->vm_page_prot) == pgprot_val(vm_page_prot) &&
+	     ((vm_flags & (VM_WRITE|VM_SHARED|VM_PFNMAP|VM_INSERTPAGE)) ==
+			  (VM_WRITE|VM_SHARED)) &&
+	     vma->vm_file && vma->vm_file->f_mapping &&
+	     mapping_cap_account_dirty(vma->vm_file->f_mapping)) ||
+	    (vma->vm_ops && vma->vm_ops->page_mkwrite))
+		vma->vm_page_prot =
+			protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];
+
 	if (!file || !vma_merge(mm, prev, addr, vma->vm_end,
 			vma->vm_flags, NULL, file, pgoff, vma_policy(vma))) {
 		file = vma->vm_file;

Index: 2.6-mm/mm/mprotect.c
===================================================================
@@ -21,6 +21,7 @@
 #include <linux/syscalls.h>
 #include <linux/swap.h>
 #include <linux/swapops.h>
+#include <linux/backing-dev.h>
 #include <asm/uaccess.h>
 #include <asm/pgtable.h>
 #include <asm/cacheflush.h>

@@ -176,10 +177,20 @@ mprotect_fixup(struct vm_area_struct *vm
 	}
 
 success:
-	/* Don't make the VMA automatically writable if it's shared, but the
-	 * backer wishes to know when pages are first written to */
+	/*
+	 * Tracking of dirty pages logic (see comment in do_mmap_pgoff)
+	 *
+	 * OR
+	 *
+	 * Don't make the VMA automatically writable if it's shared, but the
+	 * backer wishes to know when pages are first written to.
+	 */
 	mask = VM_READ|VM_WRITE|VM_EXEC|VM_SHARED;
-	if (vma->vm_ops && vma->vm_ops->page_mkwrite)
+	if ((((newflags & (VM_WRITE|VM_SHARED|VM_PFNMAP|VM_INSERTPAGE)) ==
+			  (VM_WRITE|VM_SHARED)) &&
+	     vma->vm_file && vma->vm_file->f_mapping &&
+	     mapping_cap_account_dirty(vma->vm_file->f_mapping)) ||
+	    (vma->vm_ops && vma->vm_ops->page_mkwrite))
 		mask &= ~VM_SHARED;
 
 	newprot = protection_map[newflags & mask];

Index: 2.6-mm/mm/page-writeback.c
===================================================================
@@ -29,6 +29,7 @@
 #include <linux/sysctl.h>
 #include <linux/cpu.h>
 #include <linux/syscalls.h>
+#include <linux/rmap.h>
 
 /*
  * The maximum number of pages to writeout in a single bdflush/kupdate

@@ -566,7 +567,7 @@ int do_writepages(struct address_space *
 		return 0;
 	wbc->for_writepages = 1;
 	if (mapping->a_ops->writepages)
-		ret =  mapping->a_ops->writepages(mapping, wbc);
+		ret = mapping->a_ops->writepages(mapping, wbc);
 	else
 		ret = generic_writepages(mapping, wbc);
 	wbc->for_writepages = 0;

@@ -728,6 +729,11 @@ int test_clear_page_dirty(struct page *p
 						page_index(page),
 						PAGECACHE_TAG_DIRTY);
 			write_unlock_irqrestore(&mapping->tree_lock, flags);
+			/*
+			 * We can continue to use `mapping' here because the
+			 * page is locked, which pins the address_space
+			 */
+			page_mkclean(page);
 			if (mapping_cap_account_dirty(mapping))
 				dec_page_state(nr_dirty);
 			return 1;

@@ -759,6 +765,7 @@ int clear_page_dirty_for_io(struct page 
 
 	if (mapping) {
 		if (TestClearPageDirty(page)) {
+			page_mkclean(page);
 			if (mapping_cap_account_dirty(mapping))
 				dec_page_state(nr_dirty);
 			return 1;

Index: 2.6-mm/include/linux/rmap.h
===================================================================
@@ -103,6 +103,14 @@ pte_t *page_check_address(struct page *,
  */
 unsigned long page_address_in_vma(struct page *, struct vm_area_struct *);
 
+/*
+ * Cleans the PTEs of shared mappings.
+ * (and since clean PTEs should also be readonly, write protects them too)
+ *
+ * returns the number of cleaned PTEs.
+ */
+int page_mkclean(struct page *);
+
 #else	/* !CONFIG_MMU */
 
 #define anon_vma_init()		do {} while (0)

Index: 2.6-mm/fs/buffer.c
===================================================================
@@ -2983,6 +2983,7 @@ int try_to_free_buffers(struct page *pag
 
 	spin_lock(&mapping->private_lock);
 	ret = drop_buffers(page, &buffers_to_free);
+	spin_unlock(&mapping->private_lock);
 	if (ret) {
 		/*
 		 * If the filesystem writes its buffers by hand (eg ext3)

@@ -2994,7 +2995,6 @@ int try_to_free_buffers(struct page *pag
 		 */
 		clear_page_dirty(page);
 	}
-	spin_unlock(&mapping->private_lock);
 out:
 	if (buffers_to_free) {
 		struct buffer_head *bh = buffers_to_free;

Index: 2.6-mm/include/linux/backing-dev.h
===================================================================
@@ -97,11 +97,14 @@ static inline int bdi_rw_congested(struc
 #define bdi_cap_account_dirty(bdi) \
 	(!((bdi)->capabilities & BDI_CAP_NO_ACCT_DIRTY))
 
-#define mapping_cap_writeback_dirty(mapping) \
-	bdi_cap_writeback_dirty((mapping)->backing_dev_info)
-
-#define mapping_cap_account_dirty(mapping) \
-	bdi_cap_account_dirty((mapping)->backing_dev_info)
+#define mapping_cap_writeback_dirty(__mapping)				\
+({	typeof(__mapping) mapping = (__mapping);			\
+	(mapping && bdi_cap_writeback_dirty(mapping->backing_dev_info));\
+})
 
+#define mapping_cap_account_dirty(__mapping)				\
+({	typeof(__mapping) mapping = (__mapping);			\
+  	(mapping && bdi_cap_account_dirty(mapping->backing_dev_info));	\
+})
 
 #endif		/* _LINUX_BACKING_DEV_H */

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

I don't get that far - it doesn't build for me. It dies in klibc thusly:

gcc -Wp,-MD,usr/klibc/syscalls/.typesize.o.d -nostdinc -isystem /usr/lib/gcc/i386-redhat-linux/4.1.1/include -I/home/jdike/linux/2.6/test/linux-2.6.17/usr/include/arch/i386 -Iusr/include/arch/i386 -I/home/jdike/linux/2.6/test/linux-2.6.17/usr/include/bits32 -Iusr/include/bits32 -I/home/jdike/linux/2.6/test/linux-2.6.17/obj/usr/klibc/../include -I/home/jdike/linux/2.6/test/linux-2.6.17/usr/include -Iusr/include -I/home/jdike/linux/2.6/test/linux-2.6.17/include -I/home/jdike/linux/2.6/test/linux-2.6.17/include2 -Iinclude2 -I/home/jdike/linux/2.6/test/linux-2.6.17/include -Iinclude -I/home/jdike/linux/2.6/test/linux-2.6.17/include -D__KLIBC__=1 -D__KLIBC_MINOR__=4 -D_BITSIZE=32 -m32 -march=i386 -Os -g -fomit-frame-pointer -falign-functions=0 -falign-jumps=0 -falign-loops=0 -W -Wall -Wno-sign-compare -Wno-unused-parameter -c -o usr/klibc/syscalls/typesize.o usr/klibc/syscalls/typesize.c
usr/klibc/syscalls/typesize.c:1:23: error: syscommon.h: No such file or directory
usr/klibc/syscalls/typesize.c:5: error: '__u32' undeclared here (not in a function)
usr/klibc/syscalls/typesize.c:9: error: expected ')' before 'gid_t'
usr/klibc/syscalls/typesize.c:9: warning: type defaults to 'int' in declaration of 'type name'
usr/klibc/syscalls/typesize.c:10: error: expected ')' before 'sigset_t'
usr/klibc/syscalls/typesize.c:10: warning: type defaults to 'int' in declaration of 'type name'
usr/klibc/syscalls/typesize.c:21: error: 'dev_t' undeclared here (not in a function)
usr/klibc/syscalls/typesize.c:22: error: 'fd_set' undeclared here (not in a function)
usr/klibc/syscalls/typesize.c:22: error: expected expression before ')' token

syscommon.h is in usr/klibc/syscalls, but I don't see a -I pointing there.

Jeff

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

That's probably a parallel kbuild race. Type `make' again ;)

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

Nope, it's extremely consistent, including with a non-parallel build.

Jeff

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

No, it's not. It's a problem with O=, apparently; this patch fixes it:

http://www.kernel.org/git/?p=linux/kernel/git/hpa/linux-2.6-klibc.git;a=commitdiff;h=4e51186fb663b57ac7c53517947510d2e1e9de01;hp=79317ba49e3f83d40f37b59fcdd5bd7c7635ee32

-hpa

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

That works, thanks!

Back to the original problem - 2.6.17-mm1 UML not booting. If you add stderr=1 to the command line, you'll see this:

timer_init : request_irq failed - errno = 38
NET: Registered protocol family 2
irq 0, desc: 081debe0, depth: 0, count: 0, unhandled: 0 ->handle_irq(): 0808af80, handle_bad_irq+0x0/0x1b7 ->chip(): 081d9320, 0x81d9320
->action(): 00000000
IRQ_NOPROBE set
unexpected IRQ 00
BUG: failure at include2/asm/hardirq.h:22/ack_bad_irq()! Kernel panic - not syncing: BUG!

which means that the genirq stuff needs UML work, which I was working on anyway because UML could already be made to crash like this. Except now, it always does.

Jeff

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

Do people agree about this thing? If we want it in 2.6.18, we should merge this soon. I'd prefer to not leave something like this to be a last-minute thing before the merge window closes, and I get the feeling that we're getting to where this should just go in sooner rather than later.

Comments? Hugh, does the last version address all your concerns?

Linus

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

Not even looked at the preview yet, but as far as mechanism goes, I'm sure it won't be worse than a few fixups away from good.

However, I've never understood why it should be fasttracked into 2.6.18: we usually let such patchsets cook for a cycle in -mm. 2.6.N-rc can get wider exposure than 2.6.(N-1)-mm, reveal problems missed all the while in -mm, but a cycle in -mm is still worthwhile.

My pathetically slow responses have hindered Peter's good work, yes, but I don't think they've affected the overall appropriate timing.

Is there any particular reason why 2.6.18 rather than 2.6.19 be the release that fixes this issue that's been around forever?

And have we even seen stats for it yet? We know that it shouldn't affect the vast majority of loads (not mapping shared writable), but it won't be fixing any problem on them either; and we've had reports that it does fix the issue, but at what perf cost? (I may have missed)

Several people also have doubts as to whether it's right to be focussing just on shared writable here, whether the private also needs tweaking. I'm undecided. Can be considered a separate issue, but a cycle in -mm would help settle that question too.

But if you want to push for 2.6.18, I won't be aggrieved.

Hugh

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

Sounds good.

I do not think that statistics are that important. One of the primary advantages is that this fixes up a way to deadlock the machine through dirtying too many pages. The other side effect is that dirty page writeout can begin before an application terminates. We have had cases where dirty memory was sitting for weeks in a machine because the process did not terminate. These are major VM issues that need a resolution.

Also Peter has made the tracking configurable. So there is a way to switch it off if it is harmful for some situations.

You mean anonymous pages? Anonymous pages are always dirty unless you consider swap and we currently do not take account of dirty anonymous pages. With swap we already have performance problems and maybe there are additional issues to fix in that area. But these are secondary.

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

Oh?

I intent to make swap over NFS work next.

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

mapping_cap_account_dirty is not a way for a fs to switch this on and off?

I am still a bit unclear on what you mean by "work." The only issue may be to consider the amount of swap pages about to be written out for write throttling.

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

I had assumed this was a sick joke. Please tell me people aren't really swapping over NFS. That's *insane*.

M.

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

Hey, I think it even used to be common. I think some NCR X client did basically exactly that, with _no_ local disk at all.

Linus

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

Doesn't it work already? Is there some throttling that doesn't work?

Linus

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

I do not know how 'bad' the situation is now that we have the dirty page tracking stuff, still need to create a test environment.

But the general idea is that its broken because the ACK from writeout can be delayed and the remaining free memory taken by other incomming network packets.

Until I have a test setup and some reproducable deadlocks I cannot say more I'm afraid.

Peter

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

That is already taken care of by nr_unstable which is considered for write throttling.

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

With the current code it definitly doesn't. The swap code calls ->bmap to do block mappings at swapon time and then uses bios directly. This obviously can't work on anything but blockdevice-based filesystems.

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

Well, I've got two reasons to want to fast-track it:

- it's exactly what I wanted to see, so I'm obviously personally happy with the patch

- the main _real_ issues I'd expect to surface are some subtle performance issues when the kernel knows about more dirty pages, and the dirty limit is thus _effectively_ different (never mind even the page dirtying throttling itself - just the fact that we count the dirty pages will mean that we'll see different throttling behaviour for regular writes too in the presense of dirty)

Now, the first one is admittedly purely personal, but the second one boils down to the fact that I think we want people to use it in order to find these problems, and I suspect the -mm users are very uniform: it's probably almost exclusively (kernel) developers rather than "normal users".

No, I don't thinkthat has been the problem. I think the patches have improved from the feedback from you and others, I just think we're quite possible past the point where we simply would be better off with testing than with arguing from a source perspective.

But maybe I'm just biased.

My main worry has always been the effects of this on some strange load, not the stability itself.

_Exactly_. This is why I think earlier rather than later is better.

Sitting in -mm won't get us any new unexpected load cases - only more of the same that hasn't shown any huge flags per se (although the dirty limit discussion clearly shows people are at least thinking about it).

Linus

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

one options it to ask the distributions to put this into their more experimental kernels for a bit to give it a broader exposure... it's still a bit small but at least broader than "kernel developers"...

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

Heh, IIRC you rejected the idea of doing a fault on dirtying for performance reasons, during the discussion of VM deadlocks in FUSE.

Anyway, I'm more than happy, since David and Peter basically solved the problem, so shared writable mappings should now be possible to do.

Miklos

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

(You've probably noticed that use of mprotect on problem vmas is liable to remove the cache bits. There have been occasional complaints about that, and perhaps a patch to fix it. But that's long-standing behaviour, so not something you need worry about:
we've only had to worry about a new change in behaviour in mmap.)

Dates back to Linux 2.2 if not earlier. I think the idea is that the parent has its pte dirty, and that's enough to mark the page as dirty; the child is unlikely to dirty the page further itself, and a second instance of dirty pte for that page may entail some overhead. What overhead would vary from release to release, I've not thought through what the current saving would be in 2.6.17, probably very little. But you can imagine there might have been some release which would write out the page each time it found pte dirty, in which case good reason to clear it there. But I'm guessing, it's from before my time.

	if (PageAnon(old_page)) {
		...
	} else if (shared writable vma) {
		...
	}

Oops, on the extra parentheses. I exchanged the order because that is more consistent with all the other sites.

Hmm, I'll investigate this and maybe write a follow-up patch if you or someone else doesn't beaten me to it.

Yeah, had noticed, but had assumed that since it was a user action they'd know better than to call it on dubious mappings.

However if you say there is demand; I've been through all the arch's to verify the use of that most ugly drm construct, and whipping up an arch function like pgprot_modify() that would fold a pgprot_t and protection_map[] argument together seems quite doable.

I'll keep that in mind for a future cleanup.

It's a real beauty isn't it :-)

I meant to remove the mapping_cap_account_dirty() changes, so as not to burden all call sites with the extra conditional. This one extra check in here does not make the difference between beauty and beast.

Done

You're right, added back under the wing of mapping_cap_account_dirty().

But I'll leave page_mkclean() as callable without, that way its more true to its name.

Peter

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

Since Hugh pointed that out..

It really would be nice to just encapsulate that as an inline function of its own, and move the comment at the top of it to be at the top of the inline function.

Just make it something like

	/*
	 * Some shared mappigns will want the pages marked read-only
	 * to track write events. If so, we'll downgrade vm_page_prot
	 * to the private version (using protection_map[] without the
	 * VM_SHARED bit).
	 */
	static inline int vma_wants_writenotify(struct vm_area_struct *vma)
	{
		unsigned int vm_flags = vma->vm_flags;

		/* If it was private or non-writable, the write bit is already clear */
		if ((vm_flags & (VM_SHARED | VM_WRITE)) != ((VM_SHARED | VM_WRITE))
			return 0;

		/* The open routine did something to the protections already? */
		if (pgprot_val(vma->vm_page_prot) !=
		   pgprot_val(protection_map[vm_flags & (VM_SHARED|VM_READ|VM_WRITE|VM_EXEC)]))
			return 0;

		/* The backer wishes to know when pages are first written to? */
		if (vma->vm_ops && vma->vm_ops->page_mkwrite)
			return 1;

		/* Specialty mapping? */
		if (vm_flags & (VM_PFNMAP|VM_INSERTPAGE))
			return 0;

		/* Can the mapping track the dirty pages? */
		return vma->vm_file && vma->vm_file->f_mapping &&
			mapping_cap_account_dirty(vma->vm_file->f_mapping);
	}

(And no, I didn't make sure to test that it gives the same answer as your version, it's just a more readable version of what I think your version tests ;)

And then just use it with

	if (vma_wants_writenotify(vma))
		vma->vm_page_prot = protection_map[vm_flags & (VM_READ|VM_WRITE|VM_EXEC)];

which would appear to be more readable.

Yeah, the compiler may do worse. Or it may not. It usually pays to try to write code more readably, sometimes the compiler ends up understanding it better too ;)

Linus

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

Dang, I just send out -v11, awell such is life. I'll see what I can do. Preferably mprotect can reuse that same function.

Will repost a new 1/5 shortly.

Peter

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

How about something like this? This should make all anonymous write faults do as before the page_mkwrite patch.

As for copy_one_pte(), I'm not sure what you meant, shared writable anonymous pages need not be write protected as far as I can see. If you meant to say, anonymous or file-backed, then copy_one_pte() still does the right thing. If the source is untouched/clean it will still be wrprotected and this state will be copied, if its dirtied and made writeable we don't need the notification anymore anyway and hence the regular copy is still correct.

Peter

---
 mm/memory.c |   19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Index: linux-2.6-dirty/mm/memory.c
===================================================================
@@ -1466,11 +1466,21 @@ static int do_wp_page(struct mm_struct *
 		goto gotten;
 
 	/*
-	 * Only catch write-faults on shared writable pages, read-only
-	 * shared pages can get COWed by get_user_pages(.write=1, .force=1).
+	 * Take out anonymous pages first, anonymous shared vmas are
+	 * not accountable.
 	 */
-	if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
+	if (PageAnon(old_page)) {
+		if (!TestSetPageLocked(old_page)) {
+			reuse = can_share_swap_page(old_page);
+			unlock(old_page);
+		}
+	} else if (unlikely((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
 					(VM_WRITE|VM_SHARED))) {
+		/*
+		 * Only catch write-faults on shared writable pages,
+		 * read-only shared pages can get COWed by
+		 * get_user_pages(.write=1, .force=1).
+		 */
 		if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 			/*
 			 * Notify the address space that the page is about to

@@ -1502,9 +1512,6 @@ static int do_wp_page(struct mm_struct *
 		dirty_page = old_page;
 		get_page(dirty_page);
 		reuse = 1;
-	} else if (PageAnon(old_page) && !TestSetPageLocked(old_page)) {
-		reuse = can_share_swap_page(old_page);
-		unlock_page(old_page);
 	}
 
 	if (reuse) {

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

Yes, I believe your patch below is just how it should be.

Anonymous pages in a shared writable vma, got there via ptrace poke. They're in a curious limbo between private and shared. You can reasonably argue that the page was supposed to be shared in the first place, so although it's now become private, it's reasonable for it to remain shared at least between parent and child. I don't disagree.

But if it's then swapped out under memory pressure, and brought back in, it will be treated as an ordinary anonymous page, write-protected, and once parent or child makes a modification, will cease to be shared between parent and child. Not a big deal to lose sleep over, but such pages do behave inconsistently.

Hugh

From: Peter Zijlstra <[email protected]>

Now that we can detect writers of shared mappings, throttle them. Avoids OOM by surprise.

Changes -v2:

- small helper function (Andrew Morton)

 include/linux/writeback.h |    1 +
 mm/memory.c               |    5 +++--
 mm/page-writeback.c       |   10 ++++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

Index: 2.6-mm/mm/memory.c
===================================================================
@@ -49,6 +49,7 @@
 #include <linux/module.h>
 #include <linux/delayacct.h>
 #include <linux/init.h>
+#include <linux/writeback.h>
 
 #include <asm/pgalloc.h>
 #include <asm/uaccess.h>

@@ -1572,7 +1573,7 @@ gotten:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
-		set_page_dirty(dirty_page);
+		set_page_dirty_balance(dirty_page);
 		put_page(dirty_page);
 	}
 	return ret;

@@ -2219,7 +2220,7 @@ retry:
 unlock:
 	pte_unmap_unlock(page_table, ptl);
 	if (dirty_page) {
-		set_page_dirty(dirty_page);
+		set_page_dirty_balance(dirty_page);
 		put_page(dirty_page);
 	}
 	return ret;

Index: 2.6-mm/include/linux/writeback.h
===================================================================
@@ -121,6 +121,7 @@ int sync_page_range(struct inode *inode,
 			loff_t pos, loff_t count);
 int sync_page_range_nolock(struct inode *inode, struct address_space *mapping,
 			   loff_t pos, loff_t count);
+void set_page_dirty_balance(struct page *page);
 
 /* pdflush.c */
 extern int nr_pdflush_threads;	/* Global so it can be exported to sysctl

Index: 2.6-mm/mm/page-writeback.c
===================================================================
@@ -237,6 +237,16 @@ static void balance_dirty_pages(struct a
 		pdflush_operation(background_writeout, 0);
 }
 
+void set_page_dirty_balance(struct page *page)
+{
+	if (set_page_dirty(page)) {
+		struct address_space *mapping = page_mapping(page);
+
+		if (mapping)
+			balance_dirty_pages_ratelimited(mapping);
+	}
+}
+
 /**
  * balance_dirty_pages_ratelimited_nr - balance dirty memory state
  * @mapping: address_space which was dirtied

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

From: Peter Zijlstra <[email protected]>

With the tracking of dirty pages properly done now, msync doesn't need to scan the PTEs anymore to determine the dirty status.

 mm/msync.c |  129 ++++---------------------------------------------------------
 1 file changed, 9 insertions(+), 120 deletions(-)

Index: 2.6-mm/mm/msync.c
===================================================================
@@ -20,109 +20,14 @@
 #include <asm/pgtable.h>
 #include <asm/tlbflush.h>
 
-static unsigned long msync_pte_range(struct vm_area_struct *vma, pmd_t *pmd,
-				unsigned long addr, unsigned long end)
-{
-	pte_t *pte;
-	spinlock_t *ptl;
-	int progress = 0;
-	unsigned long ret = 0;
-
-again:
-	pte = pte_offset_map_lock(vma->vm_mm, pmd, addr, &ptl);
-	do {
-		struct page *page;
-
-		if (progress >= 64) {
-			progress = 0;
-			if (need_resched() || need_lockbreak(ptl))
-				break;
-		}
-		progress++;
-		if (!pte_present(*pte))
-			continue;
-		if (!pte_maybe_dirty(*pte))
-			continue;
-		page = vm_normal_page(vma, addr, *pte);
-		if (!page)
-			continue;
-		if (ptep_clear_flush_dirty(vma, addr, pte) ||
-				page_test_and_clear_dirty(page))
-			ret += set_page_dirty(page);
-		progress += 3;
-	} while (pte++, addr += PAGE_SIZE, addr != end);
-	pte_unmap_unlock(pte - 1, ptl);
-	cond_resched();
-	if (addr != end)
-		goto again;
-	return ret;
-}
-
-static inline unsigned long msync_pmd_range(struct vm_area_struct *vma,
-			pud_t *pud, unsigned long addr, unsigned long end)
-{
-	pmd_t *pmd;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	pmd = pmd_offset(pud, addr);
-	do {
-		next = pmd_addr_end(addr, end);
-		if (pmd_none_or_clear_bad(pmd))
-			continue;
-		ret += msync_pte_range(vma, pmd, addr, next);
-	} while (pmd++, addr = next, addr != end);
-	return ret;
-}
-
-static inline unsigned long msync_pud_range(struct vm_area_struct *vma,
-			pgd_t *pgd, unsigned long addr, unsigned long end)
-{
-	pud_t *pud;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	pud = pud_offset(pgd, addr);
-	do {
-		next = pud_addr_end(addr, end);
-		if (pud_none_or_clear_bad(pud))
-			continue;
-		ret += msync_pmd_range(vma, pud, addr, next);
-	} while (pud++, addr = next, addr != end);
-	return ret;
-}
-
-static unsigned long msync_page_range(struct vm_area_struct *vma,
-				unsigned long addr, unsigned long end)
-{
-	pgd_t *pgd;
-	unsigned long next;
-	unsigned long ret = 0;
-
-	/* For hugepages we can't go walking the page table normally,
-	 * but that's ok, hugetlbfs is memory based, so we don't need
-	 * to do anything more on an msync().
-	 */
-	if (vma->vm_flags & VM_HUGETLB)
-		return 0;
-
-	BUG_ON(addr >= end);
-	pgd = pgd_offset(vma->vm_mm, addr);
-	flush_cache_range(vma, addr, end);
-	do {
-		next = pgd_addr_end(addr, end);
-		if (pgd_none_or_clear_bad(pgd))
-			continue;
-		ret += msync_pud_range(vma, pgd, addr, next);
-	} while (pgd++, addr = next, addr != end);
-	return ret;
-}
-
 /*
  * MS_SYNC syncs the entire file - including mappings.
  *
- * MS_ASYNC does not start I/O (it used to, up to 2.5.67).  Instead, it just
- * marks the relevant pages dirty.  The application may now run fsync() to
+ * MS_ASYNC does not start I/O (it used to, up to 2.5.67).
+ * Nor does it marks the relevant pages dirty (it used to up to 2.6.17).
+ * Now it doesn't do anything, since dirty pages are properly tracked.
+ *
+ * The application may now run fsync() to
  * write out the dirty pages and wait on the writeout and check the result.
  * Or the application may run fadvise(FADV_DONTNEED) against the fd to start
  * async writeout immediately.

@@ -130,16 +35,11 @@ static unsigned long msync_page_range(st
  * applications.
  */
 static int msync_interval(struct vm_area_struct *vma, unsigned long addr,
-			unsigned long end, int flags,
-			unsigned long *nr_pages_dirtied)
+			unsigned long end, int flags)
 {
-	struct file *file = vma->vm_file;
-
 	if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED))
 		return -EBUSY;
 
-	if (file && (vma->vm_flags & VM_SHARED))
-		*nr_pages_dirtied = msync_page_range(vma, addr, end);
 	return 0;
 }
 

@@ -178,7 +78,6 @@ asmlinkage long sys_msync(unsigned long 
 		goto out_unlock;
 	}
 	do {
-		unsigned long nr_pages_dirtied = 0;
 		struct file *file;
 
 		/* Here start < vma->vm_end. */

@@ -189,8 +88,7 @@ asmlinkage long sys_msync(unsigned long 
 		/* Here vma->vm_start <= start < vma->vm_end. */
 		if (end <= vma->vm_end) {
 			if (start < end) {
-				error = msync_interval(vma, start, end, flags,
-							&nr_pages_dirtied);
+				error = msync_interval(vma, start, end, flags);
 				if (error)
 					goto out_unlock;
 			}

@@ -198,22 +96,13 @@ asmlinkage long sys_msync(unsigned long 
 			done = 1;
 		} else {
 			/* Here vma->vm_start <= start < vma->vm_end < end. */
-			error = msync_interval(vma, start, vma->vm_end, flags,
-						&nr_pages_dirtied);
+			error = msync_interval(vma, start, vma->vm_end, flags);
 			if (error)
 				goto out_unlock;
 		}
 		file = vma->vm_file;
 		start = vma->vm_end;
-		if ((flags & MS_ASYNC) && file && nr_pages_dirtied) {
-			get_file(file);
-			up_read(&current->mm->mmap_sem);
-			balance_dirty_pages_ratelimited_nr(file->f_mapping,
-							nr_pages_dirtied);
-			fput(file);
-			down_read(&current->mm->mmap_sem);
-			vma = find_vma(current->mm, start);
-		} else if ((flags & MS_SYNC) && file &&
+		if ((flags & MS_SYNC) && file &&
 				(vma->vm_flags & VM_SHARED)) {
 			get_file(file);
 			up_read(&current->mm->mmap_sem);

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

Lots of nice deletions, but...

@@ -7,19 +7,12 @@
 /*
  * The msync() system call.
  */
-#include <linux/slab.h>
-#include <linux/pagemap.h>
 #include <linux/fs.h>
 #include <linux/mm.h>
 #include <linux/mman.h>
-#include <linux/hugetlb.h>
-#include <linux/writeback.h>
 #include <linux/file.h>
 #include <linux/syscalls.h>
 
-#include <asm/pgtable.h>
-#include <asm/tlbflush.h>
-
 /*
  * MS_SYNC syncs the entire file - including mappings.
  *

@@ -34,22 +27,14 @@
  * So by _not_ starting I/O in MS_ASYNC we provide complete flexibility to
  * applications.
  */
-static int msync_interval(struct vm_area_struct *vma, unsigned long addr,
-			unsigned long end, int flags)
-{
-	if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED))
-		return -EBUSY;
-
-	return 0;
-}
 
 asmlinkage long sys_msync(unsigned long start, size_t len, int flags)
 {
 	unsigned long end;
+	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma;
 	int unmapped_error = 0;
 	int error = -EINVAL;
-	int done = 0;
 
 	if (flags & ~(MS_ASYNC | MS_INVALIDATE | MS_SYNC))
 		goto out;

@@ -69,56 +54,46 @@ asmlinkage long sys_msync(unsigned long 
 	 * If the interval [start,end) covers some unmapped address ranges,
 	 * just ignore them, but return -ENOMEM at the end.
 	 */
-	down_read(&current->mm->mmap_sem);
-	if (flags & MS_SYNC)
-		current->flags |= PF_SYNCWRITE;
-	vma = find_vma(current->mm, start);
-	if (!vma) {
-		error = -ENOMEM;
-		goto out_unlock;
-	}
-	do {
+	down_read(&mm->mmap_sem);
+	vma = find_vma(mm, start);
+	for (;;) {
 		struct file *file;
 
+		/* Still start < end. */
+		error = -ENOMEM;
+		if (!vma)
+			goto out_unlock;
 		/* Here start < vma->vm_end. */
 		if (start < vma->vm_start) {
-			unmapped_error = -ENOMEM;
 			start = vma->vm_start;
+			if (start >= end)
+				goto out_unlock;
+			unmapped_error = -ENOMEM;
 		}
 		/* Here vma->vm_start <= start < vma->vm_end. */
-		if (end <= vma->vm_end) {
-			if (start < end) {
-				error = msync_interval(vma, start, end, flags);
-				if (error)
-					goto out_unlock;
-			}
-			error = unmapped_error;
-			done = 1;
-		} else {
-			/* Here vma->vm_start <= start < vma->vm_end < end. */
-			error = msync_interval(vma, start, vma->vm_end, flags);
-			if (error)
-				goto out_unlock;
+		if ((flags & MS_INVALIDATE) && (vma->vm_flags & VM_LOCKED)) {
+			error = -EBUSY;
+			goto out_unlock;
 		}
 		file = vma->vm_file;
 		start = vma->vm_end;
-		if ((flags & MS_SYNC) && file &&
-				(vma->vm_flags & VM_SHARED)) {
+		if ((flags & MS_SYNC) && file && (vma->vm_flags & VM_SHARED)) {
 			get_file(file);
-			up_read(&current->mm->mmap_sem);
+			up_read(&mm->mmap_sem);
 			error = do_fsync(file, 0);
 			fput(file);
-			down_read(&current->mm->mmap_sem);
-			if (error)
-				goto out_unlock;
-			vma = find_vma(current->mm, start);
+			if (error || start >= end)
+				goto out;
+			down_read(&mm->mmap_sem);
+			vma = find_vma(mm, start);
 		} else {
+			if (start >= end)
+				goto out_unlock;
 			vma = vma->vm_next;
 		}
-	} while (vma && !done);
+	}
 out_unlock:
-	current->flags &= ~PF_SYNCWRITE;
-	up_read(&current->mm->mmap_sem);
+	up_read(&mm->mmap_sem);
 out:
-	return error;
+	return error? : unmapped_error;
 }

From: Peter Zijlstra <[email protected]>

mprotect() resets the page protections, which could result in extra write faults for those pages whos dirty state we track using write faults and are dirty already.

---
 mm/mprotect.c |   33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

Index: 2.6-mm/mm/mprotect.c
===================================================================
@@ -28,7 +28,8 @@
 #include <asm/tlbflush.h>
 
 static void change_pte_range(struct mm_struct *mm, pmd_t *pmd,
-		unsigned long addr, unsigned long end, pgprot_t newprot)
+		unsigned long addr, unsigned long end, pgprot_t newprot,
+		int is_accountable)
 {
 	pte_t *pte, oldpte;
 	spinlock_t *ptl;

@@ -43,7 +44,13 @@ static void change_pte_range(struct mm_s
 			 * 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 = ptep_get_and_clear(mm, addr, pte);
+			ptent = pte_modify(ptent, newprot);
+			/* Avoid taking write faults for pages we know to be
+			 * dirty.
+			 */
+			if (is_accountable && pte_dirty(ptent))
+				ptent = pte_mkwrite(ptent);
 			set_pte_at(mm, addr, pte, ptent);
 			lazy_mmu_prot_update(ptent);
 #ifdef CONFIG_MIGRATION

@@ -67,7 +74,8 @@ static void change_pte_range(struct mm_s
 }
 
 static inline void change_pmd_range(struct mm_struct *mm, pud_t *pud,
-		unsigned long addr, unsigned long end, pgprot_t newprot)
+		unsigned long addr, unsigned long end, pgprot_t newprot,
+		int is_accountable)
 {
 	pmd_t *pmd;
 	unsigned long next;

@@ -77,12 +85,13 @@ static inline void change_pmd_range(stru
 		next = pmd_addr_end(addr, end);
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
-		change_pte_range(mm, pmd, addr, next, newprot);
+		change_pte_range(mm, pmd, addr, next, newprot, is_accountable);
 	} while (pmd++, addr = next, addr != end);
 }
 
 static inline void change_pud_range(struct mm_struct *mm, pgd_t *pgd,
-		unsigned long addr, unsigned long end, pgprot_t newprot)
+		unsigned long addr, unsigned long end, pgprot_t newprot,
+		int is_accountable)
 {
 	pud_t *pud;
 	unsigned long next;

@@ -92,12 +101,13 @@ static inline void change_pud_range(stru
 		next = pud_addr_end(addr, end);
 		if (pud_none_or_clear_bad(pud))
 			continue;
-		change_pmd_range(mm, pud, addr, next, newprot);
+		change_pmd_range(mm, pud, addr, next, newprot, is_accountable);
 	} while (pud++, addr = next, addr != end);
 }
 
 static void change_protection(struct vm_area_struct *vma,
-		unsigned long addr, unsigned long end, pgprot_t newprot)
+		unsigned long addr, unsigned long end, pgprot_t newprot,
+		int is_accountable)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	pgd_t *pgd;

@@ -111,7 +121,7 @@ static void change_protection(struct vm_
 		next = pgd_addr_end(addr, end);
 		if (pgd_none_or_clear_bad(pgd))
 			continue;
-		change_pud_range(mm, pgd, addr, next, newprot);
+		change_pud_range(mm, pgd, addr, next, newprot, is_accountable);
 	} while (pgd++, addr = next, addr != end);
 	flush_tlb_range(vma, start, end);
 }

@@ -129,6 +139,7 @@ mprotect_fixup(struct vm_area_struct *vm
 	pgprot_t newprot;
 	pgoff_t pgoff;
 	int error;
+	int is_accountable = 0;
 
 	if (newflags == oldflags) {
 		*pprev = vma;

@@ -184,8 +195,10 @@ success:
 	if (is_shared_writable(newflags) && vma->vm_file)
 		mapping = vma->vm_file->f_mapping;
 	if ((mapping && mapping_cap_account_dirty(mapping)) ||
-			(vma->vm_ops && vma->vm_ops->page_mkwrite))
+			(vma->vm_ops && vma->vm_ops->page_mkwrite)) {
 		mask &= ~VM_SHARED;
+		is_accountable = 1;
+	}
 
 	newprot = protection_map[newflags & mask];
 

@@ -198,7 +211,7 @@ success:
 	if (is_vm_hugetlb_page(vma))
 		hugetlb_change_protection(vma, start, end, newprot);
 	else
-		change_protection(vma, start, end, newprot);
+		change_protection(vma, start, end, newprot, is_accountable);
 	vm_stat_account(mm, oldflags, vma->vm_file, -nrpages);
 	vm_stat_account(mm, newflags, vma->vm_file, nrpages);
 	return 0;

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

From: Peter Zijlstra <[email protected]>

Smallish cleanup to install_page(), could save a memory read (haven't checked the asm output) and sure looks nicer.

---

---
 mm/fremap.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Index: 2.6-mm/mm/fremap.c
===================================================================
@@ -79,9 +79,9 @@ int install_page(struct mm_struct *mm, s
 		inc_mm_counter(mm, file_rss);
 
 	flush_icache_page(vma, page);
-	set_pte_at(mm, addr, pte, mk_pte(page, prot));
+	pte_val = mk_pte(page, prot);
+	set_pte_at(mm, addr, pte, pte_val);
 	page_add_file_rmap(page);
-	pte_val = *pte;
 	update_mmu_cache(vma, addr, pte_val);
 	lazy_mmu_prot_update(pte_val);
 	err = 0;

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

From: Christoph Lameter <[email protected]>

This may be a bit controversial but it does not seem to make sense to use the update_mmu_cache macro when we reuse the page. We are only fiddling around with the protections, the dirty and accessed bits.

With the call to update_mmu_cache the way of using the macros would be different from mprotect() and page_mkclean(). I'd rather have everything work the same way. If this breaks on some arches then also mprotect and page_mkclean() are broken. The use of mprotect() is rare, we may have breakage in some arches that we just have not seen yet.

---
 mm/memory.c |    2 --
 1 file changed, 2 deletions(-)

Index: 2.6-mm/mm/memory.c
===================================================================
@@ -1514,7 +1514,6 @@ static int do_wp_page(struct mm_struct *
 		entry = pte_mkyoung(orig_pte);
 		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
 		ptep_set_access_flags(vma, address, page_table, entry, 1);
-		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
 		ret |= VM_FAULT_WRITE;
 		goto unlock;

@@ -2317,7 +2316,6 @@ static inline int handle_pte_fault(struc
 	entry = pte_mkyoung(entry);
 	if (!pte_same(old_entry, entry)) {
 		ptep_set_access_flags(vma, address, pte, entry, write_access);
-		update_mmu_cache(vma, address, entry);
 		lazy_mmu_prot_update(entry);
 	} else {
 		/*

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

Controversial indeed. Just because Christoph tentatively puts forward such a patch is no reason to include it in your dirty page series.

Probably my fault for drawing attention to the ill-defined relationship between update_mmu_cache and more recently added lazy_mmu_prot_update, and wondering aloud on what's different between do_wp_page and mprotect.

If you want to pursue this patch, you should address it to someone experienced in update_mmu_cache, someone familiar with those architectures which implement it: ideally to Dave Miller (CCed), who designed and wrote most of Documentation/cachetlb.txt.

The answer I expect is that update_mmu_cache is essential there in do_wp_page (reuse case) and handle_pte_fault, on at least some if not all of those arches which implement it. That without those lines, they'll fault and refault endlessly, since the "MMU cache" has not been updated with the write permission.

But omitted from mprotect, since that's dealing with a batch of pages, perhaps none of which will be faulted in the near future:
a waste of resources to update for all those entries.

But now I wonder, why does do_wp_page reuse case flush_cache_page?

Hugh

Yes a likely scenario.

So we intentially allow mprotect to be racy?

Some arches may have virtual caches?

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

It's intentionally allowed to be racy (ambiguous whether a racing thread sees protections before or protections after) up until the flush_tlb_range. Should be well-defined from there on. Or am I misunderstanding you?

Sorry, I don't get it, you'll have to spell it out to me in detail. We have a page mapped for reading, we're about to map that same page for writing too. We have no modifications to flush yet, why flush_cache_page there?

Hugh

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

No thats fine but this line of thinking establishes that we need update_mmu_cache for protection changes. So the documentation on the role of these calls needs to change. lazy_mmu_prot_update does not do the notification to arch specific code as documented otherwise we would not need the flush_tlb_range. In fact it seems that lazy_mmu_prot_update does only deal with icache/dcache coherency issues and it is separate from update_mmu_cache because we can avoid checking the icache/dcache issues in every update_mmu_cache.

Hmmm. I found this by Dave Miller in 1999

http://www.ussg.iu.edu/hypermail/linux/kernel/9906.0/1237.html

flush_cache_page(vma, page) is meant to also take care of the case here for some reason the TLB entry must exist for the cache entry to be valid as well. This is the case on the HyperSparc's combined I/D L2 cache (it has no L1 cache), you cannot flush out cache entries which have no translation, it will make the cpu trap. Sparc/sun4c's mmu is like this too.

If I read this correctly then it seems the reason that flush_cache_page was placed there is that on some architecture the TLB entry must exist correctly on virtual caches to be able to flush the caches (maybe they are hashed?). update_mmu_cache is called after we changed things so there may be no way to determine how to flush the cache contents for the page contents if we later drop the page.

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