Re: [PATCH 2/2] drm/i915: properly prefault for pread/pwrite

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 28 Sep 2011 11:57:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The helper functions used are designed for pagecache io and splice,
> i.e. they prefault at most PAGE_SIZE bytes spanning at most 2 pages.
> 
> pread/pwrite want to write/read much more to avoid dropping the
> struct_mutex lock in between. So write our helper function to prefault.
> We're the only user of these pagemap.h helpers that want this behaviour,
> so keep these new helpers private.
> 
> Based on a patch by Chris Wilson. In addition to his approach this
> alos tries to prefault the last page in case the user address range
> crosses a page boundary at the end (and hence might sit on n+1 pages
> for at most n*PAGE_SIZE of date).
> 
> As a nice side-effect this rather reliably papers over the current
> code's inability to handle non-struct page-backed user memory in the
> slow paths. Because the real fix is grossly invasive, I think this
> patch is the right thing for backporting.
> 
> Cc: stable@kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=38115
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   59 ++++++++++++++++++++++++++++++++++++--
>  1 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 5f0f46e..42dc922 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -503,6 +503,32 @@ out:
>  	return ret;
>  }
>  
> +static inline int __prefault_writeable(char __user *uaddr, int size)
> +{
> +	int ret;
> +	char __user *end = uaddr + size - 1;
> +
> +	if (unlikely(size == 0))
> +		return 0;
> +
> +	/*
> +	 * Writing zeroes into userspace here is OK, because we know that if
> +	 * the zero gets there, we'll be overwriting it.
> +	 */
> +	while (uaddr <= end) {
> +		ret = __put_user(0, uaddr);
> +		if (ret != 0)
> +			return ret;
> +		uaddr += PAGE_SIZE;
> +	}
> +	if (ret == 0) {
> +		if (((unsigned long)uaddr & PAGE_MASK) !=
> +				((unsigned long)end & PAGE_MASK))
This is a little ugly. Perhaps,

  #define page_align(addr) ((unsigned long)addr & PAGE_MASK)
  if (page_align(uaddr) != page_align(end))

Otherwise, both are
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre