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

[Chris Wilson]

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.)
-Chris