| Message ID | 20180416185044.GA29337@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 17, 2018 at 12:20:44AM +0530, Souptick Joarder 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. I'm not sure what you mean by "vm_fault_t will become a distinct type"? Do you mean you'll make it into an enum, i.e.: enum vm_fault_t { VM_FAULT_OOM = 0x0001, VM_FAULT_SIGBUS = 0x0002, VM_FAULT_MAJOR = 0x0004, VM_FAULT_WRITE = 0x0008, VM_FAULT_HWPOISON = 0x0010, VM_FAULT_HWPOISON_LARGE = 0x0020, VM_FAULT_NOPAGE = 0x0100, VM_FAULT_LOCKED = 0x0200, VM_FAULT_RETRY = 0x0400, VM_FAULT_FALLBACK = 0x0800, VM_FAULT_DONE_COW = 0x1000, VM_FAULT_NEEDDSYNC = 0x2000, }; If so, I think that would be great for readability. Right now I look up the definition of vm_fault_t and see it's typedef'd to an int, and that doesn't give me as much info as the above enum would. If this is the plan, I don't understand why you need to make all the site conversions first? > 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> > --- > v2: Modified the change log > > v3: Updated the change log You've only got a v2. :) Sure, the code looks correct. You can add: Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com>
On Mon, Apr 16, 2018 at 05:02:00PM -0600, Ross Zwisler wrote: > I'm not sure what you mean by "vm_fault_t will become a distinct type"? Do > you mean you'll make it into an enum, i.e.: > > enum vm_fault_t { > VM_FAULT_OOM = 0x0001, > VM_FAULT_SIGBUS = 0x0002, > VM_FAULT_MAJOR = 0x0004, > VM_FAULT_WRITE = 0x0008, > VM_FAULT_HWPOISON = 0x0010, > VM_FAULT_HWPOISON_LARGE = 0x0020, > VM_FAULT_NOPAGE = 0x0100, > VM_FAULT_LOCKED = 0x0200, > VM_FAULT_RETRY = 0x0400, > VM_FAULT_FALLBACK = 0x0800, > VM_FAULT_DONE_COW = 0x1000, > VM_FAULT_NEEDDSYNC = 0x2000, > }; My current tree has this in it: +/* + * Different kinds of faults, as returned from fault handlers. + * Used to decide whether a process gets delivered SIGBUS or + * just gets major/minor fault counters bumped up. + */ +typedef __bitwise unsigned int vm_fault_t; +enum { + VM_FAULT_OOM = (__force vm_fault_t)0x000001, /* Out Of Memory */ + VM_FAULT_SIGBUS = (__force vm_fault_t)0x000002, /* Bad access */ + VM_FAULT_MAJOR = (__force vm_fault_t)0x000004, /* Page read from storage */ + VM_FAULT_WRITE = (__force vm_fault_t)0x000008, /* Special case for get_user_pages */ + VM_FAULT_HWPOISON = (__force vm_fault_t)0x000010, /* Hit poisoned small page */ + VM_FAULT_HWPOISON_LARGE = (__force vm_fault_t)0x000020, /* Hit poisoned large page. Index encoded in upper bits */ + VM_FAULT_SIGSEGV = (__force vm_fault_t)0x000040, + VM_FAULT_NOPAGE = (__force vm_fault_t)0x000100, /* ->fault installed the pte, not return page */ + VM_FAULT_LOCKED = (__force vm_fault_t)0x000200, /* ->fault locked the returned page */ + VM_FAULT_RETRY = (__force vm_fault_t)0x000400, /* ->fault blocked, must retry */ + VM_FAULT_FALLBACK = (__force vm_fault_t)0x000800, /* huge page fault failed, fall back to small */ + VM_FAULT_DONE_COW = (__force vm_fault_t)0x001000, /* ->fault has fully handled COW */ + VM_FAULT_NEEDDSYNC = (__force vm_fault_t)0x002000, /* ->fault did not modify page tables + * and needs fsync() to complete (for + * synchronous page faults in DAX) */ + VM_FAULT_HINDEX_MASK = (__force vm_fault_t)0x0f0000, +}; However, I'm hoping to get a better syntax into sparse ;-) > If so, I think that would be great for readability. Right now I look up the > definition of vm_fault_t and see it's typedef'd to an int, and that doesn't > give me as much info as the above enum would. > > If this is the plan, I don't understand why you need to make all the site > conversions first? Changing it from a signed to an unsigned int causes GCC to warn about an assignment from an incompatible type -- int foo(void) is incompatible with unsigned int foo(void). > Sure, the code looks correct. You can add: > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Thanks, Ross.
On Tue, Apr 17, 2018 at 4:32 AM, Ross Zwisler <ross.zwisler@linux.intel.com> wrote: > On Tue, Apr 17, 2018 at 12:20:44AM +0530, Souptick Joarder 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. > > I'm not sure what you mean by "vm_fault_t will become a distinct type"? Do > you mean you'll make it into an enum, i.e.: > > enum vm_fault_t { > VM_FAULT_OOM = 0x0001, > VM_FAULT_SIGBUS = 0x0002, > VM_FAULT_MAJOR = 0x0004, > VM_FAULT_WRITE = 0x0008, > VM_FAULT_HWPOISON = 0x0010, > VM_FAULT_HWPOISON_LARGE = 0x0020, > VM_FAULT_NOPAGE = 0x0100, > VM_FAULT_LOCKED = 0x0200, > VM_FAULT_RETRY = 0x0400, > VM_FAULT_FALLBACK = 0x0800, > VM_FAULT_DONE_COW = 0x1000, > VM_FAULT_NEEDDSYNC = 0x2000, > }; > > If so, I think that would be great for readability. Right now I look up the > definition of vm_fault_t and see it's typedef'd to an int, and that doesn't > give me as much info as the above enum would. > > If this is the plan, I don't understand why you need to make all the site > conversions first? > >> 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> >> --- >> v2: Modified the change log >> >> v3: Updated the change log > > You've only got a v2. :) Sorry about it. I will correct :) > > Sure, the code looks correct. You can add: > Reviewed-by: Ross Zwisler <ross.zwisler@linux.intel.com> Thanks.
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> --- v2: Modified the change log v3: Updated the change log drivers/dax/device.c | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-)