mm: mremap correct rmap accounting

Just spotted this possible issue when searching for another bug. Am I right in my thinking? I haven't dug out my x86-multi-ZERO_PAGE-patch to verify (or actually test the patch :P).

Comments?

Nick

SUSE Labs, Novell Inc.

When mremap()ing virtual addresses, some architectures (read: MIPS) switches underlying pages if encountering ZERO_PAGE(old_vaddr) != ZERO_PAGE(new_vaddr).

The problem is that the refcount and mapcount remain on the old page, while the actual pte is switched to the new one. This would counter underruns and confuse the rmap code.

Fix it by actually moving accounting info to the new page. Would it be neater to do this in move_pte? maybe rmap.c? (nick mumbles something about not accounting ZERO_PAGE()s)

Index: linux-2.6/mm/mremap.c
===================================================================
@@ -18,6 +18,7 @@
 #include <linux/highmem.h>
 #include <linux/security.h>
 #include <linux/syscalls.h>
+#include <linux/rmap.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>

@@ -72,7 +73,7 @@ static void move_ptes(struct vm_area_str
 {
 	struct address_space *mapping = NULL;
 	struct mm_struct *mm = vma->vm_mm;
-	pte_t *old_pte, *new_pte, pte;
+	pte_t *old_pte, *new_pte;
 	spinlock_t *old_ptl, *new_ptl;
 
 	if (vma->vm_file) {

@@ -102,12 +103,28 @@ static void move_ptes(struct vm_area_str
 
 	for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
 				   new_pte++, new_addr += PAGE_SIZE) {
+		pte_t new, old;
+
 		if (pte_none(*old_pte))
 			continue;
-		pte = ptep_clear_flush(vma, old_addr, old_pte);
+		old = ptep_clear_flush(vma, old_addr, old_pte);
 		/* ZERO_PAGE can be dependant on virtual addr */
-		pte = move_pte(pte, new_vma->vm_page_prot, old_addr, new_addr);
-		set_pte_at(mm, new_addr, new_pte, pte);
+		new = move_pte(old, new_vma->vm_page_prot, old_addr, new_addr);
+		if (unlikely(pte_pfn(old) != pte_pfn(new))) {
+			struct page *page;
+			/* must be different ZERO_PAGE()es. Update accounting */
+
+			page = vm_normal_page(vma, old_addr, old);
+			BUG_ON(page != ZERO_PAGE(old_addr));
+			put_page(page);
+			page_remove_rmap(page, vma);
+
+			page = vm_normal_page(new_vma, new_addr, new);
+			BUG_ON(page != ZERO_PAGE(new_addr));
+			get_page(page);
+			page_add_file_rmap(page);
+		}
+		set_pte_at(mm, new_addr, new_pte, new);
 	}
 
 	arch_leave_lazy_mmu_mode();

Good point. Nasty.

Tiresome, I can quite see why it brings you to mumbling.

Though it looks right, I do hate the patch cluttering up move_ptes() like that: will the compiler be able to work out that that "unlikely" means impossible (and optimize away the code) on all arches but MIPS? Even if it can, I'd rather not see it there.

Could you make the MIPS move_pte() a proper function, say in arch/mips/mm/init.c next to setup_zero_pages(), and do that tiresome stuff there - should then be able to assume ZERO_PAGEs and skip the BUG_ON embellishments.

Utter nit-of-nits: my sense of symmetry prefers that you put_page() after page_remove_rmap() instead of before.

Hugh

Yeah it doesn't look quite right.

The only thing I was thinking of was if another arch comes along and does the same thing. Also tried to keep such rmap specifics in mm/.

But if you're happy to do it that way, then I'm happy with it!

I prefer that way too, now you mention it ;)

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

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

OK, how's this one?

Not tested on MIPS, but the same move_pte compiled on i386 here.

I sent Ralf a little test program that should eventually free a ZERO_PAGE if it is run a few times (with a non-zero zero_page_mask). Do you have time to confirm, Ralf?

Thanks,
Nick

SUSE Labs, Novell Inc.

When mremap()ing virtual addresses, some architectures (read: MIPS) switches underlying pages if encountering ZERO_PAGE(old_vaddr) != ZERO_PAGE(new_vaddr).

The problem is that the refcount and mapcount remain on the old page, while the actual pte is switched to the new one. This would counter underruns and confuse the rmap code.

Fix it by actually moving accounting info to the new page in the MIPS architecture code.

Also, clean up mremap slightly, so we can see where the old and the new pte is being used.

Index: linux-2.6/mm/mremap.c
===================================================================
@@ -72,7 +72,7 @@ static void move_ptes(struct vm_area_str
 {
 	struct address_space *mapping = NULL;
 	struct mm_struct *mm = vma->vm_mm;
-	pte_t *old_pte, *new_pte, pte;
+	pte_t *old_pte, *new_pte;
 	spinlock_t *old_ptl, *new_ptl;
 
 	if (vma->vm_file) {

@@ -102,12 +102,17 @@ static void move_ptes(struct vm_area_str
 
 	for (; old_addr < old_end; old_pte++, old_addr += PAGE_SIZE,
 				   new_pte++, new_addr += PAGE_SIZE) {
+		pte_t new, old;
+
 		if (pte_none(*old_pte))
 			continue;
-		pte = ptep_clear_flush(vma, old_addr, old_pte);
-		/* ZERO_PAGE can be dependant on virtual addr */
-		pte = move_pte(pte, new_vma->vm_page_prot, old_addr, new_addr);
-		set_pte_at(mm, new_addr, new_pte, pte);
+		old = ptep_clear_flush(vma, old_addr, old_pte);
+		/*
+		 * ZERO_PAGE can be dependant on virtual addr, so pte may
+		 * actually have to be changed
+		 */
+		new = move_pte(old, new_vma->vm_page_prot, vma, old_addr, new_addr);
+		set_pte_at(mm, new_addr, new_pte, new);
 	}
 
 	arch_leave_lazy_mmu_mode();

Index: linux-2.6/arch/mips/mm/init.c
===================================================================
@@ -25,6 +25,7 @@
 #include <linux/swap.h>
 #include <linux/proc_fs.h>
 #include <linux/pfn.h>
+#include <linux/rmap.h>
 
 #include <asm/bootinfo.h>
 #include <asm/cachectl.h>

@@ -103,6 +104,41 @@ unsigned long setup_zero_pages(void)
 	return 1UL << order;
 }
 
+
+pte_t move_pte(pte_t pte, pgprot_t prot, struct vm_area_struct *old_vma,
+			unsigned long old_addr, unsigned long new_addr)
+{
+	unsigned long pfn;
+	struct page *old_page, *new_page;
+ 	pte_t newpte = pte;
+
+	if (!pte_present(pte))
+		goto out;
+
+	pfn = pte_pfn(pte);
+	if (!pfn_valid(pfn))
+		goto out;
+
+	old_page = vm_normal_page(old_vma, old_addr, pte);
+	if (likely(old_page != ZERO_PAGE(old_addr)))
+		goto out;
+
+	new_page = ZERO_PAGE(new_addr);
+	if (old_page == new_page)
+		goto out;
+
+	get_page(new_page);
+	page_add_file_rmap(new_page);
+
+	page_remove_rmap(old_page, old_vma);
+	put_page(old_page);
+
+	newpte = mk_pte(new_page, prot);
+
+out:
+	return newpte;
+}
+
 /*
  * These are almost like kmap_atomic / kunmap_atmic except they take an
  * additional address argument as the hint.

Index: linux-2.6/include/asm-generic/pgtable.h
===================================================================
@@ -159,7 +159,14 @@ static inline void ptep_set_wrprotect(st
 #endif
 
 #ifndef __HAVE_ARCH_MOVE_PTE
-#define move_pte(pte, prot, old_addr, new_addr)	(pte)
+/*
+ * Those architectures that define __HAVE_ARCH_MOVE_PTE must take care
+ * to update the rmap information on a vm_normal_page (which includes
+ * ZERO_PAGE), if they update the pte to some other page here.
+ *
+ * See MIPS for an example.
+ */
+#define move_pte(pte, prot, old_vma, old_addr, new_addr)	(pte)
 #endif
 
 /*

Index: linux-2.6/include/asm-mips/pgtable.h
===================================================================
@@ -70,14 +70,8 @@ extern unsigned long zero_page_mask;
 	(virt_to_page((void *)(empty_zero_page + (((unsigned long)(vaddr)) & zero_page_mask))))
 
 #define __HAVE_ARCH_MOVE_PTE
-#define move_pte(pte, prot, old_addr, new_addr)				\
-({									\
- 	pte_t newpte = (pte);						\
-	if (pte_present(pte) && pfn_valid(pte_pfn(pte)) &&		\
-			pte_page(pte) == ZERO_PAGE(old_addr))		\
-		newpte = mk_pte(ZERO_PAGE(new_addr), (prot));		\
-	newpte;								\
-})
+extern pte_t move_pte(pte_t pte, pgprot_t prot, struct vm_area_struct *old_vma,
+			unsigned long old_addr, unsigned long new_addr);
 
 extern void paging_init(void);
 

Index: linux-2.6/include/asm-sparc64/pgtable.h
===================================================================
@@ -691,7 +691,7 @@ static inline void set_pte_at(struct mm_
 
 #ifdef DCACHE_ALIASING_POSSIBLE
 #define __HAVE_ARCH_MOVE_PTE
-#define move_pte(pte, prot, old_addr, new_addr)				\
+#define move_pte(pte, prot, old_vma, old_addr, new_addr)		\
 ({									\
  	pte_t newpte = (pte);						\
 	if (tlb_type != hypervisor && pte_present(pte)) {		\

umm, that sounds fairly fatal. For how long has this bug been present?

--
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 would be. We have a catch to prevent ZERO_PAGE from actually getting freed, but it would spew warnings and eventually go bug in the page_mapcount underflow check.

It has been around for quite a few releases, so either we don't have many R4000 or R4400 SC and MC CPUs running a recent kernel, or this type of mremap is pretty rare, or both. We get far more reports of mapcount underflow on x86 than MIPS!

If Ralf acks then I think it could go into the next release.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

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

Grudging okay - so irritating to have to do this!

We have different ideas of what's a good cleanup
(pte -> old/new), but if you wish.

I'd much rather you'd got rid of move_pte's prot argument, that can be taken from the old_vma you're now having to pass (because of your debug additions to page_remove_rmap).

Grudging okay: thanks for fixing it.

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>

I really hate it. Like REALLY REALLY hate it.

This just seems really stupid.

How about making the zero-page on MIPS be PageCompound(), and then have all the sub-pages just point to the first page - that's how compound pages work anyway.

Then the mapcount/refcount is all done on the compound page, and none of these problems occur.

Hmm?

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>

Can we convert those bits of mips to just have a single zero-page, like everyone else?

Is that trick a correctness thing, or a performance thing? If the latter, how useful is it, and how common are the chips which use it?

--
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 was a performance thing, iirc. Apparently a fairly big deal: pages kicking each other out of the cache due to idiotic cache design. But I forget any details.

MIPS in general is a f*cking pain in the *ss. They have a few chips that are sane, but just an incredible amount of totally braindamaged ones. They're not the only ones with virtual caches, but they're certainly well-represented there. Sad.

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>

On Mon, Jan 29, 2007 at 12:18:29PM -0800, Linus Torvalds wrote:

(adding Maciej who is using R4000 for much of his MIPS hacking to cc ...)

R4000 and R4400 SC and MC versions suffer from virtual aliases but have a hardware mechanism to detect them and will throw a virtual coherency exception if so. So in theory for these processors the entire burden of handling cache aliases can be left to like ten lines of exception handling code. A real world machine may encounter millions of these exceptions in a relativly short time which sucks performancewise, so aliasing avoidance by careful selection of addresses and cacheflushes is needed.

So to make that more explicit, it's not needed for correctness but it's a performance - and sometimes a fairly big one - thing.

(The easy solution for the issue would be raising the pagesize to the next higher supported values, 16kB or 64kB. Now for the true idiocy of this exception-based scheme - it happens that the hardware checks three bits for aliasing as if the CPU had 32kB direct mapped caches even though these types only have 8kB (R4000) rsp 16kB (R4400) primary caches. This means ramping up the page size to 16kB wouldn't suffice - it would have to be 64kB to eleminate aliases which is impractical for memory reasons on many systems.

Anyway, no other MIPS processor has this "virtual coherency exception" and so I don't have an affected system at hand right now. So I hacked a 2.6.20-rc6 kernel for another machine to do R4x00 style ZERO_PAGE handling - and the system did survive a quick 10min testing just fine until a test case which Nick had mailed to me killed the system as predicted. I blame obsoletion of the chips and lucky choice of workload for this issue only ever having been reported once and that report was probably ignored because it was sufficiently obscure.

Eh... MIPS isn't the architecture with VIVT data caches. It's other arch maintainers that get their sleep robbed by truly st00pid caches.

Ralf

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

Beautiful idea, an entirely appropriate use of PageCompound().

But it won't quite work as is, since only page_count() is diverted via PageCompound(): page_mapcount() works on exactly the page given. So the MIPS ZERO_PAGEs could still hit the page_remove_rmap() BUG.

Agreed that's a surprising divergence: but it's worked fine to date, and I'm hesitant to change it in a hurry, need to pause to consider the ramifications.

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>

Ok.

How about:
- the current code has worked so far, so there is no way in hell I'll take a patch for some odd-ball architecture for a theoretical problem that nobody else cares about for 2.6.20 *anyway* and hasn't been reported until now (considering that it's apparently been around since rmap went in).

- somebody who cares would explore trying to make page_mapcount work like page_count, and see if this is a viable approach.

- at worst, we can do the Andrew thing, and just forget about the ZERO_PAGE() multi-page thing. It may well be that even MIPS people don't care any more, if the particular uarch version that was helped most isn't very common any more (we can always hope..)

Hmm?

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'm currently thinking the right answer will be to keep the MIPS multiple ZERO_PAGEs, for their performance benefit, but ditch the way MIPS moves from one ZERO_PAGE to another in move_pte, trying desperately to retain that performance benefit across mremap.

It never used to do that, it was something Nick noticed and put into 2.6.14. I've the ghost of a memory that at that time he had some code which was actually relying on ZERO_PAGE(vaddr) always being the expected ZERO_PAGE for that vaddr. I'm about to search my old mailboxes.

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>

Ah, it wasn't any special patch of Nick's that needed it for correctness, it was filemap_xip and its use the ZERO_PAGE(address) (to avoid unnecessary page allocations): with page_check_address() an interface for checking just a single page, not a range of them.

Could make it loop over them all, but a quicker patch would be as below. I've no idea if the intersection of filemap_xip users and MIPS users is the empty set or more interesting. But I'd prefer you don't just slam in the patch, better have an opinion from Carsten and/or Nick first.

Nick Piggin points out that page accounting on MIPS multiple ZERO_PAGEs is not maintained by its move_pte, and could lead to freeing a ZERO_PAGE. Instead of complicating that move_pte, just forget the minor optimization when mremapping, and change the one thing which needed it for correctness
- filemap_xip use ZERO_PAGE(0) throughout instead of according to address.

---

 include/asm-mips/pgtable.h |   10 ----------
 mm/filemap_xip.c           |    4 ++--
 mm/mremap.c                |    1 -
 3 files changed, 2 insertions(+), 13 deletions(-)

@@ -69,16 +69,6 @@ extern unsigned long zero_page_mask;
 #define ZERO_PAGE(vaddr) \
 	(virt_to_page((void *)(empty_zero_page + (((unsigned long)(vaddr)) & zero_page_mask))))
 
-#define __HAVE_ARCH_MOVE_PTE
-#define move_pte(pte, prot, old_addr, new_addr)				\
-({									\
- 	pte_t newpte = (pte);						\
-	if (pte_present(pte) && pfn_valid(pte_pfn(pte)) &&		\
-			pte_page(pte) == ZERO_PAGE(old_addr))		\
-		newpte = mk_pte(ZERO_PAGE(new_addr), (prot));		\
-	newpte;								\
-})
-
 extern void paging_init(void);
 
 /*

@@ -183,7 +183,7 @@ __xip_unmap (struct address_space * mapp
 		address = vma->vm_start +
 			((pgoff - vma->vm_pgoff) << PAGE_SHIFT);
 		BUG_ON(address < vma->vm_start || address >= vma->vm_end);
-		page = ZERO_PAGE(address);
+		page = ZERO_PAGE(0);
 		pte = page_check_address(page, mm, address, &ptl);
 		if (pte) {
 			/* Nuke the page table entry. */

@@ -246,7 +246,7 @@ xip_file_nopage(struct vm_area_struct * 
 		__xip_unmap(mapping, pgoff);
 	} else {
 		/* not shared and writable, use ZERO_PAGE() */
-		page = ZERO_PAGE(address);
+		page = ZERO_PAGE(0);
 	}
 
 out:

@@ -105,7 +105,6 @@ static void move_ptes(struct vm_area_str
 		if (pte_none(*old_pte))
 			continue;
 		pte = ptep_clear_flush(vma, old_addr, old_pte);
-		/* ZERO_PAGE can be dependant on virtual addr */
 		pte = move_pte(pte, new_vma->vm_page_prot, old_addr, new_addr);
 		set_pte_at(mm, new_addr, new_pte, pte);
 	}

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

Yeah you could do that. I found it interesting that there is no way in hell Linus will take an architecture-specific patch for this odd-ball architecture and its obscure problem, but would rather fix it in generic code.

My opinion? If we do keep the multiple zero pages thing, then I would prefer keep them coherent with their virtual addresses to prevent future surprises. Ditching multiple zero pages completely would be ideal, from a core mm/ point of view, of course.

I agree my patch is ugly for having to do refcount and rmap work outside mm/. But I don't see anything wrong with it as the minimal correctness fix that we could later reevaluate.

SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com 

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

As far as I can tell, the intersection is zero. There is no block device driver one could use for XIP on mips platforms.

Carsten

--
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 flash ;-)

XIP is mostly an embedded feature. The affected MIPS R4[40]00[SM]C processors are desktop and server processors so there almost fundamentally is no overlap.

Ralf

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

The current xip implementation does not work with flash. It only works with ram/rom, and we use it in a virtual environment to share data between multiple virtual machines.
After a long discussion with Joern Engel and David Woodhouse, I believe we came up with an idea how we would extend the existing xip support to work with memory technology device. But that is not implemented, and not work in progress.

Actually, only few people consider s390 as embedded platform. And s390 is the only exploiter of xip to date.

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

Took me some time to catch up on this thread, sorry for that. Yea, I think xip can be implemented correctly that it works on mips when we loop over all zero pages on unmap. Let me try to come up with a patch for that.

Carsten

--
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 problem. And sorry for being so slow to reply to you, there seems to have been an assault on my home connectivity.

I think it's now clear that XIP won't be impacted at all by my ZERO_PAGE(0) change, and that's the patch Linus should put in for 2.6.20 (given how much he disliked Nick's patch to maintain the different zeropage counts across mremap move). Ah, good, that's now gone into his tree since last I looked.

Sorry, no, I didn't mean for you to try that, and there appears to be no need for it at all, for the foreseeable future anyway:
please don't waste your time on that.

But there is a change which I now think you do need to make, for 2.6.21 - let it not distract attention from the pagecount correctness issue we've been discussing so far. Something I should have noticed when I first looked at your clever use of the ZERO_PAGE, but have only noticed now. Doesn't it clash with the clever use of the ZERO_PAGE when reading /dev/zero (see read_zero_pagealigned in drivers/char/mem.c)?

Consider two PROT_READ|PROT_WRITE,MAP_PRIVATE mappings of a four-page hole in a XIP file. One of them just readfaults the four pages in (and is given ZERO_PAGE for each), the other has four pages read from /dev/zero into it (which also maps the ZERO_PAGE into its four ptes).

Then imagine that non-zero data is written to the first page of that hole, by a write syscall, or through a PROT_WRITE,MAP_SHARED mapping. __xip_unmap will replace the first ZERO_PAGE in each of the MAP_PRIVATE mappings by the new non-zero data page. Which is correct for the first mapping which just did readfaults, but wrong for the second mapping which has overwritten by reading /dev/zero
- those pages ought to remain zeroed, never seeing the later data.

I've never much liked the read_zero_pagealigned cleverness, seems "too clever by half" as we say, and this overlooked clash shows why. But I'm also scared to remove any long-established optimization(?), for fear of impacting some unknown workload.

So, if you're to retain your share-one-page-for-holes cleverness, I think you need to switch over to allocating a page of your own for it, instead of using the ZERO_PAGE.

Or have I got it wrong? A simple test should show.

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>

Good to know this issue is solved. Thank you.

Wait a second, I fail to see why those pages in the second mapping should remain zeroed and never see the later data; they do reflect the file content that was mmap()ed there correctly at all times. I would expect this behavior, and would expect to see both mappings reflect this data. Am I missing something?

The test is simple indeed, will do that - and compare page cache backed file behavior with xip file.

Carsten

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

Nasty. I got your point now, but as far as I can see we're still in-spec:

MAP_PRIVATE
Create a private copy-on-write mapping. Stores to the region do not affect the original file. It is unspecified whether changes made to the file after the mmap call are visible in the mapped region.

A fix could be to use my own empty page instead of the ZERO_PAGE for xip. At least we have different behavior with/without xip here, therefore I agree that this requires fixing.

Carsten

--
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 agree that last sentence _appears_ to give you a let out. I believe its intention is to address the case where one page has been faulted in and written to by the app, the next page is unfaulted then modified by some other means and then faulted into the app for the first time:
that page will contain the mods made to the underlying object, even though they were made after the private copy of the previous page was taken (which copy should never show later mods to the underlying object). Whereas if the mapping were mmap'ed with MAP_LOCKED (or mlocked), all pages would be faulted in immediately, and subsequent mods to the underlying object never seen at all.

Whatever the wording, I don't know of any application which is happy for the modifications it makes to a MAP_PRIVATE mapping to disappear without warning - except when it actually asks for that behaviour by calling madvise(start, len, MADV_DONTNEED).

Exactly, that's what I was meaning. Just take care to do enough but not too much locking when allocating that page: for example

	if (!our_zero_page) {
		allocate a zeroed page;
		spin_lock;
		if (!our_zero_page)
			our_zero_page = page just allocated;
		else
			free page just allocted;
		spin_unlock;
	}

Yes, if your testing shows that it really does behave as I suspect.

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>

I see. Makes sense to interpret the spec that way.

I am not happy with any visibility of xip to userland via filesystem syscalls. It is supposed to be transparent for userspace.

It does indeed. My little test program shows it:
- ext2 in regular operation:
current state: read-faulted sparse mappings content of area1:
content of area2:
current state: read zero into area1
content of area1:
content of area2:
current state: write data via sys_write
content of area1:
content of area2: this change was written using sys_write

- ext with xip:
current state: read-faulted sparse mappings content of area1:
content of area2:
current state: read zero into area1
content of area1:
content of area2:
current state: write data via sys_write
content of area1: this change was written using sys_write content of area2: this change was written using sys_write

That proves your suspicion. I will submit a fix. Thank you for pointing me at it.

Carsten

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