Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660209 > unrolled thread

[PATCH 1/3] mm: add vm_insert_mixed_mkwrite()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] mm: add vm_insert_mixed_mkwrite() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-06-07 23:00 +0200
    [PATCH 2/3] dax: relocate dax_load_hole() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-06-07 23:00 +0200
    Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite() Dan Williams <dan.j.williams@intel.com> - 2017-06-09 23:30 +0200
      Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-06-10 05:10 +0200
        Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite() Dan Williams <dan.j.williams@intel.com> - 2017-06-10 05:40 +0200
          Re: [PATCH 1/3] mm: add vm_insert_mixed_mkwrite() Ross Zwisler <ross.zwisler@linux.intel.com> - 2017-06-12 06:10 +0200

#1660209 — [PATCH 1/3] mm: add vm_insert_mixed_mkwrite()

To be able to use the common 4k zero page in DAX we need to have our PTE
fault path look more like our PMD fault path where a PTE entry can be
marked as dirty and writeable as it is first inserted, rather than waiting
for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.

Right now we can rely on having a dax_pfn_mkwrite() call because we can
distinguish between these two cases in do_wp_page():

	case 1: 4k zero page => writable DAX storage
	case 2: read-only DAX storage => writeable DAX storage

This distinction is made by via vm_normal_page().  vm_normal_page() returns
false for the common 4k zero page, though, just as it does for DAX ptes.
Instead of special casing the DAX + 4k zero page case, we will simplify our
DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
get rid of dax_pfn_mkwrite() completely.

This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
will do the work that was previously done by wp_page_reuse() as part of the
dax_pfn_mkwrite() call path.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 include/linux/mm.h |  9 +++++++--
 mm/memory.c        | 21 ++++++++++++++-------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index b892e95..11e323a 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 			unsigned long pfn);
 int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
 			unsigned long pfn, pgprot_t pgprot);
-int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
-			pfn_t pfn);
+int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
+			pfn_t pfn, bool mkwrite);
 int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
 
+static inline int vm_insert_mixed(struct vm_area_struct *vma,
+		unsigned long addr, pfn_t pfn)
+{
+	return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
+}
 
 struct page *follow_page_mask(struct vm_area_struct *vma,
 			      unsigned long address, unsigned int foll_flags,
diff --git a/mm/memory.c b/mm/memory.c
index 2e65df1..8d0eef6 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1646,7 +1646,7 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
 EXPORT_SYMBOL(vm_insert_page);
 
 static int insert_pfn(struct vm_area_struct *vma, unsigned long addr,
-			pfn_t pfn, pgprot_t prot)
+			pfn_t pfn, pgprot_t prot, bool mkwrite)
 {
 	struct mm_struct *mm = vma->vm_mm;
 	int retval;
@@ -1658,7 +1658,7 @@ static int insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 	if (!pte)
 		goto out;
 	retval = -EBUSY;
-	if (!pte_none(*pte))
+	if (!pte_none(*pte) && !mkwrite)
 		goto out_unlock;
 
 	/* Ok, finally just insert the thing.. */
@@ -1666,6 +1666,12 @@ static int insert_pfn(struct vm_area_struct *vma, unsigned long addr,
 		entry = pte_mkdevmap(pfn_t_pte(pfn, prot));
 	else
 		entry = pte_mkspecial(pfn_t_pte(pfn, prot));
+
+	if (mkwrite) {
+		entry = pte_mkyoung(entry);
+		entry = maybe_mkwrite(pte_mkdirty(entry), vma);
+	}
+
 	set_pte_at(mm, addr, pte, entry);
 	update_mmu_cache(vma, addr, pte); /* XXX: why not for insert_page? */
 
@@ -1736,14 +1742,15 @@ int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
 
 	track_pfn_insert(vma, &pgprot, __pfn_to_pfn_t(pfn, PFN_DEV));
 
-	ret = insert_pfn(vma, addr, __pfn_to_pfn_t(pfn, PFN_DEV), pgprot);
+	ret = insert_pfn(vma, addr, __pfn_to_pfn_t(pfn, PFN_DEV), pgprot,
+			false);
 
 	return ret;
 }
 EXPORT_SYMBOL(vm_insert_pfn_prot);
 
-int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
-			pfn_t pfn)
+int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
+			pfn_t pfn, bool mkwrite)
 {
 	pgprot_t pgprot = vma->vm_page_prot;
 
@@ -1772,9 +1779,9 @@ int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
 		page = pfn_to_page(pfn_t_to_pfn(pfn));
 		return insert_page(vma, addr, page, pgprot);
 	}
-	return insert_pfn(vma, addr, pfn, pgprot);
+	return insert_pfn(vma, addr, pfn, pgprot, mkwrite);
 }
-EXPORT_SYMBOL(vm_insert_mixed);
+EXPORT_SYMBOL(vm_insert_mixed_mkwrite);
 
 /*
  * maps a range of physical memory into the requested pages. the old
-- 
2.9.4

[toc] | [next] | [standalone]

#1660217 — [PATCH 2/3] dax: relocate dax_load_hole()

dax_load_hole() will soon need to call dax_insert_mapping_entry(), so it
needs to be moved lower in dax.c so the definition exists.

Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
 fs/dax.c | 88 ++++++++++++++++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 44 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index 2a6889b..66e0e93 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -469,50 +469,6 @@ int dax_invalidate_mapping_entry_sync(struct address_space *mapping,
 	return __dax_invalidate_mapping_entry(mapping, index, false);
 }
 
-/*
- * The user has performed a load from a hole in the file.  Allocating
- * a new page in the file would cause excessive storage usage for
- * workloads with sparse files.  We allocate a page cache page instead.
- * We'll kick it out of the page cache if it's ever written to,
- * otherwise it will simply fall out of the page cache under memory
- * pressure without ever having been dirtied.
- */
-static int dax_load_hole(struct address_space *mapping, void **entry,
-			 struct vm_fault *vmf)
-{
-	struct inode *inode = mapping->host;
-	struct page *page;
-	int ret;
-
-	/* Hole page already exists? Return it...  */
-	if (!radix_tree_exceptional_entry(*entry)) {
-		page = *entry;
-		goto finish_fault;
-	}
-
-	/* This will replace locked radix tree entry with a hole page */
-	page = find_or_create_page(mapping, vmf->pgoff,
-				   vmf->gfp_mask | __GFP_ZERO);
-	if (!page) {
-		ret = VM_FAULT_OOM;
-		goto out;
-	}
-
-finish_fault:
-	vmf->page = page;
-	ret = finish_fault(vmf);
-	vmf->page = NULL;
-	*entry = page;
-	if (!ret) {
-		/* Grab reference for PTE that is now referencing the page */
-		get_page(page);
-		ret = VM_FAULT_NOPAGE;
-	}
-out:
-	trace_dax_load_hole(inode, vmf, ret);
-	return ret;
-}
-
 static int copy_user_dax(struct block_device *bdev, struct dax_device *dax_dev,
 		sector_t sector, size_t size, struct page *to,
 		unsigned long vaddr)
@@ -936,6 +892,50 @@ int dax_pfn_mkwrite(struct vm_fault *vmf)
 }
 EXPORT_SYMBOL_GPL(dax_pfn_mkwrite);
 
+/*
+ * The user has performed a load from a hole in the file.  Allocating
+ * a new page in the file would cause excessive storage usage for
+ * workloads with sparse files.  We allocate a page cache page instead.
+ * We'll kick it out of the page cache if it's ever written to,
+ * otherwise it will simply fall out of the page cache under memory
+ * pressure without ever having been dirtied.
+ */
+static int dax_load_hole(struct address_space *mapping, void **entry,
+			 struct vm_fault *vmf)
+{
+	struct inode *inode = mapping->host;
+	struct page *page;
+	int ret;
+
+	/* Hole page already exists? Return it...  */
+	if (!radix_tree_exceptional_entry(*entry)) {
+		page = *entry;
+		goto finish_fault;
+	}
+
+	/* This will replace locked radix tree entry with a hole page */
+	page = find_or_create_page(mapping, vmf->pgoff,
+				   vmf->gfp_mask | __GFP_ZERO);
+	if (!page) {
+		ret = VM_FAULT_OOM;
+		goto out;
+	}
+
+finish_fault:
+	vmf->page = page;
+	ret = finish_fault(vmf);
+	vmf->page = NULL;
+	*entry = page;
+	if (!ret) {
+		/* Grab reference for PTE that is now referencing the page */
+		get_page(page);
+		ret = VM_FAULT_NOPAGE;
+	}
+out:
+	trace_dax_load_hole(inode, vmf, ret);
+	return ret;
+}
+
 static bool dax_range_is_aligned(struct block_device *bdev,
 				 unsigned int offset, unsigned int length)
 {
-- 
2.9.4

[toc] | [prev] | [next] | [standalone]

#1662760

On Wed, Jun 7, 2017 at 1:48 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> To be able to use the common 4k zero page in DAX we need to have our PTE
> fault path look more like our PMD fault path where a PTE entry can be
> marked as dirty and writeable as it is first inserted, rather than waiting
> for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
>
> Right now we can rely on having a dax_pfn_mkwrite() call because we can
> distinguish between these two cases in do_wp_page():
>
>         case 1: 4k zero page => writable DAX storage
>         case 2: read-only DAX storage => writeable DAX storage
>
> This distinction is made by via vm_normal_page().  vm_normal_page() returns
> false for the common 4k zero page, though, just as it does for DAX ptes.
> Instead of special casing the DAX + 4k zero page case, we will simplify our
> DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
> get rid of dax_pfn_mkwrite() completely.
>
> This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
> and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
> will do the work that was previously done by wp_page_reuse() as part of the
> dax_pfn_mkwrite() call path.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  include/linux/mm.h |  9 +++++++--
>  mm/memory.c        | 21 ++++++++++++++-------
>  2 files changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index b892e95..11e323a 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>                         unsigned long pfn);
>  int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
>                         unsigned long pfn, pgprot_t pgprot);
> -int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
> -                       pfn_t pfn);
> +int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> +                       pfn_t pfn, bool mkwrite);

