[PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[Chris Wilson]

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(-)

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;
 
-- 
1.7.5.4

[PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Chris Wilson]

These paths hold onto the struct mutex whilst accessing pages. In
order, to prevent a recursive dead-lock should we fault-in a GTT mapped
page we need to return -EFAULT and fallback to the slow path.

Lockdep has complained before about the potential dead-lock, but rvis is
the first application found to sufficiently abuse the API to trigger it.

Cursory performance regression testing on a 1GiB PineView system using
x11perf, cairo-perf-trace, glxgears and a few game benchmarks suggested
no large regressions with just a 2% slowdown for firefox. The caveat is
that this was an otherwise idle system and that for 32-bit systems
io_mapping_map_atomic_wc() already disabled page-faults.

Bugzilla: 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 |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2fce620..ecb27fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -365,9 +365,15 @@ i915_gem_shmem_pread_fast(struct drm_device *dev,
 			return PTR_ERR(page);
 
 		vaddr = kmap_atomic(page);
+		/* We have to disable faulting here in case the user address
+		 * is really a GTT mapping and so we can not enter
+		 * i915_gem_fault() whilst already holding struct_mutex.
+		 */
+		pagefault_disable();
 		ret = __copy_to_user_inatomic(user_data,
 					      vaddr + page_offset,
 					      page_length);
+		pagefault_enable();
 		kunmap_atomic(vaddr);
 
 		mark_page_accessed(page);
@@ -593,8 +599,14 @@ fast_user_write(struct io_mapping *mapping,
 	unsigned long unwritten;
 
 	vaddr_atomic = io_mapping_map_atomic_wc(mapping, page_base);
+	/* We have to disable faulting here in case the user address
+	 * is really a GTT mapping and so we can not enter
+	 * i915_gem_fault() whilst already holding struct_mutex.
+	 */
+	pagefault_disable();
 	unwritten = __copy_from_user_inatomic_nocache(vaddr_atomic + page_offset,
 						      user_data, length);
+	pagefault_enable();
 	io_mapping_unmap_atomic(vaddr_atomic);
 	return unwritten;
 }
@@ -812,11 +824,17 @@ i915_gem_shmem_pwrite_fast(struct drm_device *dev,
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		vaddr = kmap_atomic(page, KM_USER0);
+		vaddr = kmap_atomic(page);
+		/* We have to disable faulting here in case the user address
+		 * is really a GTT mapping and so we can not enter
+		 * i915_gem_fault() whilst already holding struct_mutex.
+		 */
+		pagefault_disable();
 		ret = __copy_from_user_inatomic(vaddr + page_offset,
 						user_data,
 						page_length);
-		kunmap_atomic(vaddr, KM_USER0);
+		pagefault_enable();
+		kunmap_atomic(vaddr);
 
 		set_page_dirty(page);
 		mark_page_accessed(page);
-- 
1.7.5.4

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 1874 bytes --]

On Sat,  9 Jul 2011 09:38:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> These paths hold onto the struct mutex whilst accessing pages. In
> order, to prevent a recursive dead-lock should we fault-in a GTT mapped
> page we need to return -EFAULT and fallback to the slow path.
> 
> Lockdep has complained before about the potential dead-lock, but rvis is
> the first application found to sufficiently abuse the API to trigger it.
> 
> Cursory performance regression testing on a 1GiB PineView system using
> x11perf, cairo-perf-trace, glxgears and a few game benchmarks suggested
> no large regressions with just a 2% slowdown for firefox. The caveat is
> that this was an otherwise idle system and that for 32-bit systems
> io_mapping_map_atomic_wc() already disabled page-faults.
> 
> Bugzilla: 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 |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 2fce620..ecb27fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c

> -		vaddr = kmap_atomic(page, KM_USER0);
> +		vaddr = kmap_atomic(page);
> +		/* We have to disable faulting here in case the user address
> +		 * is really a GTT mapping and so we can not enter
> +		 * i915_gem_fault() whilst already holding struct_mutex.
> +		 */
> +		pagefault_disable();
>  		ret = __copy_from_user_inatomic(vaddr + page_offset,
>  						user_data,
>  						page_length);
> -		kunmap_atomic(vaddr, KM_USER0);
> +		pagefault_enable();
> +		kunmap_atomic(vaddr);

does this even compile?  Looks like you dropped an arg.

Looks like a reasonable fix for the problem, though.

[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Chris Wilson]

On Sat, 09 Jul 2011 07:41:57 -0700, Eric Anholt <eric@anholt.net> wrote:
> On Sat,  9 Jul 2011 09:38:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> 
> > -		vaddr = kmap_atomic(page, KM_USER0);
> > +		vaddr = kmap_atomic(page);
> > +		/* We have to disable faulting here in case the user address
> > +		 * is really a GTT mapping and so we can not enter
> > +		 * i915_gem_fault() whilst already holding struct_mutex.
> > +		 */
> > +		pagefault_disable();
> >  		ret = __copy_from_user_inatomic(vaddr + page_offset,
> >  						user_data,
> >  						page_length);
> > -		kunmap_atomic(vaddr, KM_USER0);
> > +		pagefault_enable();
> > +		kunmap_atomic(vaddr);
> 
> does this even compile?  Looks like you dropped an arg.

That parameter was removed several months ago and although a pass was made
through the kernel to update all callsites, this one inexplicably remained.

commit t 3e4d3af501cccdc8a8cca41bdbe57d54ad7e7e73
Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
Date:   Tue Oct 26 14:21:51 2010 -0700

    mm: stack based kmap_atomic()

-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 625 bytes --]

On Sat,  9 Jul 2011 09:38:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> +		/* We have to disable faulting here in case the user address
> +		 * is really a GTT mapping and so we can not enter
> +		 * i915_gem_fault() whilst already holding struct_mutex.
> +		 */

I would (far, far) rather disallow pread through the GTT
mapping. There's no credible reason to allow it. Is there some
reasonably fast way to detect that these addresses are within the GTT
and just bail?

Any performance penalty that serves solely to enable abuse of the
interface is not reasonable.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Chris Wilson]

On Sat, 09 Jul 2011 13:24:02 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Sat,  9 Jul 2011 09:38:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > +		/* We have to disable faulting here in case the user address
> > +		 * is really a GTT mapping and so we can not enter
> > +		 * i915_gem_fault() whilst already holding struct_mutex.
> > +		 */
> 
> I would (far, far) rather disallow pread through the GTT
> mapping. There's no credible reason to allow it. Is there some
> reasonably fast way to detect that these addresses are within the GTT
> and just bail?

Something like:

vma = find_vma(current->mm, uaddr);
if (vma->vm_ops == dev->driver->gem_vm_ops)
	return -EINVAL;

I think would do, find_vma() is not necessary cheap though, and there are a
couple of optimisations that we haven't done for pwrite/pread yet to speed
up the transition to the slow path.

> Any performance penalty that serves solely to enable abuse of the
> interface is not reasonable.

The current code generates lockdep OOPSes and inconsistently applies
pagefault_disable along some paths, in particular for 32-bit kernels,
but not others. And the abuse is permitted through the OpenGL
specification, I believe. The offending app is just doing
glBufferData(glMapBuffer()), iiuc;
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1014 bytes --]

On Sat, 09 Jul 2011 21:50:26 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> I think would do, find_vma() is not necessary cheap though, and there are a
> couple of optimisations that we haven't done for pwrite/pread yet to speed
> up the transition to the slow path.

Yeah, find_vma is a rb tree walk over the whole address space. Yikes!
And, of course, we'd actually need to walk over the whole mapping in
case the application managed to walk from non-GTT space into GTT space.

> The current code generates lockdep OOPSes and inconsistently applies
> pagefault_disable along some paths, in particular for 32-bit kernels,
> but not others. And the abuse is permitted through the OpenGL
> specification, I believe. The offending app is just doing
> glBufferData(glMapBuffer()), iiuc;

Sure, it's permitted, so ideally we'd detect this abuse and fall back to
the slow path, but we need a cheap check which takes the slow path,
perhaps pessimistically.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Chris Wilson]

On Sat, 09 Jul 2011 14:06:23 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Sat, 09 Jul 2011 21:50:26 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Sure, it's permitted, so ideally we'd detect this abuse and fall back to
> the slow path, but we need a cheap check which takes the slow path,
> perhaps pessimistically.

If we prefault every page, then we only hit the slow path under system
load combined or severe memory pressure. I don't think that is too bad a
compromise.

I can stick some counters in there and find out what the impact actually
is.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 958 bytes --]

On Sat, 09 Jul 2011 22:23:28 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Sat, 09 Jul 2011 14:06:23 -0700, Keith Packard <keithp@keithp.com> wrote:
> > On Sat, 09 Jul 2011 21:50:26 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Sure, it's permitted, so ideally we'd detect this abuse and fall back to
> > the slow path, but we need a cheap check which takes the slow path,
> > perhaps pessimistically.
> 
> If we prefault every page, then we only hit the slow path under system
> load combined or severe memory pressure. I don't think that is too bad a
> compromise.
> 
> I can stick some counters in there and find out what the impact actually
> is.

Cool. I've separately posted a query to lkml asking if the existing
fault_in_pages_writeable/fault_in_pages_readable should be fixed - that
looks like it will provide a performance boost for filesystem reads
larger than two pages.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Chris Wilson]

On Sat, 09 Jul 2011 15:07:28 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Sat, 09 Jul 2011 22:23:28 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > I can stick some counters in there and find out what the impact actually
> > is.

On a SNB laptop, so surplus memory and surplus processing power, we don't
see that many faults at all whilst messing around with firefox, gitk and a
couple of timedemos (padman, openarena):

no-prefaulting:
  pread: shmem fast: 1273764/3395, slow: 0/0
  pwrite: sheme fast: 51163148/14554, slow: 23552/9
  pwrite: gtt fast: 32744702068/12658489, slow: 29376/10

all-prefaulted:
  pread: shmem fast: 6081544/5999, slow: 0/0
  pwrite: shmem fast: 56867580/15932, slow: 0/0
  pwrite: gtt fast: 32976168476/12753355, slow: 0/0

I'll look at a PNV netbook next.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 384 bytes --]

On Mon, 11 Jul 2011 17:51:08 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> no-prefaulting:
>   pread: shmem fast: 1273764/3395, slow: 0/0
>   pwrite: sheme fast: 51163148/14554, slow: 23552/9
>   pwrite: gtt fast: 32744702068/12658489, slow: 29376/10

Looks like it won't matter that much, at least when you've got plenty of memory.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 911 bytes --]

On Sat, 09 Jul 2011 13:24:02 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Sat,  9 Jul 2011 09:38:51 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > +		/* We have to disable faulting here in case the user address
> > +		 * is really a GTT mapping and so we can not enter
> > +		 * i915_gem_fault() whilst already holding struct_mutex.
> > +		 */
> 
> I would (far, far) rather disallow pread through the GTT
> mapping. There's no credible reason to allow it. Is there some
> reasonably fast way to detect that these addresses are within the GTT
> and just bail?

That means that I can't give users of GL pointers to GTT mappings for
the buffer mapping API, because then I'd have to check in userland
whether the pointer they give me for other API entrypoints is to a known
GTT mapping before doing a pread into it.  And then I imagine cross-API
interactions and shudder.


[-- Attachment #1.2: Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Disable page-faults around the fast pwrite/pread paths

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 617 bytes --]

On Sun, 10 Jul 2011 11:45:29 -0700, Eric Anholt <eric@anholt.net> wrote:

> That means that I can't give users of GL pointers to GTT mappings for
> the buffer mapping API, because then I'd have to check in userland
> whether the pointer they give me for other API entrypoints is to a known
> GTT mapping before doing a pread into it.  And then I imagine cross-API
> interactions and shudder.

Yeah, it clearly has to work; it's not clear how fast it needs to
be. However, if we get the prefaulting stuff fixed, we should still be
able to hit the fast path most of the time.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 542 bytes --]

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).

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[Ben Widawsky]

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

Re: [PATCH 1/2] drm/i915: Prefault all pages for pread and pwrite

[Chris Wilson]

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

-- 
Chris Wilson, Intel Open Source Technology Centre