| Message ID | 20180417192038.GA22918@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | powerpc: platform: cell: spufs: Change return type to vm_fault_t | expand |
On Wed, Apr 18, 2018 at 2:17 AM, Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Apr 18, 2018 at 12:50:38AM +0530, Souptick Joarder wrote: >> Use new return type vm_fault_t for 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_pfn() returns err but driver returns >> VM_FAULT_NOPAGE as default. The new function vmf_insert_pfn() >> will replace this inefficiency by returning correct VM_FAULT_* >> type. >> >> vmf_handle_error is a inline wrapper function which >> will convert error number to vm_fault_t type err. > > I think you sent the wrong version of this one ... > > The commit message should mention that we're fixing a minor bug, that > the error from vm_insert_pfn() was being ignored and the effect of this > is likely to be only felt in OOM situations. Ok, I will add this. > >> @@ -256,11 +257,11 @@ static ssize_t spufs_attr_write(struct file *file, const char __user *buf, >> vma->vm_page_prot = pgprot_noncached_wc(vma->vm_page_prot); >> pfn = (ctx->spu->local_store_phys + offset) >> PAGE_SHIFT; >> } >> - vm_insert_pfn(vma, vmf->address, pfn); >> + ret = vmf_insert_pfn(vma, vmf->address, pfn); >> >> spu_release(ctx); >> >> - return VM_FAULT_NOPAGE; >> + return ret; >> } > > I thought I said not to introduce vmf_handle_error(), because it's too > trivial and obfuscates what's actually going on. > >> -static int spufs_ps_fault(struct vm_fault *vmf, >> +static inline vm_fault_t vmf_handle_error(int err) >> +{ >> + return VM_FAULT_NOPAGE; >> +} >> + > > Re-reading spufs_ps_fault(), I wouldn't change anything inside it. Just > change its return type to vm_fault_t and call it done. In that case, return value of spufs_wait() has to changed to VM_FAULT_ type and we end with changing all the references where spufs_wait() is called. I think we shouldn't go with that approach. That's the reason I introduce inline vmf_handle_error() and convert err to VM_FAULT_ type.
On Thu, Apr 19, 2018 at 12:34:15AM +0530, Souptick Joarder wrote: > > Re-reading spufs_ps_fault(), I wouldn't change anything inside it. Just > > change its return type to vm_fault_t and call it done. > > In that case, return value of spufs_wait() has to changed > to VM_FAULT_ type and we end with changing all the > references where spufs_wait() is called. I think we shouldn't > go with that approach. That's the reason I introduce inline > vmf_handle_error() and convert err to VM_FAULT_ type. No, don't change the type of 'ret' or spufs_wait. Just do this: -static int spufs_ps_fault(struct vm_fault *vmf, +static vm_fault_t spufs_ps_fault(struct vm_fault *vmf, unsigned long ps_offs, unsigned long ps_size)
On Thu, Apr 19, 2018 at 12:57 AM, Matthew Wilcox <willy@infradead.org> wrote: > On Thu, Apr 19, 2018 at 12:34:15AM +0530, Souptick Joarder wrote: >> > Re-reading spufs_ps_fault(), I wouldn't change anything inside it. Just >> > change its return type to vm_fault_t and call it done. >> >> In that case, return value of spufs_wait() has to changed >> to VM_FAULT_ type and we end with changing all the >> references where spufs_wait() is called. I think we shouldn't >> go with that approach. That's the reason I introduce inline >> vmf_handle_error() and convert err to VM_FAULT_ type. > > No, don't change the type of 'ret' or spufs_wait. Just do this: > > -static int spufs_ps_fault(struct vm_fault *vmf, > +static vm_fault_t spufs_ps_fault(struct vm_fault *vmf, > unsigned long ps_offs, > unsigned long ps_size) > Agree. but vm_insert_pfn should be replaced with new vmf_insert_pfn, right ?
On Thu, Apr 19, 2018 at 01:03:43AM +0530, Souptick Joarder wrote: > On Thu, Apr 19, 2018 at 12:57 AM, Matthew Wilcox <willy@infradead.org> wrote: > > On Thu, Apr 19, 2018 at 12:34:15AM +0530, Souptick Joarder wrote: > >> > Re-reading spufs_ps_fault(), I wouldn't change anything inside it. Just > >> > change its return type to vm_fault_t and call it done. > >> > >> In that case, return value of spufs_wait() has to changed > >> to VM_FAULT_ type and we end with changing all the > >> references where spufs_wait() is called. I think we shouldn't > >> go with that approach. That's the reason I introduce inline > >> vmf_handle_error() and convert err to VM_FAULT_ type. > > > > No, don't change the type of 'ret' or spufs_wait. Just do this: > > > > -static int spufs_ps_fault(struct vm_fault *vmf, > > +static vm_fault_t spufs_ps_fault(struct vm_fault *vmf, > > unsigned long ps_offs, > > unsigned long ps_size) > > > > Agree. but vm_insert_pfn should be replaced with new > vmf_insert_pfn, right ? Oh, right, sorry, I missed that. So something like this: -static int spufs_ps_fault(struct vm_fault *vmf, +static vm_fault_t spufs_ps_fault(struct vm_fault *vmf, unsigned long ps_offs, unsigned long ps_size) { struct spu_context *ctx = vmf->vma->vm_file->private_data; unsigned long area, offset = vmf->pgoff << PAGE_SHIFT; - int ret = 0; + int err = 0; + vm_fault_t ret = VM_FAULT_NOPAGE; spu_context_nospu_trace(spufs_ps_fault__enter, ctx); @@ -349,21 +350,22 @@ static int spufs_ps_fault(struct vm_fault *vmf, if (ctx->state == SPU_STATE_SAVED) { up_read(¤t->mm->mmap_sem); spu_context_nospu_trace(spufs_ps_fault__sleep, ctx); - ret = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE); + err = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE); spu_context_trace(spufs_ps_fault__wake, ctx, ctx->spu); down_read(¤t->mm->mmap_sem); } else { area = ctx->spu->problem_phys + ps_offs; - vm_insert_pfn(vmf->vma, vmf->address, (area + offset) >> PAGE_SHIFT); + ret = vmf_insert_pfn(vmf->vma, vmf->address, + (area + offset) >> PAGE_SHIFT); spu_context_trace(spufs_ps_fault__insert, ctx, ctx->spu); } - if (!ret) + if (!err) spu_release(ctx); refault: put_spu_context(ctx); - return VM_FAULT_NOPAGE; + return ret; }
diff --git a/arch/powerpc/platforms/cell/spufs/file.c b/arch/powerpc/platforms/cell/spufs/file.c index 469bdd0..a1dca9a 100644 --- a/arch/powerpc/platforms/cell/spufs/file.c +++ b/arch/powerpc/platforms/cell/spufs/file.c @@ -232,12 +232,13 @@ static ssize_t spufs_attr_write(struct file *file, const char __user *buf, return size; } -static int +static vm_fault_t spufs_mem_mmap_fault(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; struct spu_context *ctx = vma->vm_file->private_data; unsigned long pfn, offset; + vm_fault_t ret; offset = vmf->pgoff << PAGE_SHIFT; if (offset >= LS_SIZE) @@ -256,11 +257,11 @@ static ssize_t spufs_attr_write(struct file *file, const char __user *buf, vma->vm_page_prot = pgprot_noncached_wc(vma->vm_page_prot); pfn = (ctx->spu->local_store_phys + offset) >> PAGE_SHIFT; } - vm_insert_pfn(vma, vmf->address, pfn); + ret = vmf_insert_pfn(vma, vmf->address, pfn); spu_release(ctx); - return VM_FAULT_NOPAGE; + return ret; } static int spufs_mem_mmap_access(struct vm_area_struct *vma, @@ -312,13 +313,19 @@ static int spufs_mem_mmap(struct file *file, struct vm_area_struct *vma) .mmap = spufs_mem_mmap, }; -static int spufs_ps_fault(struct vm_fault *vmf, +static inline vm_fault_t vmf_handle_error(int err) +{ + return VM_FAULT_NOPAGE; +} + +static vm_fault_t spufs_ps_fault(struct vm_fault *vmf, unsigned long ps_offs, unsigned long ps_size) { struct spu_context *ctx = vmf->vma->vm_file->private_data; unsigned long area, offset = vmf->pgoff << PAGE_SHIFT; - int ret = 0; + int err = 0; + vm_fault_t ret = VM_FAULT_NOPAGE; spu_context_nospu_trace(spufs_ps_fault__enter, ctx); @@ -349,12 +356,14 @@ static int spufs_ps_fault(struct vm_fault *vmf, if (ctx->state == SPU_STATE_SAVED) { up_read(¤t->mm->mmap_sem); spu_context_nospu_trace(spufs_ps_fault__sleep, ctx); - ret = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE); + err = spufs_wait(ctx->run_wq, ctx->state == SPU_STATE_RUNNABLE); + ret = vmf_handle_error(err); spu_context_trace(spufs_ps_fault__wake, ctx, ctx->spu); down_read(¤t->mm->mmap_sem); } else { area = ctx->spu->problem_phys + ps_offs; - vm_insert_pfn(vmf->vma, vmf->address, (area + offset) >> PAGE_SHIFT); + ret = vmf_insert_pfn(vmf->vma, vmf->address, + (area + offset) >> PAGE_SHIFT); spu_context_trace(spufs_ps_fault__insert, ctx, ctx->spu); } @@ -363,11 +372,11 @@ static int spufs_ps_fault(struct vm_fault *vmf, refault: put_spu_context(ctx); - return VM_FAULT_NOPAGE; + return ret; } #if SPUFS_MMAP_4K -static int spufs_cntl_mmap_fault(struct vm_fault *vmf) +static vm_fault_t spufs_cntl_mmap_fault(struct vm_fault *vmf) { return spufs_ps_fault(vmf, 0x4000, SPUFS_CNTL_MAP_SIZE); } @@ -1040,7 +1049,7 @@ static ssize_t spufs_signal1_write(struct file *file, const char __user *buf, return 4; } -static int +static vm_fault_t spufs_signal1_mmap_fault(struct vm_fault *vmf) { #if SPUFS_SIGNAL_MAP_SIZE == 0x1000 @@ -1178,7 +1187,7 @@ static ssize_t spufs_signal2_write(struct file *file, const char __user *buf, } #if SPUFS_MMAP_4K -static int +static vm_fault_t spufs_signal2_mmap_fault(struct vm_fault *vmf) { #if SPUFS_SIGNAL_MAP_SIZE == 0x1000 @@ -1307,7 +1316,7 @@ static u64 spufs_signal2_type_get(struct spu_context *ctx) spufs_signal2_type_set, "%llu\n", SPU_ATTR_ACQUIRE); #if SPUFS_MMAP_4K -static int +static vm_fault_t spufs_mss_mmap_fault(struct vm_fault *vmf) { return spufs_ps_fault(vmf, 0x0000, SPUFS_MSS_MAP_SIZE); @@ -1369,7 +1378,7 @@ static int spufs_mss_open(struct inode *inode, struct file *file) .llseek = no_llseek, }; -static int +static vm_fault_t spufs_psmap_mmap_fault(struct vm_fault *vmf) { return spufs_ps_fault(vmf, 0x0000, SPUFS_PS_MAP_SIZE); @@ -1429,7 +1438,7 @@ static int spufs_psmap_open(struct inode *inode, struct file *file) #if SPUFS_MMAP_4K -static int +static vm_fault_t spufs_mfc_mmap_fault(struct vm_fault *vmf) { return spufs_ps_fault(vmf, 0x3000, SPUFS_MFC_MAP_SIZE);
Use new return type vm_fault_t for 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_pfn() returns err but driver returns VM_FAULT_NOPAGE as default. The new function vmf_insert_pfn() will replace this inefficiency by returning correct VM_FAULT_* type. vmf_handle_error is a inline wrapper function which will convert error number to vm_fault_t type err. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com> --- arch/powerpc/platforms/cell/spufs/file.c | 37 ++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 14 deletions(-)