| Message ID | 1310200731-18086-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 9 Jul 2011 09:38:50 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > +static int prefault_writeable(unsigned long uaddr, unsigned long len) > +static int prefault_readable(unsigned long uaddr, unsigned long len) These seems like a functions which belongs alongside (or in place of) the existing fault_in_pages_writable and fault_in_pages_readable functions. I think, in fact, that one could convincingly argue that the existing functions are misleadingly named (or just broken).
On Sat, 09 Jul 2011 13:21:48 -0700, Keith Packard <keithp@keithp.com> wrote: > On Sat, 9 Jul 2011 09:38:50 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > > +static int prefault_writeable(unsigned long uaddr, unsigned long len) > > > +static int prefault_readable(unsigned long uaddr, unsigned long len) > > These seems like a functions which belongs alongside (or in place of) the > existing fault_in_pages_writable and fault_in_pages_readable functions. > > I think, in fact, that one could convincingly argue that the existing > functions are misleadingly named (or just broken). I agree that I was indeed misled by the description of those two functions! However, I don't like why this appears to fix the issue of get_user_pages() failing on the non-prefaulted address. -Chris
On Sat, Jul 09, 2011 at 09:38:50AM +0100, Chris Wilson wrote: > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 4fc9738..2fce620 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -503,6 +503,19 @@ out: > return ret; > } > > +static int prefault_writeable(unsigned long uaddr, unsigned long len) > +{ > + int ret = 0; > + > + len += uaddr; > + while (uaddr < len) { > + ret |= __put_user(0, (char __user *)uaddr); > + uaddr += 4096; > + } > + > + return ret ? -EFAULT : 0; > +} > + > /** > * Reads data from the object referenced by handle. > * What's the reason for not breaking out of the loop on the first failing fault? It seems like you could incur a bunch of latency for a call which you know will ending up failin anyway. Although TBH I'm not clear how it could actually fail if you've called access_ok(). Also you should probably use PAGE_SIZE instead of 4096. Ben
On Sun, 10 Jul 2011 01:40:31 +0000, Ben Widawsky <ben@bwidawsk.net> wrote: > On Sat, Jul 09, 2011 at 09:38:50AM +0100, Chris Wilson wrote: > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > > index 4fc9738..2fce620 100644 > > --- a/drivers/gpu/drm/i915/i915_gem.c > > +++ b/drivers/gpu/drm/i915/i915_gem.c > > @@ -503,6 +503,19 @@ out: > > return ret; > > } > > > > +static int prefault_writeable(unsigned long uaddr, unsigned long len) > > +{ > > + int ret = 0; > > + > > + len += uaddr; > > + while (uaddr < len) { > > + ret |= __put_user(0, (char __user *)uaddr); > > + uaddr += 4096; > > + } > > + > > + return ret ? -EFAULT : 0; > > +} > > + > > /** > > * Reads data from the object referenced by handle. > > * > > What's the reason for not breaking out of the loop on the first failing > fault? It seems like you could incur a bunch of latency for a call which > you know will ending up failin anyway. Although TBH I'm not clear how > it could actually fail if you've called access_ok(). Because encountering a fault in that loop is an extremely rare event, and the branch inside the loop appeared on the profiles. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 4fc9738..2fce620 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -503,6 +503,19 @@ out: return ret; } +static int prefault_writeable(unsigned long uaddr, unsigned long len) +{ + int ret = 0; + + len += uaddr; + while (uaddr < len) { + ret |= __put_user(0, (char __user *)uaddr); + uaddr += 4096; + } + + return ret ? -EFAULT : 0; +} + /** * Reads data from the object referenced by handle. * @@ -524,8 +537,7 @@ i915_gem_pread_ioctl(struct drm_device *dev, void *data, args->size)) return -EFAULT; - ret = fault_in_pages_writeable((char __user *)(uintptr_t)args->data_ptr, - args->size); + ret = prefault_writeable(args->data_ptr, args->size); if (ret) return -EFAULT; @@ -943,6 +955,21 @@ out: return ret; } +static int prefault_readable(unsigned long uaddr, unsigned long len) +{ + volatile char c; + int ret = 0; + + len += uaddr; + while (uaddr < len) { + ret |= __get_user(c, (const char __user *)uaddr); + uaddr += 4096; + } + + return ret ? -EFAULT : 0; + (void)c; +} + /** * Writes data to the object referenced by handle. * @@ -964,8 +991,7 @@ i915_gem_pwrite_ioctl(struct drm_device *dev, void *data, args->size)) return -EFAULT; - ret = fault_in_pages_readable((char __user *)(uintptr_t)args->data_ptr, - args->size); + ret = prefault_readable(args->data_ptr, args->size); if (ret) return -EFAULT;
When using a GTT mapping as a source or destination for the pwrite or pread command respectively, unless the PTEs for the GTT vma had been prepopulated then get_user_pages() would fail with EFAULT. Usually, we only write small amounts of data with pwrite that happened to be conveniently prefaulted by the 2-page fault_in_pages_readable. By prefaulting all pages before we take the struct mutex, we avoid this issue, can stay in the fast path longer and reduce the likelihood of a recursive deadlock. "Fixes" gem_mmap_gtt. References: https://bugs.freedesktop.org/show_bug.cgi?id=38115 Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_gem.c | 34 ++++++++++++++++++++++++++++++---- 1 files changed, 30 insertions(+), 4 deletions(-)