[Intel-gfx] [PATCH 1/3] drm/i915: Only update the current userptr worker

[Chris Wilson]

On Wed, Jul 01, 2015 at 01:26:59PM +0100, Tvrtko Ursulin wrote:
> 
> On 07/01/2015 12:09 PM, Chris Wilson wrote:
> >On Wed, Jul 01, 2015 at 11:58:46AM +0100, Tvrtko Ursulin wrote:
> >>On 07/01/2015 10:59 AM, Chris Wilson wrote:
> >>>On Wed, Jul 01, 2015 at 10:48:59AM +0100, Tvrtko Ursulin wrote:
> >>>>Previously the canceled worker would allow another worker to be
> >>>>created in case it failed (obj->userptr.work != &work->work; ret =
> >>>>0;) and now it still does since obj->userptr.work remains at NULL
> >>>>from cancellation.
> >>>>
> >>>>Both seem wrong, am I missing the change?
> >>>
> >>>No, the obj->userptr.work must remain NULL until a new get_pages()
> >>>because we don't actually know if this worker's gup was before or after
> >>>the cancellation  - mmap_sem vs struct_mutex ordering.
> >>
> >>No one is not wrong, or no I was not missing the change?
> >
> >The only change is that we don't change the value of userptr.work if it
> >is set to something else. The only time it should be different was if it
> >had been cancelled and so NULL. The patch just makes it so that a coding
> >error is less damaging - and I think easier to read because of that.
> >
> >>I am thinking more and more that we should just mark it canceled
> >>forever and not allow get_pages to succeed ever since.
> >
> >Yes, I toyed with that yesterday in response to you being able to alias
> >a GTT mmap address with the userptr after munmap(userptr.ptr). The
> >problem is that cancel_userptr() is caller for any change in the CPU
> >PTE's, including mprotect() or cow after forking. Both of those are
> >valid situations where we want to keep the userptr around, but with a
> >new gup.
> 
> Why do we want that? I would be surprised if someone is using it
> like that. How would it be defined on the GEM handle level even?

I would be surprised as well, but it is a race condition we can handle
correctly and succinctly.

The race is just
	bo = userptr(ptr, size);
	set-to-domain(bo);
	mremap(ptr, newptr, size);
	set-to-domain(bo); // or exec(bo);
-Chris


-- 
Chris Wilson, Intel Open Source Technology Centre