Allow single pagefault in write access of a VM_MIXEDMAP mapping

PATCH In review Boaz Harrosh 3 participants 8 messages Jan 10, 2016 – Jan 12, 2016

Subsystems (4)

Memory management VFS MM core Filesystem direct access (DAX)

lore

Share & tools

lore

Boaz Harrosh

author

2016-01-10 13:59 UTC #0 lore raw

cover Allow single pagefault in write access of a VM_MIXEDMAP mapping

Today any VM_MIXEDMAP or VM_PFN mapping when enabling a write access to their mapping, will have a double pagefault for every write access.

This is because vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).

Which means that it is always inserted with read-only under the assumption that we want to be notified when write access occurs.

But this is not always true and adds an unnecessary page-fault on every new mmap-write access

This patchset is trying to give the fault handler more choice by passing an pgprot_t to vm_insert_mixed() via a new vm_insert_mixed_prot() API.

If the mm guys feel that the pgprot_t and its helpers and flags are private to mm/memory.c I can easily do a new: vm_insert_mixed_rw() instead. of the above vm_insert_mixed_prot() which enables any control not only write.

Following is a patch to DAX to optimize out the extra page-fault.

TODO: I only did 4k mapping perhaps 2M mapping can enjoy the same single fault on write access. If interesting to anyone I can attempt a fix.

Dan Andrew who needs to pick this up please?

list of patches:
[PATCH 1/2] mm: Allow single pagefault on mmap-write with VM_MIXEDMAP [PATCH 2/2] dax: Only fault once on mmap write access

Thank you
Boaz

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

Boaz Harrosh

author

2016-01-10 14:02 UTC #1 lore raw

PATCH 1/2 mm: Allow single pagefault on mmap-write with VM_MIXEDMAP

Until now vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).

Which meant that it was always inserted with read-only under the assumption that we want to be notified when write access occurs. This is not always true and adds an unnecessary page-fault on every new mmap-write.

This patch adds a more granular approach and lets the fault handler decide how it wants to map the mixmap pfn.

The old vm_insert_mixed() now receives a new pgprot_t prot and is renamed to: vm_insert_mixed_prot().
A new inline vm_insert_mixed() is defined which is a wrapper over vm_insert_mixed_prot(), with the vma->vm_page_prot default as before, so to satisfy all current users.

CC Andrew Morton <[email protected]>

CC "Kirill A. Shutemov" <[email protected]>

CC Oleg Nesterov <[email protected]>

CC Mel Gorman <[email protected]>

CC Johannes Weiner <[email protected]>

CC Matthew Wilcox <[email protected]>

CC [email protected] (open list:MEMORY MANAGEMENT)

Reviewed-by Yigal Korman <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 include/linux/mm.h |  8 +++++++-
 mm/memory.c        | 10 +++++-----
 2 files changed, 12 insertions(+), 6 deletions(-)

include/linux/mm.h +7 -1

index 80001de..46a9a19 100644
@@ -2108,8 +2108,14 @@ int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
 int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 			unsigned long pfn);
+int vm_insert_mixed_prot(struct vm_area_struct *vma, unsigned long addr,
+			 unsigned long pfn, pgprot_t prot);
+static inline
 int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
-			unsigned long pfn);
+		    unsigned long pfn)
+{
+	return vm_insert_mixed_prot(vma, addr, pfn, vma->vm_page_prot);
+}
 int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);

mm/memory.c +5 -5

index deb679c..c716913 100644
@@ -1589,8 +1589,8 @@ int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 }
 EXPORT_SYMBOL(vm_insert_pfn);
 
-int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
-			unsigned long pfn)
+int vm_insert_mixed_prot(struct vm_area_struct *vma, unsigned long addr,
+			 unsigned long pfn, pgprot_t prot)
 {
 	BUG_ON(!(vma->vm_flags & VM_MIXEDMAP));

@@ -1608,11 +1608,11 @@ int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
 		struct page *page;
 
 		page = pfn_to_page(pfn);
-		return insert_page(vma, addr, page, vma->vm_page_prot);
+		return insert_page(vma, addr, page, prot);
 	}
-	return insert_pfn(vma, addr, pfn, vma->vm_page_prot);
+	return insert_pfn(vma, addr, pfn, prot);
 }
