[Intel-gfx] [PATCH 24/30] drm/i915: Refactor pwrite/pread to use single copy of get_user_pages

[Daniel Vetter]

On Wed, Apr 13, 2011 at 08:56:26PM +0100, Chris Wilson wrote:
> On Wed, 13 Apr 2011 21:26:24 +0200, Daniel Vetter <daniel at ffwll.ch> wrote:
> > On Tue, Apr 12, 2011 at 09:31:52PM +0100, Chris Wilson wrote:
> > > Replace the three nearly identical copies of the code with a single
> > > function. And take advantage of the opportunity to do some
> > > micro-optimisation: avoid the vmalloc if at all possible and also avoid
> > > dropping the lock unless we are forced to acquire the mm semaphore.
> > 
> > One tiny nitpick: Perhaps put an api comment at the top of
> > gem_get_user_pages that this function drops the struct_mutex. That's not
> > something we normally do and could cause endless amounts of fun if
> > neglected.
> 
> How about:
> 
> /**
>  * Magically retrieves the pages for the user addr whilst holding the
>  * dev->struct_mutex.
>  *
>  * Since we can not take the mm semaphore whilst holding our dev->struct_mutex,
>  * due to the pre-existing lock dependency established by i915_gem_fault(),
>  * we have to perform some sleight-of-hand.
>  *
>  * First, we try the lockless variant of gup whilst continuing to hold the
>  * mutex. If that fails to get all the user pages, then we no choice but
>  * to acquire the mm semaphore (thus dropping the lock on dev->struct_mutex
>  * to do so). The dev->struct_mutex is then re-acquired before we return.
>  *
>  * Returns: an error code *and* the number of user pages acquired. Even
>  * on an error, you must iterate over the return pages and release them.
>  */

Perfect. And just reminded me that my review wasn't too careful, I've
glossed a bit over that num_pages detail ...
-Daniel
-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48