Re: [PATCH v3] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Nov 18, 2015 at 05:18:30PM +0000, Tvrtko Ursulin wrote:
> 
> On 17/11/15 17:56, Daniel Vetter wrote:
> > On Tue, Nov 17, 2015 at 05:24:01PM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 17/11/15 17:08, Daniel Vetter wrote:
> >>> On Tue, Nov 17, 2015 at 04:54:50PM +0000, Tvrtko Ursulin wrote:
> >>>>
> >>>> On 17/11/15 16:39, Daniel Vetter wrote:
> >>>>> On Tue, Nov 17, 2015 at 04:27:12PM +0000, Tvrtko Ursulin wrote:
> >>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>>>>
> >>>>>> In the following commit:
> >>>>>>
> >>>>>>      commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
> >>>>>>      Author: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>>>>      Date:   Mon Oct 5 13:26:36 2015 +0100
> >>>>>>
> >>>>>>          drm/i915: Clean up associated VMAs on context destruction
> >>>>>>
> >>>>>> I added a WARN_ON assertion that VM's active list must be empty
> >>>>>> at the time of owning context is getting freed, but that turned
> >>>>>> out to be a wrong assumption.
> >>>>>>
> >>>>>> Due ordering of operations in i915_gem_object_retire__read, where
> >>>>>> contexts are unreferenced before VMAs are moved to the inactive
> >>>>>> list, the described situation can in fact happen.
> >>>>>>
> >>>>>> It feels wrong to do things in such order so this fix makes sure
> >>>>>> a reference to context is held until the move to inactive list
> >>>>>> is completed.
> >>>>>>
> >>>>>> v2: Rather than hold a temporary context reference move the
> >>>>>>      request unreference to be the last operation. (Daniel Vetter)
> >>>>>>
> >>>>>> v3: Fix use after free. (Chris Wilson)
> >>>>>>
> >>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638
> >>>>>> Cc: Michel Thierry <michel.thierry@xxxxxxxxx>
> >>>>>> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >>>>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/i915/i915_gem.c | 33 ++++++++++++++++++---------------
> >>>>>>   1 file changed, 18 insertions(+), 15 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >>>>>> index 98c83286ab68..094ac17a712d 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >>>>>> @@ -2404,29 +2404,32 @@ i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
> >>>>>>   	RQ_BUG_ON(!(obj->active & (1 << ring)));
> >>>>>>
> >>>>>>   	list_del_init(&obj->ring_list[ring]);
> >>>>>> -	i915_gem_request_assign(&obj->last_read_req[ring], NULL);
> >>>>>>
> >>>>>>   	if (obj->last_write_req && obj->last_write_req->ring->id == ring)
> >>>>>>   		i915_gem_object_retire__write(obj);
> >>>>>>
> >>>>>>   	obj->active &= ~(1 << ring);
> >>>>>> -	if (obj->active)
> >>>>>> -		return;
> >>>>>
> >>>>> 	if (obj->active) {
> >>>>> 		i915_gem_request_assign(&obj->last_read_req[ring], NULL);
> >>>>> 		return;
> >>>>> 	}
> >>>>>
> >>>>> Would result in less churn in the code and drop the unecessary indent
> >>>>> level. Also comment is missing as to why we need to do things in a
> >>>>> specific order.
> >>>>
> >>>> Actually I think I changed my mind and that v1 is the way to go.
> >>>>
> >>>> Just re-ordering the code here still makes it possible for the context
> >>>> destructor to run with VMAs on the active list I think.
> >>>>
> >>>> If we hold the context then it is 100% clear it is not possible.
> >>>
> >>> request_assign _is_ the function which adjust the refcounts for us, which
> >>> means if we drop that reference too early then grabbing a temp reference
> >>> is just papering over the real bug.
> >>>
> >>> Written out your patch looks something like
> >>>
> >>> 	a_reference(a);
> >>> 	a_unreference(a);
> >>>
> >>> 	/* more cleanup code that should get run before a_unreference but isn't */
> >>>
> >>> 	a_unrefernce(a); /* for real this time */
> >>>
> >>> Unfortunately foo_assign is a new pattern and not well-established, so
> >>> that connection isn't clear. Maybe we should rename it to
> >>> foo_reference_assign to make it more obvious. Or just drop the pretense
> >>> and open-code it since we unconditionally assign NULL as the new pointer
> >>> value, and we know the current value of the pointer is non-NULL. So
> >>> there's really no benefit to the helper here, it only obfuscates. And
> >>> since that obfuscation tripped you up it's time to remove it ;-)
> >>
> >> Then foo_reference_unreference_assign. :)
> >>
> >> But seriously, I think it is more complicated that..
> >>
> >> The thing it trips over is that moving VMAs to inactive does not correspond
> >> in time to request retirement. But in fact VMAs are moved to inactive only
> >> when all requests associated with an object are done.
> >>
> >> This is the unintuitive thing I was working around. To make sure when
> >> context destructor runs there are not active VMAs for that VM.
> >>
> >> I don't know how to guarantee that with what you propose. Perhaps I am
> >> missing something?
> > 
> > Ok, my example was slightly off, since we have 2 objects:
> > 
> > 	b_reference(a->b);
> > 	a_unreference(a); /* might unref a->b if it's the last reference */
> > 
> > 	/* more cleanup code that should get run before a_unreference but isn't */
> > 
> > 	b_unrefernce(a->b); /* for real this time */
> > 
> > Holding the ref to a makes sure that b doesn't disappear. We rely on that
> > in a fundamental way (a request really needs the ctx to stick around), and
> > the bug really is that we drop the ref to a too early. That it's the
> > releasing of a->b which is eventually blowing things up doesn't really
> > matter.
> > 
> > Btw would it be possible to have an igt for this? I should be possible to
> > hit this with some varian of gem_unref_active_buffers.
> 
> I was trying to do that today and it is proving to be a bit tricky.
> 
> I need a blitter workload which will run for long enough for the retire
> worker to run. So I'll try and build a bit bb tomorrow which will do that.
> 
> Alternatively I did this:
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 6ed7d63a0688..db51e4b42a20 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -699,6 +699,8 @@ i915_gem_execbuffer_reserve(struct intel_engine_cs *ring,
>         int retry;
>  
>         i915_gem_retire_requests_ring(ring);
> +       if (i915.enable_execlists)
> +               intel_execlists_retire_requests(ring);
>  
>         vm = list_first_entry(vmas, struct i915_vma, exec_list)->vm;
> 
> And that enables me to trigger the WARN from my igt even with the current
> shorter blitter workload (copy 64Mb).
> 
> Going back to the original problem, how about something like this hunk for a fix?
> 
> @@ -2413,19 +2416,36 @@ static void
>  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
>  {
>         struct i915_vma *vma;
> +       struct i915_hw_ppgtt *ppgtt;
>  
>         RQ_BUG_ON(obj->last_read_req[ring] == NULL);
>         RQ_BUG_ON(!(obj->active & (1 << ring)));
>  
>         list_del_init(&obj->ring_list[ring]);
> +
> +       ppgtt = obj->last_read_req[ring]->ctx->ppgtt;
> +       if (ppgtt) {
> +               list_for_each_entry(vma, &obj->vma_list, vma_link) {
> +                       if (vma->vm == &ppgtt->base &&
> +                           !list_empty(&vma->mm_list)) {
> +                               list_move_tail(&vma->mm_list,
> +                                              &vma->vm->inactive_list);
> +                       }
> +               }
> +       }
> +
>         i915_gem_request_assign(&obj->last_read_req[ring], NULL);
> 
> This moves VMAs immediately to inactive as requests are retired and avoids
> the problem with them staying on active for undefined amount of time.

You can't put active objects onto the inactive list, i.e. the obj->active
check is non-optional. And the if (ppgtt) case is abstraction violation.
I really don't get why we can't just move the unref to the right place ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux