On Fri, Oct 04, 2019 at 01:01:36PM +0100, Tvrtko Ursulin wrote:
>
> On 04/10/2019 12:17, Chris Wilson wrote:
> > Quoting Chris Wilson (2019-10-04 12:07:10)
> > > Quoting Tvrtko Ursulin (2019-10-04 10:15:20)
> > > >
> > > > On 03/10/2019 22:01, Chris Wilson wrote:
> > > > > A few callers need to serialise the destruction of their drm_mm_node and
> > > > > ensure it is removed from the drm_mm before freeing. However, to be
> > > > > completely sure that any access from another thread is complete before
> > > > > we free the struct, we require the RELEASE semantics of
> > > > > clear_bit_unlock().
> > > > >
> > > > > This allows the conditional locking such as
> > > > >
> > > > > Thread A Thread B
> > > > > mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) {
> > > > > drm_mm_node_remove(node); mutex_lock(mm_lock);
> > > > > mutex_unlock(mm_lock); drm_mm_node_remove(node);
> > > > > mutex_unlock(mm_lock);
> > > > > }
> > > > > kfree(node);
> > > >
> > > > My understanding is that release semantics on node allocated mean 1 -> 0
> > > > transition is guaranteed to be seen only when thread A
> > > > drm_mm_node_remove is fully done with the removal. But if it is in the
> > > > middle of removal, node is still seen as allocated outside and thread B
> > > > can enter the if-body, wait for the lock, and then drm_mm_node_remove
> > > > will attempt a double removal. So I think another drm_mm_node_allocated
> > > > under the lock is needed.
> > >
> > > Yes. Check after the lock is indeed required in this scenario. And
> > > drm_mm_node_remove() insists the caller doesn't try a double remove.
> >
> > I had to go back and double check the vma code, and that's fine.
> > (We hit this case where one thread is evicting and another thread is
> > destroying the object. And for us we do the check under the lock inside
> > __i915_vma_unbind() on the destroy path.)
>
> So I think if you amend the commit message to contain the repeated check
> under the lock patch looks good to me. With that:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
I think a follow-up patch to update the kerneldoc to mention that we're
guaranteeing this now is missing here (best with the above fixed example).
Plus maybe a oneline code comment for the ALLOCATED_BIT.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
