| Message ID | 20180417134755.GA30291@jordon-HP-15-Notebook-PC (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 17, 2018 at 07:17:55PM +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_page() returns err which driver > mapped into VM_FAULT_* type. The new function > vmf_insert_page() will replace this inefficiency by > returning VM_FAULT_* type. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > --- > drivers/gpu/drm/tegra/gem.c | 18 ++---------------- > 1 file changed, 2 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c > index 49b9bf2..6121493 100644 > --- a/drivers/gpu/drm/tegra/gem.c > +++ b/drivers/gpu/drm/tegra/gem.c > @@ -422,14 +422,13 @@ int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm, > return 0; > } > > -static int tegra_bo_fault(struct vm_fault *vmf) > +static vm_fault_t tegra_bo_fault(struct vm_fault *vmf) > { > struct vm_area_struct *vma = vmf->vma; > struct drm_gem_object *gem = vma->vm_private_data; > struct tegra_bo *bo = to_tegra_bo(gem); > struct page *page; > pgoff_t offset; > - int err; > > if (!bo->pages) > return VM_FAULT_SIGBUS; > @@ -437,20 +436,7 @@ static int tegra_bo_fault(struct vm_fault *vmf) > offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; > page = bo->pages[offset]; > > - err = vm_insert_page(vma, vmf->address, page); > - switch (err) { > - case -EAGAIN: > - case 0: > - case -ERESTARTSYS: > - case -EINTR: > - case -EBUSY: > - return VM_FAULT_NOPAGE; > - > - case -ENOMEM: > - return VM_FAULT_OOM; > - } > - > - return VM_FAULT_SIGBUS; > + return vmf_insert_page(vma, vmf->address, page); > } This new function returns VM_FAULT_NOPAGE only for 0 and -EBUSY, whereas we used to return VM_FAULT_NOPAGE for -EAGAIN, -ERESTARTSYS and -EINTR as well. Was this previously wrong? Thierry
> This new function returns VM_FAULT_NOPAGE only for 0 and -EBUSY, whereas > we used to return VM_FAULT_NOPAGE for -EAGAIN, -ERESTARTSYS and -EINTR > as well. Previously vm_insert_page unable to return VM_FAULT_ type due to which different drivers have their own mapping from err to VM_FAULT_ type. With new vmf_insert_page() introduce in 4.17-rc1 we have a common mapping for err to VM_FAULT_ which remain same for every drivers/file systems. Was this previously wrong? I think Matthew is the right person to answer this.
On Wed, Apr 18, 2018 at 10:24:39AM +0200, Thierry Reding wrote: > > @@ -437,20 +436,7 @@ static int tegra_bo_fault(struct vm_fault *vmf) > > offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; > > page = bo->pages[offset]; > > > > - err = vm_insert_page(vma, vmf->address, page); > > - switch (err) { > > - case -EAGAIN: > > - case 0: > > - case -ERESTARTSYS: > > - case -EINTR: > > - case -EBUSY: > > - return VM_FAULT_NOPAGE; > > - > > - case -ENOMEM: > > - return VM_FAULT_OOM; > > - } > > - > > - return VM_FAULT_SIGBUS; > > + return vmf_insert_page(vma, vmf->address, page); > > } > > This new function returns VM_FAULT_NOPAGE only for 0 and -EBUSY, whereas > we used to return VM_FAULT_NOPAGE for -EAGAIN, -ERESTARTSYS and -EINTR > as well. Was this previously wrong? Not so much wrong as unnecessary. vm_insert_page() can't return -EAGAIN, -ERESTARTSYS or -EINTR.
On Mon, Apr 23, 2018 at 4:24 PM, Matthew Wilcox <willy@infradead.org> wrote: > On Wed, Apr 18, 2018 at 10:24:39AM +0200, Thierry Reding wrote: >> > @@ -437,20 +436,7 @@ static int tegra_bo_fault(struct vm_fault *vmf) >> > offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; >> > page = bo->pages[offset]; >> > >> > - err = vm_insert_page(vma, vmf->address, page); >> > - switch (err) { >> > - case -EAGAIN: >> > - case 0: >> > - case -ERESTARTSYS: >> > - case -EINTR: >> > - case -EBUSY: >> > - return VM_FAULT_NOPAGE; >> > - >> > - case -ENOMEM: >> > - return VM_FAULT_OOM; >> > - } >> > - >> > - return VM_FAULT_SIGBUS; >> > + return vmf_insert_page(vma, vmf->address, page); >> > } >> >> This new function returns VM_FAULT_NOPAGE only for 0 and -EBUSY, whereas >> we used to return VM_FAULT_NOPAGE for -EAGAIN, -ERESTARTSYS and -EINTR >> as well. Was this previously wrong? > > Not so much wrong as unnecessary. vm_insert_page() can't return -EAGAIN, > -ERESTARTSYS or -EINTR. If no further comment on this patch, We would like to get this patch queued for 4.18.
On Tue, Apr 17, 2018 at 07:17:55PM +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_page() returns err which driver > mapped into VM_FAULT_* type. The new function > vmf_insert_page() will replace this inefficiency by > returning VM_FAULT_* type. > > Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> > --- > drivers/gpu/drm/tegra/gem.c | 18 ++---------------- > 1 file changed, 2 insertions(+), 16 deletions(-) Applied, thanks. Thierry
diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c index 49b9bf2..6121493 100644 --- a/drivers/gpu/drm/tegra/gem.c +++ b/drivers/gpu/drm/tegra/gem.c @@ -422,14 +422,13 @@ int tegra_bo_dumb_create(struct drm_file *file, struct drm_device *drm, return 0; } -static int tegra_bo_fault(struct vm_fault *vmf) +static vm_fault_t tegra_bo_fault(struct vm_fault *vmf) { struct vm_area_struct *vma = vmf->vma; struct drm_gem_object *gem = vma->vm_private_data; struct tegra_bo *bo = to_tegra_bo(gem); struct page *page; pgoff_t offset; - int err; if (!bo->pages) return VM_FAULT_SIGBUS; @@ -437,20 +436,7 @@ static int tegra_bo_fault(struct vm_fault *vmf) offset = (vmf->address - vma->vm_start) >> PAGE_SHIFT; page = bo->pages[offset]; - err = vm_insert_page(vma, vmf->address, page); - switch (err) { - case -EAGAIN: - case 0: - case -ERESTARTSYS: - case -EINTR: - case -EBUSY: - return VM_FAULT_NOPAGE; - - case -ENOMEM: - return VM_FAULT_OOM; - } - - return VM_FAULT_SIGBUS; + return vmf_insert_page(vma, vmf->address, page); } const struct vm_operations_struct tegra_bo_vm_ops = {
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_page() returns err which driver mapped into VM_FAULT_* type. The new function vmf_insert_page() will replace this inefficiency by returning VM_FAULT_* type. Signed-off-by: Souptick Joarder <jrdr.linux@gmail.com> --- drivers/gpu/drm/tegra/gem.c | 18 ++---------------- 1 file changed, 2 insertions(+), 16 deletions(-) -- 1.9.1