-EXPORT_SYMBOL(vm_insert_mixed);
+EXPORT_SYMBOL(vm_insert_mixed_prot);
 
 /*
  * maps a range of physical memory into the requested pages. the old

Signature · 8 lines

1.9.3


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

Boaz Harrosh

author

2016-01-10 14:03 UTC #2 lore raw

PATCH 2/2 dax: Only fault once on mmap write access

In reply to Boaz Harrosh

In current code for any mmap-write access there are two page faults. One that maps the pfn into the vma (vm_insert_mixed()), and a second one that converts the read-only mapping to read-write (via pfn_mkwrite).

But since we already know that this is a write access we can map the pfn read-write and save the extra fault.

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/dax.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

fs/dax.c +5 -1

index a86d3cc..3fee696 100644
@@ -289,6 +289,7 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 	sector_t sector = bh->b_blocknr << (inode->i_blkbits - 9);
 	unsigned long vaddr = (unsigned long)vmf->virtual_address;
 	void __pmem *addr;
+	pgprot_t prot = vma->vm_page_prot;
 	unsigned long pfn;
 	pgoff_t size;
 	int error;

@@ -321,7 +322,10 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 		wmb_pmem();
 	}
 
-	error = vm_insert_mixed(vma, vaddr, pfn);
+	if (vmf->flags & FAULT_FLAG_WRITE)
+		prot = pgprot_modify(prot, PAGE_SHARED);
+
+	error = vm_insert_mixed_prot(vma, vaddr, pfn, prot);
 
  out:
 	i_mmap_unlock_read(mapping);

Signature · 8 lines

1.9.3


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

Dan Williams

2016-01-11 01:19 UTC #3 lore raw

In reply to Boaz Harrosh

Quoted 21 lines · Boaz Harrosh

Hi
Today any VM_MIXEDMAP or VM_PFN mapping when enabling a write access to their mapping, will have a double pagefault for every write access.
This is because vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).
Which means that it is always inserted with read-only under the assumption that we want to be notified when write access occurs.
But this is not always true and adds an unnecessary page-fault on every new mmap-write access
This patchset is trying to give the fault handler more choice by passing an pgprot_t to vm_insert_mixed() via a new vm_insert_mixed_prot() API.
If the mm guys feel that the pgprot_t and its helpers and flags are private to mm/memory.c I can easily do a new: vm_insert_mixed_rw() instead. of the above vm_insert_mixed_prot() which enables any control not only write.

Quoted 6 lines

Following is a patch to DAX to optimize out the extra page-fault.
TODO: I only did 4k mapping perhaps 2M mapping can enjoy the same single fault on write access. If interesting to anyone I can attempt a fix.
Dan Andrew who needs to pick this up please?

This collides with the patches currently pending in -mm for 4.5, lets take a look at this for 4.6.

Boaz Harrosh

author

2016-01-11 09:22 UTC #4 lore raw

Quoted 24 lines · Dan Williams

Quoted 23 lines · Boaz Harrosh
Hi
Today any VM_MIXEDMAP or VM_PFN mapping when enabling a write access to their mapping, will have a double pagefault for every write access.
This is because vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).
Which means that it is always inserted with read-only under the assumption that we want to be notified when write access occurs.
But this is not always true and adds an unnecessary page-fault on every new mmap-write access
This patchset is trying to give the fault handler more choice by passing an pgprot_t to vm_insert_mixed() via a new vm_insert_mixed_prot() API.
If the mm guys feel that the pgprot_t and its helpers and flags are private to mm/memory.c I can easily do a new: vm_insert_mixed_rw() instead. of the above vm_insert_mixed_prot() which enables any control not only write.
Following is a patch to DAX to optimize out the extra page-fault.

Quoted 8 lines

Quoted 4 lines
TODO: I only did 4k mapping perhaps 2M mapping can enjoy the same single fault on write access. If interesting to anyone I can attempt a fix.
Dan Andrew who needs to pick this up please?
This collides with the patches currently pending in -mm for 4.5, lets take a look at this for 4.6.

OK thanks, I will try to work this over current linux-next and sure we will wait for 4.5-rc1 to look at this again.

Do you have any comments in general about this?

Thanks
Boaz

Dan Williams

2016-01-11 16:37 UTC #5 lore raw

Quoted 30 lines · Boaz Harrosh

Quoted 29 lines · Dan Williams
Quoted 27 lines · Boaz Harrosh
Hi
Today any VM_MIXEDMAP or VM_PFN mapping when enabling a write access to their mapping, will have a double pagefault for every write access.
This is because vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).
Which means that it is always inserted with read-only under the assumption that we want to be notified when write access occurs.
But this is not always true and adds an unnecessary page-fault on every new mmap-write access
This patchset is trying to give the fault handler more choice by passing an pgprot_t to vm_insert_mixed() via a new vm_insert_mixed_prot() API.
If the mm guys feel that the pgprot_t and its helpers and flags are private to mm/memory.c I can easily do a new: vm_insert_mixed_rw() instead. of the above vm_insert_mixed_prot() which enables any control not only write.
Following is a patch to DAX to optimize out the extra page-fault.
TODO: I only did 4k mapping perhaps 2M mapping can enjoy the same single fault on write access. If interesting to anyone I can attempt a fix.
Dan Andrew who needs to pick this up please?

Quoted 8 lines

Quoted 3 lines
This collides with the patches currently pending in -mm for 4.5, lets take a look at this for 4.6.
OK thanks, I will try to work this over current linux-next and sure we will wait for 4.5-rc1 to look at this again.
Do you have any comments in general about this?

Looks worthwhile at first glance, the only concern that comes to mind is integration with Ross' fsync/msync enabling. How much does this change matter in practice? If the mapping is long standing then I expect this cost gets hidden?

Matthew Wilcox

2016-01-11 19:35 UTC #6 lore raw

In reply to Boaz Harrosh

Quoted 12 lines · Boaz Harrosh

Today any VM_MIXEDMAP or VM_PFN mapping when enabling a write access to their mapping, will have a double pagefault for every write access.
This is because vma->vm_page_prot defines how a page/pfn is inserted into the page table (see vma_wants_writenotify in mm/mmap.c).
Which means that it is always inserted with read-only under the assumption that we want to be notified when write access occurs.
But this is not always true and adds an unnecessary page-fault on every new mmap-write access

Quoted 6 lines

This patchset is trying to give the fault handler more choice by passing an pgprot_t to vm_insert_mixed() via a new vm_insert_mixed_prot() API.
If the mm guys feel that the pgprot_t and its helpers and flags are private to mm/memory.c I can easily do a new: vm_insert_mixed_rw() instead. of the above vm_insert_mixed_prot() which enables any control not only write.

We've known about this one for a while, and it's never been terribly high on the priority list to fix it. This is the obvious way to fix it but, as you note, it might be seen as increasing the leak between the abstractions.

I would rather see the memory.c code move in the direction of the huge_memory.c code. How about something like this?

fs/dax.c +7 -6

index a610cbe..09b6c8c 100644
@@ -498,6 +498,7 @@ EXPORT_SYMBOL_GPL(dax_writeback_mapping_range);
 static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 			struct vm_area_struct *vma, struct vm_fault *vmf)
 {
+	const bool write = vmf->flags & FAULT_FLAG_WRITE;
 	unsigned long vaddr = (unsigned long)vmf->virtual_address;
 	struct address_space *mapping = inode->i_mapping;
 	struct block_device *bdev = bh->b_bdev;

@@ -534,12 +535,11 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 	}
 	dax_unmap_atomic(bdev, &dax);
 
-	error = dax_radix_entry(mapping, vmf->pgoff, dax.sector, false,
-			vmf->flags & FAULT_FLAG_WRITE);
+	error = dax_radix_entry(mapping, vmf->pgoff, dax.sector, false, write);
 	if (error)
 		goto out;
 
-	error = vm_insert_mixed(vma, vaddr, dax.pfn);
+	error = vmf_insert_pfn(vma, vaddr, dax.pfn, write);
 
  out:
 	i_mmap_unlock_read(mapping);

@@ -559,7 +559,7 @@ static int dax_pte_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
 	unsigned blkbits = inode->i_blkbits;
 	sector_t block;
 	pgoff_t size;
-	int error;
+	int result, error;
 	int major = 0;
 
 	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;

@@ -660,13 +660,14 @@ static int dax_pte_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
 	 * indicate what the callback should do via the uptodate variable, same
 	 * as for normal BH based IO completions.
 	 */
