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

[Chris Wilson]

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.

It's tricky to know what the right thing to do is. For example, another
quirk is that we can recover a failed get_pages() by repeatedly invoking
it after a new aliasing. Again, I'm not sure if the current behaviour is
a little too lax.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre