| Message ID | 20180417132842.GA30189@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 >
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.
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.
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.
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); }
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(-)