Don't needlessly dirty mlocked pages when initially faulting them in.

PATCH In review Suleiman Souhlal 6 participants 17 messages Jul 26, 2007 – Oct 12, 2007

Subsystems (2)

Memory management MM core

lore

Share & tools

lore

Suleiman Souhlal

author

2007-07-26 23:53 UTC #0 lore raw

PATCH 1/1 Don't needlessly dirty mlocked pages when initially faulting them in.

make_pages_present() is dirtying mlocked pages if the VMA is writable, even though it shouldn't, by telling get_user_pages() to simulate a write fault.

A simple way to test this is to mlock a multi-GB file, and then sync. The sync will take a long time.

As far as I can see, it should be safe to just not simulate a write fault.

Signed-off-by Suleiman Souhlal <[email protected]>

---
 mm/memory.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

mm/memory.c +2 -3

index f64cbf9..f43c9e8 100644
@@ -2664,18 +2664,17 @@ #endif /* __PAGETABLE_PMD_FOLDED */
 
 int make_pages_present(unsigned long addr, unsigned long end)
 {
-	int ret, len, write;
+	int ret, len;
 	struct vm_area_struct * vma;
 
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;
-	write = (vma->vm_flags & VM_WRITE) != 0;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = (end+PAGE_SIZE-1)/PAGE_SIZE-addr/PAGE_SIZE;
 	ret = get_user_pages(current, current->mm, addr,
-			len, write, 0, NULL, NULL);
+			len, 0, 0, NULL, NULL);
 	if (ret < 0)
 		return ret;
 	return ret == len ? 0 : -1;

Signature · 1 line

1.4.2

Andrew Morton

maintainer

2007-07-27 00:23 UTC #1 lore raw

Quoted 5 lines · On Thu, 26 Jul 2007 16:52:44 -0700 Suleiman Souhlal <[email protected]>…

make_pages_present() is dirtying mlocked pages if the VMA is writable, even though it shouldn't, by telling get_user_pages() to simulate a write fault.
A simple way to test this is to mlock a multi-GB file, and then sync. The sync will take a long time.

ugh, how bad of us.

Quoted 1 line

As far as I can see, it should be safe to just not simulate a write fault.

We pass in "write=1" to force a COW. This is because we want to do all that memory allocation at mlock()-time, not later on, when the app writes to the page.

Quoted 20 lines

Signed-off-by Suleiman Souhlal <[email protected]>

---
 mm/memory.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

mm/memory.c +1 -1

index f64cbf9..f43c9e8 100644
@@ -2664,18 +2664,17 @@ #endif /* __PAGETABLE_PMD_FOLDED */
 
 int make_pages_present(unsigned long addr, unsigned long end)
 {
-	int ret, len, write;
+	int ret, len;
 	struct vm_area_struct * vma;
 
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;

mm/memory.c #endif /* __PAGETABLE_PMD_FOLDED */ · line 2680

Quoted 10 lines

mm/memory.c +1 -2

-	write = (vma->vm_flags & VM_WRITE) != 0;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = (end+PAGE_SIZE-1)/PAGE_SIZE-addr/PAGE_SIZE;
 	ret = get_user_pages(current, current->mm, addr,
-			len, write, 0, NULL, NULL);
+			len, 0, 0, NULL, NULL);
 	if (ret < 0)
 		return ret;
 	return ret == len ? 0 : -1;

So something sterner will need to be done. I guess the write_access arg to handle_mm_fault() would need to become a three-value thing.

Peter Zijlstra

2007-07-27 06:34 UTC #2 lore raw

Quoted 8 lines · Andrew Morton

Quoted 5 lines · On Thu, 26 Jul 2007 16:52:44 -0700 Suleiman Souhlal <[email protected]>…
make_pages_present() is dirtying mlocked pages if the VMA is writable, even though it shouldn't, by telling get_user_pages() to simulate a write fault.
A simple way to test this is to mlock a multi-GB file, and then sync. The sync will take a long time.

Quoted 7 lines

