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

[Jason Ekstrand]

On Wed, Mar 15, 2017 at 8:37 AM, Chris Wilson <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]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!


> >    How about if I just check the refcount again after I've entered the
> mutex
> >    and release the mutex and bail if it isn't zero?  The only time the
> >    reference count is ever imported is in import and it's done inside the
> >    mutex.  That's the cheapest option.
>
> The second thread (that acquired the ref from the cache) may also drop
> it before you acquire the mutex. You now have two threads that see
> bo->refcount as 0 inside anv_bo_cache_release().
>

Ugh... You're right.  Concurrency is hard...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170315/54833e27/attachment.html>