[PATCH 06/27] drm/etnaviv: get rid of userptr worker

[Lucas Stach]

Am Freitag, den 01.12.2017, 16:51 +0000 schrieb Russell King - ARM
Linux:
> On Fri, Dec 01, 2017 at 05:38:51PM +0100, Philipp Zabel wrote:
> > On Fri, 2017-12-01 at 11:36 +0100, Lucas Stach wrote:
> > > All code paths which populate userptr BOs are fine with the
> > > get_pages
> > > function taking the mmap_sem lock. This allows to get rid of the
> > > pretty
> > > involved architecture with a worker being scheduled if the
> > > mmap_sem
> > > needs to be taken, but instead call GUP directly and allow it to
> > > take
> > > the lock if necessary.
> > > 
> > > This simplifies the code a lot and removes the possibility of
> > > this
> > > function returning -EAGAIN, which complicates object population
> > > handling at the callers.
> > > 
> > > Signed-off-by: Lucas Stach <l.stach at pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 146 +++++---------------
> > > --------------
> > >  drivers/gpu/drm/etnaviv/etnaviv_gem.h |   3 +-
> > >  2 files changed, 23 insertions(+), 126 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > index a52220eeee45..fcc969fa0e69 100644
> > > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> > > @@ -705,141 +705,41 @@ int etnaviv_gem_new_private(struct
> > > drm_device *dev, size_t size, u32 flags,
> > >  	return 0;
> > >  }
> > >  
> > > -struct get_pages_work {
> > > -	struct work_struct work;
> > > -	struct mm_struct *mm;
> > > -	struct task_struct *task;
> > > -	struct etnaviv_gem_object *etnaviv_obj;
> > > -};
> > > -
> > > -static struct page **etnaviv_gem_userptr_do_get_pages(
> > > -	struct etnaviv_gem_object *etnaviv_obj, struct mm_struct
> > > *mm, struct task_struct *task)
> > > -{
> > > -	int ret = 0, pinned, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > > -	struct page **pvec;
> > > -	uintptr_t ptr;
> > > -	unsigned int flags = 0;
> > > -
> > > -	pvec = kvmalloc_array(npages, sizeof(struct page *),
> > > GFP_KERNEL);
> > > -	if (!pvec)
> > > -		return ERR_PTR(-ENOMEM);
> > > -
> > > -	if (!etnaviv_obj->userptr.ro)
> > > -		flags |= FOLL_WRITE;
> > > -
> > > -	pinned = 0;
> > > -	ptr = etnaviv_obj->userptr.ptr;
> > > -
> > > -	down_read(&mm->mmap_sem);
> > > -	while (pinned < npages) {
> > > -		ret = get_user_pages_remote(task, mm, ptr,
> > > npages - pinned,
> > > -					    flags, pvec +
> > > pinned, NULL, NULL);
> > > -		if (ret < 0)
> > > -			break;
> > > -
> > > -		ptr += ret * PAGE_SIZE;
> > > -		pinned += ret;
> > > -	}
> > > -	up_read(&mm->mmap_sem);
> > > -
> > > -	if (ret < 0) {
> > > -		release_pages(pvec, pinned);
> > > -		kvfree(pvec);
> > > -		return ERR_PTR(ret);
> > > -	}
> > > -
> > > -	return pvec;
> > > -}
> > > -
> > > -static void __etnaviv_gem_userptr_get_pages(struct work_struct
> > > *_work)
> > > -{
> > > -	struct get_pages_work *work = container_of(_work,
> > > typeof(*work), work);
> > > -	struct etnaviv_gem_object *etnaviv_obj = work-
> > > >etnaviv_obj;
> > > -	struct page **pvec;
> > > -
> > > -	pvec = etnaviv_gem_userptr_do_get_pages(etnaviv_obj,
> > > work->mm, work->task);
> > > -
> > > -	mutex_lock(&etnaviv_obj->lock);
> > > -	if (IS_ERR(pvec)) {
> > > -		etnaviv_obj->userptr.work = ERR_CAST(pvec);
> > > -	} else {
> > > -		etnaviv_obj->userptr.work = NULL;
> > > -		etnaviv_obj->pages = pvec;
> > > -	}
> > > -
> > > -	mutex_unlock(&etnaviv_obj->lock);
> > > -	drm_gem_object_put_unlocked(&etnaviv_obj->base);
> > > -
> > > -	mmput(work->mm);
> > > -	put_task_struct(work->task);
> > > -	kfree(work);
> > > -}
> > > -
> > >  static int etnaviv_gem_userptr_get_pages(struct
> > > etnaviv_gem_object *etnaviv_obj)
> > >  {
> > >  	struct page **pvec = NULL;
> > > -	struct get_pages_work *work;
> > > -	struct mm_struct *mm;
> > > -	int ret, pinned, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > > +	struct etnaviv_gem_userptr *userptr = &etnaviv_obj-
> > > >userptr;
> > > +	int ret, pinned = 0, npages = etnaviv_obj->base.size >>
> > > PAGE_SHIFT;
> > >  
> > >  	might_lock_read(&current->mm->mmap_sem);
> > >  
> > > -	if (etnaviv_obj->userptr.work) {
> > > -		if (IS_ERR(etnaviv_obj->userptr.work)) {
> > > -			ret = PTR_ERR(etnaviv_obj-
> > > >userptr.work);
> > > -			etnaviv_obj->userptr.work = NULL;
> > > -		} else {
> > > -			ret = -EAGAIN;
> > > -		}
> > > -		return ret;
> > > -	}
> > > +	if (userptr->mm != current->mm)
> > > +		return -EPERM;
> > 
> > I don't pretend to understand the implications of this patch
> > completely.
> > It looks mostly good to me, but this part seems to limit previously
> > allowed behaviour. I think this warrants a mention in the commit
> > message.
> 
> The point of the complexity is to be able to grab the mmap_sem
> avoiding
> any locking dependencies that would normally occur if it were done in
> the ioctl.
> 
> However, drm locking has changed quite a bit over the last year, and
> this is probably not be needed anymore - I assume that Lucas has
> thoroughly verified the locking dependencies.  However, I notice i915
> still retains this code though.

Yes, I have. Both by going through, reading a lot of the userptr
related core kernel code and by improving lockdeps ability to reason
about the locking used inside of etnaviv.

i915 still retains this code, as they are still using the "big DRM
lock" struct_mutex. Etnaviv has a much more fine-grained locking
scheme, allowing us to drop this complexity.

Philipp is right in that this change has a functional change by
rejecting attempts to populate userptr objects created by foreign
processes. Something which was allowed before, but would have been
pretty broken in practice if anyone dared to do so, as it violates the
coherency rules set by the etnaviv GEM object handling.

Regards,
Lucas