On Fri, Feb 23, 2018 at 9:26 AM, Scott D Phillips <
scott.d.phill...@intel.com> wrote:

> Jason Ekstrand <ja...@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.
>

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.


> 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.
>

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.


> >                                                       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.
>

Your argument seems sound.  Perhaps this is unneeded.


> > -         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);
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to