[Mesa-dev] [PATCH] anv/allocator: Unconditionally call futex_wake

[Scott D Phillips]

Jason Ekstrand <jason at jlekstrand.net> writes:

> There is a potential race between the __sync_fetch_and_add and the
> futex_wake where another thread could come in and start waiting.  If we
> hit this case, the other thread will never get woken back up because the
> futex_wake doesn't get called.  We can fix this by calling futex_wake
> unconditionally.
>
> There was another potential bug because __sync_fetch_and_add does not
> guarantee that previous writes are globally visible.

__sync_fetch_and_add is a 'lock xadd' instruction,
__sync_lock_test_and_set is an 'xchg' instruction, both of which are
locked. So the updates to the pool can't be reordered after the xchg in
the growing thread, and the loads of the pool can't be reordered before
the lock xadd in the allocing thread.

It looks like another reordering could happen though, where one thread
goes through the first case of the `if` (just bumping the pointer and
returning). Then a next thread could come in and goes through the second
case of the `if` where it starts growing the pool. The first thread's
loads of the pool's fields like map, center_bo_offset, and gem_handle
might be insconsistent then. I think that can happen, I haven't checked
what the full implication is if it can.

>                                                       In particular, the
> updates to the pool caused by growing it may not be visible.  If memory
> writes from the growing thread happen out-of-order, this could cause a
> waiting thread to come in and try to pull a block before the grow has
> completed.  Now that we are no longer predicating the futex_wake, we no
> longer need the result of the __sync_fetch_and_add and it can be
> replaced with a __sync_synchronize and a regular 64-bit write.
> ---
>  src/intel/vulkan/anv_allocator.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_allocator.c b/src/intel/vulkan/anv_allocator.c
> index fe14d6c..a64ebd0 100644
> --- a/src/intel/vulkan/anv_allocator.c
> +++ b/src/intel/vulkan/anv_allocator.c
> @@ -546,7 +546,7 @@ anv_block_pool_alloc_new(struct anv_block_pool *pool,
>                           struct anv_block_state *pool_state,
>                           uint32_t block_size)
>  {
> -   struct anv_block_state state, old, new;
> +   struct anv_block_state state, new;
>  
>     while (1) {
>        state.u64 = __sync_fetch_and_add(&pool_state->u64, block_size);
> @@ -564,9 +564,9 @@ anv_block_pool_alloc_new(struct anv_block_pool *pool,
>              new.end = anv_block_pool_grow(pool, pool_state);
>           } while (new.end < new.next);
>  
> -         old.u64 = __sync_lock_test_and_set(&pool_state->u64, new.u64);

If another thread comes and updates state between
__sync_lock_test_and_set() and futex_wake() then it would have fetched a
state where next <= end, so the other thread can't have gone into the
futex_wait case. It would either have just bumped the next offset or
gone into the growing case. If it has started a new grow operation then
we wouldn't want to wake the waiters of that new, other grow.

> -         if (old.next != state.next)
> -            futex_wake(&pool_state->end, INT_MAX);
> +         __sync_synchronize();
> +         pool_state->u64 = new.u64;
> +         futex_wake(&pool_state->end, INT_MAX);
>           return state.next;
>        } else {
>           futex_wait(&pool_state->end, state.end, NULL);