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

[Scott D Phillips]

Jason Ekstrand <jason at jlekstrand.net> writes:

> On Fri, Feb 23, 2018 at 9:26 AM, Scott D Phillips <
> scott.d.phillips at intel.com> wrote:
>
> > 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.
> >
>
> 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.

I'm certainly not an expert here; I'm looking at Volume 3A of the System
Programming Guide. Section 8.2 where it is describing the conceptual
model of memory ordering and specifically 8.2.3.8 and 8.2.3.9 where it
illustrates ordering of locked instructions.

IIUC, the ordering model is saying that it's the system's job to handle
that case you mention, to keep the locked store from appearing to other
processors before those preceeding stores.

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

That's what I saw in a quick browse, and an old map or a new one is fine
for this purpose, so I can buy that this isn't a problem.