Are there any other planned public users of vm_insert_mixed_mkwrite()
that would pass false? I think not.

>  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
>
> +static inline int vm_insert_mixed(struct vm_area_struct *vma,
> +               unsigned long addr, pfn_t pfn)
> +{
> +       return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
> +}

...in other words instead of making the distinction of
vm_insert_mixed_mkwrite() and vm_insert_mixed() with extra flag
argument just move the distinction into mm/memory.c directly.

So, the prototype remains the same as vm_insert_mixed()

int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long
addr, pfn_t pfn);

...and only static insert_pfn(...) needs to change.

[toc] | [prev] | [next] | [standalone]

#1662860

On Fri, Jun 09, 2017 at 02:23:51PM -0700, Dan Williams wrote:
> On Wed, Jun 7, 2017 at 1:48 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > To be able to use the common 4k zero page in DAX we need to have our PTE
> > fault path look more like our PMD fault path where a PTE entry can be
> > marked as dirty and writeable as it is first inserted, rather than waiting
> > for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
> >
> > Right now we can rely on having a dax_pfn_mkwrite() call because we can
> > distinguish between these two cases in do_wp_page():
> >
> >         case 1: 4k zero page => writable DAX storage
> >         case 2: read-only DAX storage => writeable DAX storage
> >
> > This distinction is made by via vm_normal_page().  vm_normal_page() returns
> > false for the common 4k zero page, though, just as it does for DAX ptes.
> > Instead of special casing the DAX + 4k zero page case, we will simplify our
> > DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
> > get rid of dax_pfn_mkwrite() completely.
> >
> > This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
> > and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
> > will do the work that was previously done by wp_page_reuse() as part of the
> > dax_pfn_mkwrite() call path.
> >
> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > ---
> >  include/linux/mm.h |  9 +++++++--
> >  mm/memory.c        | 21 ++++++++++++++-------
> >  2 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index b892e95..11e323a 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
> >                         unsigned long pfn);
> >  int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
> >                         unsigned long pfn, pgprot_t pgprot);
> > -int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
> > -                       pfn_t pfn);
> > +int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
> > +                       pfn_t pfn, bool mkwrite);
> 
> Are there any other planned public users of vm_insert_mixed_mkwrite()
> that would pass false? I think not.
> 
> >  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
> >
> > +static inline int vm_insert_mixed(struct vm_area_struct *vma,
> > +               unsigned long addr, pfn_t pfn)
> > +{
> > +       return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
> > +}
> 
> ...in other words instead of making the distinction of
> vm_insert_mixed_mkwrite() and vm_insert_mixed() with extra flag
> argument just move the distinction into mm/memory.c directly.
> 
> So, the prototype remains the same as vm_insert_mixed()
> 
> int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long
> addr, pfn_t pfn);
> 
> ...and only static insert_pfn(...) needs to change.

My usage of vm_insert_mixed_mkwrite() in fs/dax.c needs the mkwrite flag to be
there.  From dax_insert_mapping():

        return vm_insert_mixed_mkwrite(vma, vaddr, pfn,
	                        vmf->flags & FAULT_FLAG_WRITE);

So, yes, we could do what you suggest, but then that code becomes:

	if (vmf->flags & FAULT_FLAG_WRITE)
		vm_insert_mixed_mkwrite(vma, vaddr, pfn);
	else
		vm_insert_mixed(vma, vaddr, pfn);

And vm_insert_mixed_mkwrite() and vm_insert_mixed() are redundant with only
the insert_pfn() line differing?  This doesn't seem better...unless I'm
missing something?

The way it is, vm_insert_mixed_mkwrite() also closely matches
insert_pfn_pmd(), which we use in the PMD case and which also takes a 'write'
boolean which works the same as our newly added 'mkwrite'.

[toc] | [prev] | [next] | [standalone]

#1662862