-	error = dax_insert_mapping(inode, &bh, vma, vmf);
+	result = dax_insert_mapping(inode, &bh, vma, vmf);
 	if (buffer_unwritten(&bh)) {
 		if (complete_unwritten)
-			complete_unwritten(&bh, !error);
+			complete_unwritten(&bh, !(result & VM_FAULT_ERROR));
 		else
 			WARN_ON_ONCE(!(vmf->flags & FAULT_FLAG_WRITE));
 	}
+	return result | major;
 
  out:
 	if (error == -ENOMEM)

include/linux/mm.h +3 -1

index 27dbd1b..a95242c 100644
@@ -2170,8 +2170,10 @@ struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn, unsigned long size, pgprot_t);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
-int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_pfn(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn);
+int vmf_insert_pfn(struct vm_area_struct *, unsigned long addr,
+			pfn_t pfn, bool write);
 int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
 			pfn_t pfn);
 int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);

mm/memory.c +39 ~~-13~~

102 lines not on this page · raw message

Matthew Wilcox

2016-01-12 13:30 UTC #7 lore raw

Quoted 2 lines · Matthew Wilcox

I would rather see the memory.c code move in the direction of the huge_memory.c code. How about something like this?

Whoops, missed some bits in the DAX conversion where we'd return an -errno instead of VM_FAULT flags. Take two.