ugh, how bad of us.
Quoted 1 line
As far as I can see, it should be safe to just not simulate a write fault.
We pass in "write=1" to force a COW. This is because we want to do all that memory allocation at mlock()-time, not later on, when the app writes to the page.

Quoted 2 lines

So something sterner will need to be done. I guess the write_access arg to handle_mm_fault() would need to become a three-value thing.

That would be most painfull. Can't we simply set write=0 for shared mappings? Those won't have COW to break and are the onces that do requires writeback. Anonymous and private mappings do COW but will never writeback and are thus save to touch with write=1.

How about something like this:

Signed-off-by Peter Zijlstra <[email protected]>

---
 mm/memory.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

mm/memory.c +6 -1

Index: linux-2.6/mm/memory.c
===================================================================
@@ -2716,7 +2716,12 @@ int make_pages_present(unsigned long add
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;
-	write = (vma->vm_flags & VM_WRITE) != 0;
+	/*
+	 * We want to touch writable mappings with a write fault in otder
+	 * to break COW, except for shared mappings because these don't COW
+	 * and we would not want to dirty them for nothing.
+	 */
+	write = (vma->vm_flags & VM_WRITE|VM_SHARED) == VM_WRITE;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = DIV_ROUND_UP(end, PAGE_SIZE) - addr/PAGE_SIZE;

Suleiman Souhlal

author

2007-10-12 07:19 UTC #3 lore raw

Quoted 42 lines · Peter Zijlstra

Quoted 8 lines · Andrew Morton
Quoted 5 lines · On Thu, 26 Jul 2007 16:52:44 -0700 Suleiman Souhlal <[email protected]>…
make_pages_present() is dirtying mlocked pages if the VMA is writable, even though it shouldn't, by telling get_user_pages() to simulate a write fault.
A simple way to test this is to mlock a multi-GB file, and then sync. The sync will take a long time.
Quoted 7 lines
ugh, how bad of us.
Quoted 1 line
As far as I can see, it should be safe to just not simulate a write fault.
We pass in "write=1" to force a COW. This is because we want to do all that memory allocation at mlock()-time, not later on, when the app writes to the page.
<snip patch>
Quoted 2 lines
So something sterner will need to be done. I guess the write_access arg to handle_mm_fault() would need to become a three-value thing.
That would be most painfull. Can't we simply set write=0 for shared mappings? Those won't have COW to break and are the onces that do requires writeback. Anonymous and private mappings do COW but will never writeback and are thus save to touch with write=1.
How about something like this:
Signed-off-by Peter Zijlstra <[email protected]>
---
 mm/memory.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Index: linux-2.6/mm/memory.c
===================================================================
mm/memory.c +0 -0
@@ -2716,7 +2716,12 @@ int make_pages_present(unsigned long add
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;

mm/memory.c int make_pages_present(unsigned long add · line 2727

Quoted 10 lines

mm/memory.c +6 -1

-	write = (vma->vm_flags & VM_WRITE) != 0;
+	/*
+	 * We want to touch writable mappings with a write fault in otder
+	 * to break COW, except for shared mappings because these don't COW
+	 * and we would not want to dirty them for nothing.
+	 */
+	write = (vma->vm_flags & VM_WRITE|VM_SHARED) == VM_WRITE;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = DIV_ROUND_UP(end, PAGE_SIZE) - addr/PAGE_SIZE;

Oops, I completely forgot about this. Sorry about that.

It works for me (as long as you put parens around VM_WRITE|VM_SHARED).

Please consider committing it. :-)

Signed-off-by Suleiman Souhlal <[email protected]>

-- Suleiman

Peter Zijlstra

2007-10-12 09:03 UTC #4 lore raw

PATCH 1/1 mm: avoid dirtying shared mappings on mlock

Subject: mm: avoid dirtying shared mappings on mlock

Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.

Signed-off-by Peter Zijlstra <[email protected]>

Acked-by Suleiman Souhlal <[email protected]>

---
 mm/memory.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

mm/memory.c +6 -1

Index: linux-2.6/mm/memory.c
===================================================================
@@ -2719,7 +2719,12 @@ int make_pages_present(unsigned long add
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;
-	write = (vma->vm_flags & VM_WRITE) != 0;
+	/*
+	 * We want to touch writable mappings with a write fault in order
+	 * to break COW, except for shared mappings because these don't COW
+	 * and we would not want to dirty them for nothing.
+	 */
+	write = (vma->vm_flags & (VM_WRITE|VM_SHARED)) == VM_WRITE;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = DIV_ROUND_UP(end, PAGE_SIZE) - addr/PAGE_SIZE;

Nick Piggin

2007-10-11 16:57 UTC #5 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 7 lines · Peter Zijlstra

Subject: mm: avoid dirtying shared mappings on mlock
Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Suleiman Souhlal <[email protected]>

Umm, I don't see the other piece of this thread, so I don't know what the actual problem was.

But I would really rather not do this. If you do this, then you now can get random SIGBUSes when you write into the memory if it can't allocate blocks or ... (some other filesystem specific condition).

I agree it feels suboptimal, but we've _always_ done this, right? Is it suddenly a problem? Unless a really nice general way is made to solve this, optimising it like this makes it harder to ensure good semantics for all corner cases I think (and then once the optimisation is in place, it's a lot harder to remove it).

I'll go away and have a better look for the old thread.

Quoted 12 lines

---
 mm/memory.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
Index: linux-2.6/mm/memory.c
===================================================================
mm/memory.c +0 -0
@@ -2719,7 +2719,12 @@ int make_pages_present(unsigned long add
 	vma = find_vma(current->mm, addr);
 	if (!vma)
 		return -1;

mm/memory.c int make_pages_present(unsigned long add · line 2730

Quoted 10 lines

mm/memory.c +6 -1

-	write = (vma->vm_flags & VM_WRITE) != 0;
+	/*
+	 * We want to touch writable mappings with a write fault in order
+	 * to break COW, except for shared mappings because these don't COW
+	 * and we would not want to dirty them for nothing.
+	 */
+	write = (vma->vm_flags & (VM_WRITE|VM_SHARED)) == VM_WRITE;
 	BUG_ON(addr >= end);
 	BUG_ON(end > vma->vm_end);
 	len = DIV_ROUND_UP(end, PAGE_SIZE) - addr/PAGE_SIZE;

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

Nick Piggin

2007-10-11 17:07 UTC #6 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 11 lines · Nick Piggin

Quoted 7 lines · Peter Zijlstra
Subject: mm: avoid dirtying shared mappings on mlock
Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Suleiman Souhlal <[email protected]>
Umm, I don't see the other piece of this thread, so I don't know what the actual problem was.

Found it, but no more clues. Presumably it's some horrible google workload... they're pretty happy to carry these kinds of patches internally, right? ;)

Quoted 10 lines

But I would really rather not do this. If you do this, then you now can get random SIGBUSes when you write into the memory if it can't allocate blocks or ... (some other filesystem specific condition).
I agree it feels suboptimal, but we've _always_ done this, right? Is it suddenly a problem? Unless a really nice general way is made to solve this, optimising it like this makes it harder to ensure good semantics for all corner cases I think (and then once the optimisation is in place, it's a lot harder to remove it).

Yeah, I really would rather not do this. If we actually go through the whole fault path in mlock, then it doesn't really matter what future baggage we attach to fault handlers... (OK, we still technically have some problems with invalidations, but mostly they're avoidable unless doing something silly).

How about just doing another PROT_READ mmap, and mlocking that? (I was going to suggest another syscall, but that's probably overkill).

Peter Zijlstra

2007-10-12 10:37 UTC #7 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

In reply to Nick Piggin

Quoted 9 lines · Nick Piggin

Quoted 7 lines · Peter Zijlstra
Subject: mm: avoid dirtying shared mappings on mlock
Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Suleiman Souhlal <[email protected]>

Quoted 7 lines

Umm, I don't see the other piece of this thread, so I don't know what the actual problem was.
But I would really rather not do this. If you do this, then you now can get random SIGBUSes when you write into the memory if it can't allocate blocks or ... (some other filesystem specific condition).

I'm not getting this, make_pages_present() only has to ensure all the pages are read from disk and in memory. How is this different from a read-scan?

The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().

Nick Piggin

2007-10-11 18:14 UTC #8 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Nick Piggin
Quoted 7 lines · Peter Zijlstra
Subject: mm: avoid dirtying shared mappings on mlock
Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Suleiman Souhlal <[email protected]>
Umm, I don't see the other piece of this thread, so I don't know what the actual problem was.

Quoted 8 lines

Quoted 4 lines
But I would really rather not do this. If you do this, then you now can get random SIGBUSes when you write into the memory if it can't allocate blocks or ... (some other filesystem specific condition).
I'm not getting this, make_pages_present() only has to ensure all the pages are read from disk and in memory. How is this different from a read-scan?

I guess because we've mlocked a region that has PROT_WRITE access... but actually, I suppose mlock doesn't technically require that we can write to the memory, only that the page isn't swapped out.

Still, it is nice to be able to have a reasonable guarantee of writability.

Quoted 2 lines

The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().

Which can SIGBUS, no?

Peter Zijlstra

2007-10-12 10:50 UTC #9 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 27 lines · Nick Piggin

Quoted 13 lines · Peter Zijlstra
Quoted 12 lines · Nick Piggin
Quoted 7 lines · Peter Zijlstra
Subject: mm: avoid dirtying shared mappings on mlock
Suleiman noticed that shared mappings get dirtied when mlocked. Avoid this by teaching make_pages_present about this case.
Signed-off-by Peter Zijlstra <[email protected]>
Acked-by Suleiman Souhlal <[email protected]>
Umm, I don't see the other piece of this thread, so I don't know what the actual problem was.
Quoted 8 lines
Quoted 4 lines
But I would really rather not do this. If you do this, then you now can get random SIGBUSes when you write into the memory if it can't allocate blocks or ... (some other filesystem specific condition).
I'm not getting this, make_pages_present() only has to ensure all the pages are read from disk and in memory. How is this different from a read-scan?
I guess because we've mlocked a region that has PROT_WRITE access... but actually, I suppose mlock doesn't technically require that we can write to the memory, only that the page isn't swapped out.

Quoted 8 lines

Still, it is nice to be able to have a reasonable guarantee of writability.
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?

Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.

I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.

Nick Piggin

2007-10-12 12:23 UTC #10 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 10 lines · Peter Zijlstra

Quoted 6 lines · Nick Piggin
Quoted 2 lines · Peter Zijlstra
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.

Well it is a change in behaviour (admittedly, so was the change to SIGBUS mmaped writes in the first place). It's a matter of semantics I guess. Is the current behaviour actually a _problem_ for anyone? If not, then do we need to change it?

I'm not saying it does matter, just that it might matter ;)

Arjan van de Ven

2007-10-12 14:53 UTC #11 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

In reply to Peter Zijlstra

On Fri, 12 Oct 2007 12:50:22 +0200

Quoted 10 lines

Quoted 4 lines
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.
I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.

well... there's another consideration
people use mlock() in cases where they don't want to go to the filesystem for paging and stuff as well (think the various iscsi daemons and other things that get in trouble).. those kind of uses really use mlock to avoid
1) IO to the filesystem
2) Needing memory allocations for pagefault like things at least for the more "hidden" cases...

