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

[Daniel Vetter]

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

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