[Mesa-dev] [PATCH v2 07/18] anv/allocator: Add a BO cache

[Chris Wilson]

On Wed, Mar 15, 2017 at 08:54:40AM -0700, Jason Ekstrand wrote:
>    On Wed, Mar 15, 2017 at 8:37 AM, Chris Wilson
>    <[1]chris at chris-wilson.co.uk> wrote:
> 
>      On Wed, Mar 15, 2017 at 08:28:24AM -0700, Jason Ekstrand wrote:
>      >    On Wed, Mar 15, 2017 at 5:21 AM, Chris Wilson
>      >    <[1][2]chris at chris-wilson.co.uk> wrote:
>      >
>      >      On Wed, Mar 15, 2017 at 09:50:57AM +0000, Chris Wilson wrote:
>      >      > On Tue, Mar 14, 2017 at 10:43:07PM -0700, Jason Ekstrand wrote:
>      >      > > +void
>      >      > > +anv_bo_cache_release(struct anv_device *device,
>      >      > > +                     struct anv_bo_cache *cache,
>      >      > > +                     struct anv_bo *bo_in,
>      >      > > +                     VkAllocationCallbacks *alloc)
>      >      > > +{
>      >      > > +   assert(anv_bo_cache_lookup(cache, bo_in->gem_handle) ==
>      bo_in);
>      >      > > +   struct anv_cached_bo *bo = (struct anv_cached_bo *)bo_in;
>      >      > > +
>      >      > > +   uint32_t count = __sync_fetch_and_add(&bo->refcount, -1);
>      >      > > +   assert(count > 0);
>      >      > > +   if (count > 1)
>      >      > > +      return;
>      >      > > +   assert(count == 1);
>      >      > > +   assert(bo->refcount == 0);
>      >      >
>      >      > There's a window here for a second thread to acquire a
>      reference to
>      >      the
>      >      > bo, just before you then go and free it.
>      >      >
>      >      > The trick is the final unref has to be inside the mutex,
>      earlier can
>      >      be
>      >      > outside. See refcount_dec_and_lock() in the kernel.
>      >
>      >      The alternative is to check against the dead bo when retrieving
>      from the
>      >      cache (under the mutex) -- refcount_add_not_zero. That is
>      probably the
>      >      cheaper option.
> 
>    I don't think that will actually work.  I assume that what you mean is to
>    do refcount_add_not_zero and, if it fails (because the refcount is
>    currently zero), then we treat it as if the cached BO has already been
>    deleted.  The problem is that, if we hit this case, we've already acquired
>    a GEM handle and if we let the release go ahead and delete the cached BO,
>    then our gem handle will get deleted.  I think I need to go with option
>    1.  Thanks for all your review!

Yeah, you would have to pass on the unexpected error. More complicated
dance around unref it is, have fun! :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre