[Intel-gfx] [PATCH 5/5] drm/mm: Use clear_bit_unlock() for releasing the drm_mm_node()

[Tvrtko Ursulin]

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 at intel.com>

Regards,

Tvrtko