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@intel.com
Regards,
Tvrtko