[PATCH v3] dax: Change return type to vm_fault_t

[PATCH v3] dax: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for fault and huge_fault
handler. For now, this is just documenting that the
function returns a VM_FAULT value rather than an errno.
Once all instances are converted, vm_fault_t will become
a distinct type.

Reference id -> 1c8f422059ae ("mm: change return type to
vm_fault_t")

Previously vm_insert_mixed() returns err which driver
mapped into VM_FAULT_* type. The new function 
vmf_insert_mixed() will replace this inefficiency by
returning VM_FAULT_* type.

Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
---
v2: Modified the change log

v3: Updated the change log and
    added Ross in review list

 drivers/dax/device.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 2137dbc..a122701 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -243,11 +243,11 @@ __weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff,
 	return -1;
 }
 
-static int __dev_dax_pte_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
+static vm_fault_t __dev_dax_pte_fault(struct dev_dax *dev_dax,
+				struct vm_fault *vmf)
 {
 	struct device *dev = &dev_dax->dev;
 	struct dax_region *dax_region;
-	int rc = VM_FAULT_SIGBUS;
 	phys_addr_t phys;
 	pfn_t pfn;
 	unsigned int fault_size = PAGE_SIZE;
@@ -274,17 +274,11 @@ static int __dev_dax_pte_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
 
 	pfn = phys_to_pfn_t(phys, dax_region->pfn_flags);
 
-	rc = vm_insert_mixed(vmf->vma, vmf->address, pfn);
-
-	if (rc == -ENOMEM)
-		return VM_FAULT_OOM;
-	if (rc < 0 && rc != -EBUSY)
-		return VM_FAULT_SIGBUS;
-
-	return VM_FAULT_NOPAGE;
+	return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
 }
 
-static int __dev_dax_pmd_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
+static vm_fault_t __dev_dax_pmd_fault(struct dev_dax *dev_dax,
+				struct vm_fault *vmf)
 {
 	unsigned long pmd_addr = vmf->address & PMD_MASK;
 	struct device *dev = &dev_dax->dev;
@@ -335,7 +329,8 @@ static int __dev_dax_pmd_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
 }
 
 #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
-static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
+static vm_fault_t __dev_dax_pud_fault(struct dev_dax *dev_dax,
+				struct vm_fault *vmf)
 {
 	unsigned long pud_addr = vmf->address & PUD_MASK;
 	struct device *dev = &dev_dax->dev;
@@ -386,13 +381,14 @@ static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
 			vmf->flags & FAULT_FLAG_WRITE);
 }
 #else
-static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
+static vm_fault_t __dev_dax_pud_fault(struct dev_dax *dev_dax,
+				struct vm_fault *vmf)
 {
 	return VM_FAULT_FALLBACK;
 }
 #endif /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
 
-static int dev_dax_huge_fault(struct vm_fault *vmf,
+static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
 		enum page_entry_size pe_size)
 {
 	int rc, id;
@@ -423,7 +419,7 @@ static int dev_dax_huge_fault(struct vm_fault *vmf,
 	return rc;
 }
 
-static int dev_dax_fault(struct vm_fault *vmf)
+static vm_fault_t dev_dax_fault(struct vm_fault *vmf)
 {
 	return dev_dax_huge_fault(vmf, PE_SIZE_PTE);
 }
-- 
1.9.1

_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] dax: Change return type to vm_fault_t

[Souptick Joarder]

Hi Matthew/ Ross,

There are two changes exist in mm/huge_memory.c as part of this
patch. vmf_insert_pfn_pmd() and vmf_insert_pfn_pud() functions are
invoked from this patch.

Shall we put both in a single patch that it will easy to bisect in case
we have any issue ?

On Tue, Apr 17, 2018 at 6:58 PM, Souptick Joarder <jrdr.linux@gmail.com> wrote:
> Use new return type vm_fault_t for fault and huge_fault
> handler. For now, this is just documenting that the
> function returns a VM_FAULT value rather than an errno.
> Once all instances are converted, vm_fault_t will become
> a distinct type.
>
> Reference id -> 1c8f422059ae ("mm: change return type to
> vm_fault_t")
>
> Previously vm_insert_mixed() returns err which driver
> mapped into VM_FAULT_* type. The new function
> vmf_insert_mixed() will replace this inefficiency by
> returning VM_FAULT_* type.
>
> Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com>
> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>
> Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
> v2: Modified the change log
>
> v3: Updated the change log and
>     added Ross in review list
>
>  drivers/dax/device.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 2137dbc..a122701 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -243,11 +243,11 @@ __weak phys_addr_t dax_pgoff_to_phys(struct dev_dax *dev_dax, pgoff_t pgoff,
>         return -1;
>  }
>
> -static int __dev_dax_pte_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
> +static vm_fault_t __dev_dax_pte_fault(struct dev_dax *dev_dax,
> +                               struct vm_fault *vmf)
>  {
>         struct device *dev = &dev_dax->dev;
>         struct dax_region *dax_region;
> -       int rc = VM_FAULT_SIGBUS;
>         phys_addr_t phys;
>         pfn_t pfn;
>         unsigned int fault_size = PAGE_SIZE;
> @@ -274,17 +274,11 @@ static int __dev_dax_pte_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
>
>         pfn = phys_to_pfn_t(phys, dax_region->pfn_flags);
>
> -       rc = vm_insert_mixed(vmf->vma, vmf->address, pfn);
> -
> -       if (rc == -ENOMEM)
> -               return VM_FAULT_OOM;
> -       if (rc < 0 && rc != -EBUSY)
> -               return VM_FAULT_SIGBUS;
> -
> -       return VM_FAULT_NOPAGE;
> +       return vmf_insert_mixed(vmf->vma, vmf->address, pfn);
>  }
>
> -static int __dev_dax_pmd_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
> +static vm_fault_t __dev_dax_pmd_fault(struct dev_dax *dev_dax,
> +                               struct vm_fault *vmf)
>  {
>         unsigned long pmd_addr = vmf->address & PMD_MASK;
>         struct device *dev = &dev_dax->dev;
> @@ -335,7 +329,8 @@ static int __dev_dax_pmd_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
>  }
>
>  #ifdef CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD
> -static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
> +static vm_fault_t __dev_dax_pud_fault(struct dev_dax *dev_dax,
> +                               struct vm_fault *vmf)
>  {
>         unsigned long pud_addr = vmf->address & PUD_MASK;
>         struct device *dev = &dev_dax->dev;
> @@ -386,13 +381,14 @@ static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
>                         vmf->flags & FAULT_FLAG_WRITE);
>  }
>  #else
> -static int __dev_dax_pud_fault(struct dev_dax *dev_dax, struct vm_fault *vmf)
> +static vm_fault_t __dev_dax_pud_fault(struct dev_dax *dev_dax,
> +                               struct vm_fault *vmf)
>  {
>         return VM_FAULT_FALLBACK;
>  }
>  #endif /* !CONFIG_HAVE_ARCH_TRANSPARENT_HUGEPAGE_PUD */
>
> -static int dev_dax_huge_fault(struct vm_fault *vmf,
> +static vm_fault_t dev_dax_huge_fault(struct vm_fault *vmf,
>                 enum page_entry_size pe_size)
>  {
>         int rc, id;
> @@ -423,7 +419,7 @@ static int dev_dax_huge_fault(struct vm_fault *vmf,
>         return rc;
>  }
>
> -static int dev_dax_fault(struct vm_fault *vmf)
> +static vm_fault_t dev_dax_fault(struct vm_fault *vmf)
>  {
>         return dev_dax_huge_fault(vmf, PE_SIZE_PTE);
>  }
> --
> 1.9.1
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] dax: Change return type to vm_fault_t

[David Rientjes]

On Fri, 27 Apr 2018, Souptick Joarder wrote:

> Hi Matthew/ Ross,
> 
> There are two changes exist in mm/huge_memory.c as part of this
> patch. vmf_insert_pfn_pmd() and vmf_insert_pfn_pud() functions are
> invoked from this patch.
> 
> Shall we put both in a single patch that it will easy to bisect in case
> we have any issue ?
> 

Please put them into a single patch, there's no reason to convert

int foo() -> vm_fault_t foo()

but leave

int bar()
{
	return foo()
}

It would be best just to convert all callers to also return vm_fault_t as 
I outlined in my response.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] dax: Change return type to vm_fault_t

[Matthew Wilcox]

On Fri, Apr 27, 2018 at 01:37:02AM -0700, David Rientjes wrote:
> On Fri, 27 Apr 2018, Souptick Joarder wrote:
> 
> > Hi Matthew/ Ross,
> > 
> > There are two changes exist in mm/huge_memory.c as part of this
> > patch. vmf_insert_pfn_pmd() and vmf_insert_pfn_pud() functions are
> > invoked from this patch.
> > 
> > Shall we put both in a single patch that it will easy to bisect in case
> > we have any issue ?
> > 
> 
> Please put them into a single patch, there's no reason to convert
> 
> int foo() -> vm_fault_t foo()
> 
> but leave
> 
> int bar()
> {
> 	return foo()
> }
> 
> It would be best just to convert all callers to also return vm_fault_t as 
> I outlined in my response.

Yes, they're all getting converted, but there are too many to do in a
single patch.  So it's just a matter of how to split them up.  Since the
types are compatible (for now), I advised Souptick to split them by
maintenance area in order to minimise conflicts with other patches.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm

Re: [PATCH v3] dax: Change return type to vm_fault_t

[David Rientjes]

On Fri, 27 Apr 2018, Matthew Wilcox wrote:

> > > Hi Matthew/ Ross,
> > > 
> > > There are two changes exist in mm/huge_memory.c as part of this
> > > patch. vmf_insert_pfn_pmd() and vmf_insert_pfn_pud() functions are
> > > invoked from this patch.
> > > 
> > > Shall we put both in a single patch that it will easy to bisect in case
> > > we have any issue ?
> > > 
> > 
> > Please put them into a single patch, there's no reason to convert
> > 
> > int foo() -> vm_fault_t foo()
> > 
> > but leave
> > 
> > int bar()
> > {
> > 	return foo()
> > }
> > 
> > It would be best just to convert all callers to also return vm_fault_t as 
> > I outlined in my response.
> 
> Yes, they're all getting converted, but there are too many to do in a
> single patch.  So it's just a matter of how to split them up.  Since the
> types are compatible (for now), I advised Souptick to split them by
> maintenance area in order to minimise conflicts with other patches.
> 

All I'm saying is that it is 1000 times easier to review and audit if foo 
and bar are converted in the same patch above instead of getting feedback 
saying "oh, you converted foo() but not bar()" or vice versa and then 
referring to another patch posted on some other mailing list as was done 
here.  How big the patch isn't very important if it's more reviewable.  
And converting foo() here and not bar() does nothing but make it harder to 
review.
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm