<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Feb 23, 2018 at 9:26 AM, Scott D Phillips <span dir="ltr"><<a href="mailto:scott.d.phillips@intel.com" target="_blank">scott.d.phillips@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> writes:<br>
<br>
> There is a potential race between the __sync_fetch_and_add and the<br>
> futex_wake where another thread could come in and start waiting.  If we<br>
> hit this case, the other thread will never get woken back up because the<br>
> futex_wake doesn't get called.  We can fix this by calling futex_wake<br>
> unconditionally.<br>
><br>
> There was another potential bug because __sync_fetch_and_add does not<br>
> guarantee that previous writes are globally visible.<br>
<br>
</span>__sync_fetch_and_add is a 'lock xadd' instruction,<br>
__sync_lock_test_and_set is an 'xchg' instruction, both of which are<br>
locked. So the updates to the pool can't be reordered after the xchg in<br>
the growing thread, and the loads of the pool can't be reordered before<br>
the lock xadd in the allocing thread.<br></blockquote><div><br></div><div>They can't be re-ordered after the exchange, but it also doesn't guarantee that they've happened.  What happens if the exchange is on a hot cache line but some of the other writes before it are not.  In that case, based on my reading of the GCC docs (which may not be all that good), the exchange could return *before* those writes are completed and some other thread could (if it has a hot cache line) come in and read before the data is updated.  There are a lot of caching details that would have to fall into place to make that happen but the possibility scares me.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
It looks like another reordering could happen though, where one thread<br>
goes through the first case of the `if` (just bumping the pointer and<br>
returning). Then a next thread could come in and goes through the second<br>
case of the `if` where it starts growing the pool. The first thread's<br>
loads of the pool's fields like map, center_bo_offset, and gem_handle<br>
might be insconsistent then. I think that can happen, I haven't checked<br>
what the full implication is if it can.<br><div><div class="h5"></div></div></blockquote><div><br></div><div>Yes, that can happen.  However, center_bo_offset and gem_handle are only really used inside the giant device-level mutex lock so they are safe.  The map, however, isn't.  This is one of the reasons why we never throw away any block pool maps.  The old maps persist until the end of time.<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">
>                                                       In particular, the<br>
> updates to the pool caused by growing it may not be visible.  If memory<br>
> writes from the growing thread happen out-of-order, this could cause a<br>
> waiting thread to come in and try to pull a block before the grow has<br>
> completed.  Now that we are no longer predicating the futex_wake, we no<br>
> longer need the result of the __sync_fetch_and_add and it can be<br>
> replaced with a __sync_synchronize and a regular 64-bit write.<br>
> ---<br>
>  src/intel/vulkan/anv_<wbr>allocator.c | 17 +++++++++--------<br>
>  1 file changed, 9 insertions(+), 8 deletions(-)<br>
><br>
> diff --git a/src/intel/vulkan/anv_<wbr>allocator.c b/src/intel/vulkan/anv_<wbr>allocator.c<br>
> index fe14d6c..a64ebd0 100644<br>
> --- a/src/intel/vulkan/anv_<wbr>allocator.c<br>
> +++ b/src/intel/vulkan/anv_<wbr>allocator.c<br>
> @@ -546,7 +546,7 @@ anv_block_pool_alloc_new(<wbr>struct anv_block_pool *pool,<br>
>                           struct anv_block_state *pool_state,<br>
>                           uint32_t block_size)<br>
>  {<br>
> -   struct anv_block_state state, old, new;<br>
> +   struct anv_block_state state, new;<br>
><br>
>     while (1) {<br>
>        state.u64 = __sync_fetch_and_add(&pool_<wbr>state->u64, block_size);<br>
> @@ -564,9 +564,9 @@ anv_block_pool_alloc_new(<wbr>struct anv_block_pool *pool,<br>
>              new.end = anv_block_pool_grow(pool, pool_state);<br>
>           } while (new.end < new.next);<br>
><br>
> -         old.u64 = __sync_lock_test_and_set(&<wbr>pool_state->u64, new.u64);<br>
<br>
</div></div>If another thread comes and updates state between<br>
__sync_lock_test_and_set() and futex_wake() then it would have fetched a<br>
state where next <= end, so the other thread can't have gone into the<br>
futex_wait case. It would either have just bumped the next offset or<br>
gone into the growing case. If it has started a new grow operation then<br>
we wouldn't want to wake the waiters of that new, other grow.<br><div class="HOEnZb"><div class="h5"></div></div></blockquote><div><br></div><div>Your argument seems sound.  Perhaps this is unneeded.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">
> -         if (old.next != state.next)<br>
> -            futex_wake(&pool_state->end, INT_MAX);<br>
> +         __sync_synchronize();<br>
> +         pool_state->u64 = new.u64;<br>
> +         futex_wake(&pool_state->end, INT_MAX);<br>
>           return state.next;<br>
>        } else {<br>
>           futex_wait(&pool_state->end, state.end, NULL);<br>
</div></div></blockquote></div><br></div></div>