prefaulting everything ready pretty much gives them that... letting things fault on demand... nicely breaks that.

Peter Zijlstra

2007-10-12 14:58 UTC #12 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 21 lines · Arjan van de Ven

On Fri, 12 Oct 2007 12:50:22 +0200
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.
I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.
well... there's another consideration
people use mlock() in cases where they don't want to go to the filesystem for paging and stuff as well (think the various iscsi daemons and other things that get in trouble).. those kind of uses really use mlock to avoid
1) IO to the filesystem
2) Needing memory allocations for pagefault like things at least for the more "hidden" cases...

Quoted 2 lines

prefaulting everything ready pretty much gives them that... letting things fault on demand... nicely breaks that.

Non of that is changed. So I'm a little puzzled as to which side you argue.

Suleiman Souhlal

author

2007-10-12 17:45 UTC #13 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 22 lines · Peter Zijlstra

Quoted 21 lines · Arjan van de Ven
On Fri, 12 Oct 2007 12:50:22 +0200
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.
I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.
well... there's another consideration
people use mlock() in cases where they don't want to go to the filesystem for paging and stuff as well (think the various iscsi daemons and other things that get in trouble).. those kind of uses really use mlock to avoid
1) IO to the filesystem
2) Needing memory allocations for pagefault like things at least for the more "hidden" cases...