fs/dax.c +18 ~~-18~~

index a610cbe..deff70f 100644
@@ -498,6 +498,7 @@ EXPORT_SYMBOL_GPL(dax_writeback_mapping_range);
 static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 			struct vm_area_struct *vma, struct vm_fault *vmf)
 {
+	const bool write = vmf->flags & FAULT_FLAG_WRITE;
 	unsigned long vaddr = (unsigned long)vmf->virtual_address;
 	struct address_space *mapping = inode->i_mapping;
 	struct block_device *bdev = bh->b_bdev;

@@ -506,7 +507,7 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 		.size = bh->b_size,
 	};
 	pgoff_t size;
-	int error;
+	int result;
 
 	i_mmap_lock_read(mapping);

@@ -518,15 +519,11 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 	 * allocated past the end of the file.
 	 */
 	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	if (unlikely(vmf->pgoff >= size)) {
-		error = -EIO;
-		goto out;
-	}
+	if (unlikely(vmf->pgoff >= size))
+		goto sigbus;
 
-	if (dax_map_atomic(bdev, &dax) < 0) {
-		error = PTR_ERR(dax.addr);
-		goto out;
-	}
+	if (dax_map_atomic(bdev, &dax) < 0)
+		goto sigbus;
 
 	if (buffer_unwritten(bh) || buffer_new(bh)) {
 		clear_pmem(dax.addr, PAGE_SIZE);

@@ -534,17 +531,19 @@ static int dax_insert_mapping(struct inode *inode, struct buffer_head *bh,
 	}
 	dax_unmap_atomic(bdev, &dax);
 
-	error = dax_radix_entry(mapping, vmf->pgoff, dax.sector, false,
-			vmf->flags & FAULT_FLAG_WRITE);
-	if (error)
-		goto out;
+	if (dax_radix_entry(mapping, vmf->pgoff, dax.sector, false, write))
+		goto sigbus;
 
-	error = vm_insert_mixed(vma, vaddr, dax.pfn);
+	result = vmf_insert_pfn(vma, vaddr, dax.pfn, write);
 
  out:
 	i_mmap_unlock_read(mapping);
 
-	return error;
+	return result;
+
+ sigbus:
+	result = VM_FAULT_SIGBUS;
+	goto out;
 }
 
 static int dax_pte_fault(struct vm_area_struct *vma, struct vm_fault *vmf,

@@ -559,7 +558,7 @@ static int dax_pte_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
 	unsigned blkbits = inode->i_blkbits;
 	sector_t block;
 	pgoff_t size;
-	int error;
+	int result, error;
 	int major = 0;
 
 	size = (i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT;

@@ -660,13 +659,14 @@ static int dax_pte_fault(struct vm_area_struct *vma, struct vm_fault *vmf,
 	 * indicate what the callback should do via the uptodate variable, same
 	 * as for normal BH based IO completions.
 	 */
-	error = dax_insert_mapping(inode, &bh, vma, vmf);
+	result = dax_insert_mapping(inode, &bh, vma, vmf);
 	if (buffer_unwritten(&bh)) {
 		if (complete_unwritten)
-			complete_unwritten(&bh, !error);
+			complete_unwritten(&bh, !(result & VM_FAULT_ERROR));
 		else
 			WARN_ON_ONCE(!(vmf->flags & FAULT_FLAG_WRITE));
 	}
+	return result | major;
 
  out:
 	if (error == -ENOMEM)

include/linux/mm.h +3 -1

index 27dbd1b..a95242c 100644
@@ -2170,8 +2170,10 @@ struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn, unsigned long size, pgprot_t);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
-int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_pfn(struct vm_area_struct *, unsigned long addr,
 			unsigned long pfn);
+int vmf_insert_pfn(struct vm_area_struct *, unsigned long addr,
+			pfn_t pfn, bool write);
 int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
 			pfn_t pfn);
 int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);

mm/memory.c +39 ~~-13~~

102 lines not on this page · raw message