<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Mar 15, 2017 at 8:37 AM, Chris Wilson <span dir="ltr"><<a href="mailto:chris@chris-wilson.co.uk" target="_blank">chris@chris-wilson.co.uk</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Wed, Mar 15, 2017 at 08:28:24AM -0700, Jason Ekstrand wrote:<br>
>    On Wed, Mar 15, 2017 at 5:21 AM, Chris Wilson<br>
</span><div><div class="h5">>    <[1]<a href="mailto:chris@chris-wilson.co.uk">chris@chris-wilson.co.uk</a>> wrote:<br>
><br>
>      On Wed, Mar 15, 2017 at 09:50:57AM +0000, Chris Wilson wrote:<br>
>      > On Tue, Mar 14, 2017 at 10:43:07PM -0700, Jason Ekstrand wrote:<br>
>      > > +void<br>
>      > > +anv_bo_cache_release(struct anv_device *device,<br>
>      > > +                     struct anv_bo_cache *cache,<br>
>      > > +                     struct anv_bo *bo_in,<br>
>      > > +                     VkAllocationCallbacks *alloc)<br>
>      > > +{<br>
>      > > +   assert(anv_bo_cache_lookup(<wbr>cache, bo_in->gem_handle) == bo_in);<br>
>      > > +   struct anv_cached_bo *bo = (struct anv_cached_bo *)bo_in;<br>
>      > > +<br>
>      > > +   uint32_t count = __sync_fetch_and_add(&bo-><wbr>refcount, -1);<br>
>      > > +   assert(count > 0);<br>
>      > > +   if (count > 1)<br>
>      > > +      return;<br>
>      > > +   assert(count == 1);<br>
>      > > +   assert(bo->refcount == 0);<br>
>      ><br>
>      > There's a window here for a second thread to acquire a reference to<br>
>      the<br>
>      > bo, just before you then go and free it.<br>
>      ><br>
>      > The trick is the final unref has to be inside the mutex, earlier can<br>
>      be<br>
>      > outside. See refcount_dec_and_lock() in the kernel.<br>
><br>
>      The alternative is to check against the dead bo when retrieving from the<br>
>      cache (under the mutex) -- refcount_add_not_zero. That is probably the<br>
>      cheaper option.<br></div></div></blockquote><div><br></div><div>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!<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
>    How about if I just check the refcount again after I've entered the mutex<br>
>    and release the mutex and bail if it isn't zero?  The only time the<br>
>    reference count is ever imported is in import and it's done inside the<br>
>    mutex.  That's the cheapest option.<br>
<br>
</div></div>The second thread (that acquired the ref from the cache) may also drop<br>
it before you acquire the mutex. You now have two threads that see<br>
bo->refcount as 0 inside anv_bo_cache_release().<br>
</blockquote><div><br></div><div>Ugh... You're right.  Concurrency is hard... <br></div></div><br></div></div>