Quoted 5 lines

Quoted 2 lines
prefaulting everything ready pretty much gives them that... letting things fault on demand... nicely breaks that.
Non of that is changed. So I'm a little puzzled as to which side you argue.

I think this might change the behavior in case you mlock sparse files.
I guess currently the holes disappear when you mlock them, but with the patch the blocks wouldn't get allocated until they get written to.

-- Suleiman

Arjan van de Ven

2007-10-12 17:53 UTC #14 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

Quoted 28 lines · On Fri, 12 Oct 2007 10:45:17 -0700 Suleiman Souhlal <[email protected]>…

Quoted 25 lines · Peter Zijlstra
Quoted 23 lines · Arjan van de Ven
On Fri, 12 Oct 2007 12:50:22 +0200
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.
I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.
well... there's another consideration
people use mlock() in cases where they don't want to go to the filesystem for paging and stuff as well (think the various iscsi daemons and other things that get in trouble).. those kind of uses really use mlock to avoid
1) IO to the filesystem
2) Needing memory allocations for pagefault like things at least for the more "hidden" cases...
prefaulting everything ready pretty much gives them that... letting things fault on demand... nicely breaks that.

Quoted 6 lines

Quoted 2 lines
Non of that is changed. So I'm a little puzzled as to which side you argue.
I think this might change the behavior in case you mlock sparse files. I guess currently the holes disappear when you mlock them, but with the patch the blocks wouldn't get allocated until they get written to.

eh yeah I forgot to mention this was for the sparse case....

Peter Zijlstra

2007-10-12 18:02 UTC #15 lore raw

Re: [PATCH] mm: avoid dirtying shared mappings on mlock

In reply to Suleiman Souhlal

Quoted 27 lines · Suleiman Souhlal

Quoted 25 lines · Peter Zijlstra
Quoted 23 lines · Arjan van de Ven
On Fri, 12 Oct 2007 12:50:22 +0200
Quoted 10 lines
Quoted 4 lines
Quoted 2 lines
The pages will still be read-only due to dirty tracking, so the first write will still do page_mkwrite().
Which can SIGBUS, no?
Sure, but that is no different than any other mmap'ed write. I'm not seeing how an mlocked region is special here.
I agree it would be nice if mmap'ed writes would have better error reporting than SIGBUS, but such is life.
well... there's another consideration
people use mlock() in cases where they don't want to go to the filesystem for paging and stuff as well (think the various iscsi daemons and other things that get in trouble).. those kind of uses really use mlock to avoid
1) IO to the filesystem
2) Needing memory allocations for pagefault like things at least for the more "hidden" cases...
prefaulting everything ready pretty much gives them that... letting things fault on demand... nicely breaks that.

Quoted 6 lines

Quoted 2 lines
Non of that is changed. So I'm a little puzzled as to which side you argue.
I think this might change the behavior in case you mlock sparse files. I guess currently the holes disappear when you mlock them, but with the patch the blocks wouldn't get allocated until they get written to.

Sure, but by point 1 - avoiding IO - that doesn't matter. Once you write to a shared mapping you'll generate IO and you'll hit kernel allocations and other delays no matter what you do.

Mauro Giachero

2007-10-12 13:10 UTC #16 lore raw

In reply to Peter Zijlstra

Quoted 3 lines · Peter Zijlstra

[...]
+ * We want to touch writable mappings with a write fault in otder
[...]

otder -> order

Greetings
Mauro