On Fri, Jun 9, 2017 at 8:03 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Fri, Jun 09, 2017 at 02:23:51PM -0700, Dan Williams wrote:
>> On Wed, Jun 7, 2017 at 1:48 PM, Ross Zwisler
>> <ross.zwisler@linux.intel.com> wrote:
>> > To be able to use the common 4k zero page in DAX we need to have our PTE
>> > fault path look more like our PMD fault path where a PTE entry can be
>> > marked as dirty and writeable as it is first inserted, rather than waiting
>> > for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
>> >
>> > Right now we can rely on having a dax_pfn_mkwrite() call because we can
>> > distinguish between these two cases in do_wp_page():
>> >
>> >         case 1: 4k zero page => writable DAX storage
>> >         case 2: read-only DAX storage => writeable DAX storage
>> >
>> > This distinction is made by via vm_normal_page().  vm_normal_page() returns
>> > false for the common 4k zero page, though, just as it does for DAX ptes.
>> > Instead of special casing the DAX + 4k zero page case, we will simplify our
>> > DAX PTE page fault sequence so that it matches our DAX PMD sequence, and
>> > get rid of dax_pfn_mkwrite() completely.
>> >
>> > This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
>> > and allow us to pass in a 'mkwrite' flag.  If 'mkwrite' is set insert_pfn()
>> > will do the work that was previously done by wp_page_reuse() as part of the
>> > dax_pfn_mkwrite() call path.
>> >
>> > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
>> > ---
>> >  include/linux/mm.h |  9 +++++++--
>> >  mm/memory.c        | 21 ++++++++++++++-------
>> >  2 files changed, 21 insertions(+), 9 deletions(-)
>> >
>> > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > index b892e95..11e323a 100644
>> > --- a/include/linux/mm.h
>> > +++ b/include/linux/mm.h
>> > @@ -2294,10 +2294,15 @@ int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
>> >                         unsigned long pfn);
>> >  int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr,
>> >                         unsigned long pfn, pgprot_t pgprot);
>> > -int vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr,
>> > -                       pfn_t pfn);
>> > +int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long addr,
>> > +                       pfn_t pfn, bool mkwrite);
>>
>> Are there any other planned public users of vm_insert_mixed_mkwrite()
>> that would pass false? I think not.
>>
>> >  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long len);
>> >
>> > +static inline int vm_insert_mixed(struct vm_area_struct *vma,
>> > +               unsigned long addr, pfn_t pfn)
>> > +{
>> > +       return vm_insert_mixed_mkwrite(vma, addr, pfn, false);
>> > +}
>>
>> ...in other words instead of making the distinction of
>> vm_insert_mixed_mkwrite() and vm_insert_mixed() with extra flag
>> argument just move the distinction into mm/memory.c directly.
>>
>> So, the prototype remains the same as vm_insert_mixed()
>>
>> int vm_insert_mixed_mkwrite(struct vm_area_struct *vma, unsigned long
>> addr, pfn_t pfn);
>>
>> ...and only static insert_pfn(...) needs to change.
>
> My usage of vm_insert_mixed_mkwrite() in fs/dax.c needs the mkwrite flag to be
> there.  From dax_insert_mapping():
>
>         return vm_insert_mixed_mkwrite(vma, vaddr, pfn,
>                                 vmf->flags & FAULT_FLAG_WRITE);

Ok, I missed that.

>
> So, yes, we could do what you suggest, but then that code becomes:
>
>         if (vmf->flags & FAULT_FLAG_WRITE)
>                 vm_insert_mixed_mkwrite(vma, vaddr, pfn);
>         else
>                 vm_insert_mixed(vma, vaddr, pfn);

I think that's more readable / greppable.

>
> And vm_insert_mixed_mkwrite() and vm_insert_mixed() are redundant with only
> the insert_pfn() line differing?  This doesn't seem better...unless I'm
> missing something?
>
> The way it is, vm_insert_mixed_mkwrite() also closely matches
> insert_pfn_pmd(), which we use in the PMD case and which also takes a 'write'
> boolean which works the same as our newly added 'mkwrite'.

Hmm, but now the pfn and pmd cases are inconsistent, if you put the
flag name in the function then don't add an argument, or make it like
the pmd case and add an argument to vm_insert_mixed(). I prefer the
former.

[toc] | [prev] | [next] | [standalone]

#1663235

On Fri, Jun 09, 2017 at 08:35:08PM -0700, Dan Williams wrote:
> On Fri, Jun 9, 2017 at 8:03 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > And vm_insert_mixed_mkwrite() and vm_insert_mixed() are redundant with only
> > the insert_pfn() line differing?  This doesn't seem better...unless I'm
> > missing something?
> >
> > The way it is, vm_insert_mixed_mkwrite() also closely matches
> > insert_pfn_pmd(), which we use in the PMD case and which also takes a 'write'
> > boolean which works the same as our newly added 'mkwrite'.
> 
> Hmm, but now the pfn and pmd cases are inconsistent, if you put the
> flag name in the function then don't add an argument, or make it like
> the pmd case and add an argument to vm_insert_mixed(). I prefer the
> former.

Okay, I'll fix this for v2.  Thanks for the review.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web