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.ursu...@intel.com>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx