<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>On 9/19/19 12:43 PM, Derek Lesho wrote:<br>
</p>
<blockquote type="cite"
cite="mid:184bb838-13c8-cc50-a112-4fb649a4e1a6@codeweavers.com">
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
On 9/19/19 11:12 AM, Conor McCarthy wrote:<br>
<blockquote type="cite"
cite="mid:CAOua6-8qdBw5P7swoKB0+19Feyg4bW0isG1EijfgHFWQezuUmA@mail.gmail.com">
<meta http-equiv="content-type" content="text/html;
charset=UTF-8">
<div dir="ltr">
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Sep 20, 2019 at
1:32 AM Derek Lesho <<a
href="mailto:dlesho@codeweavers.com"
moz-do-not-send="true">dlesho@codeweavers.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">Also, in your patch, I
noticed that you intentionally unlock before <br>
performing the copy or overwrite. This puzzles me, if one
thread waits <br>
on the global mutex for access to a descriptor's view to
dereference, <br>
and we unlock the mutex for them when the view is NULL,
they may crash <br>
when trying to decrement the reference of the NULL view.<br>
</blockquote>
<div><br>
</div>
<div>Yes, it's missing a null check after locking the mutex.
If it's null after locking then the function can return
because it means another thread has destroyed the
descriptor. I think the only remaining problem is the old
one of a single mutex being used for all descriptors.
Maybe performance can be improved.<br>
</div>
</div>
</div>
</blockquote>
Even if one thread destroys a descriptor, and then we let both
concurrently write their data, we will leak the Vulkan handle of
written by the thread which writes it to the descriptor first and
gets overwritten.<br>
</blockquote>
<p>I did some testing with your patch as is, and compared it with a
solution where we destroy the vulkan handle after unlocking the
global mutex. The difference was within the margin of error.</p>
<p>I also tried adding a dedicated mutex for this purpose, but that
didn't have much of an affect on performance either. A dedicated
spinlock actually worsened performance, even without the Vulkan
call inside the lock.<br>
</p>
<p>If we can confirm that no resource leakage is occurring, the
patch you sent is probably fine as is.</p>
</body>
</html>