data race reported in alSourceUnqueuBuffers

[maxbachmann]

Tsan reports

WARNING: ThreadSanitizer: data race (pid=586753)
  Write of size 8 at 0x72500008fbc0 by main thread (mutexes: write M0):
    #0 operator delete(void*, unsigned long) <null> (libtsan.so.2+0x69a1f) (BuildId: dc0ba0fbb4c898af5813c254dc49d26023dac0d0)
    #1 std::__new_allocator<ALbufferQueueItem>::deallocate(ALbufferQueueItem*, unsigned long) /usr/include/c++/16/bits/new_allocator.h:183 (test.so+0xcc727f) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)
    #2 std::allocator_traits<std::allocator<ALbufferQueueItem> >::deallocate(std::allocator<ALbufferQueueItem>&, ALbufferQueueItem*, unsigned long) /usr/include/c++/16/bits/alloc_traits.h:689 (test.so+0xcc727f)
    #3 std::_Deque_base<ALbufferQueueItem, std::allocator<ALbufferQueueItem> >::_M_deallocate_node(ALbufferQueueItem*) /usr/include/c++/16/bits/stl_deque.h:593 (test.so+0xcc727f)
    #4 std::deque<ALbufferQueueItem, std::allocator<ALbufferQueueItem> >::_M_pop_front_aux() /usr/include/c++/16/bits/deque.tcc:581 (test.so+0xcc727f)
    #5 std::deque<ALbufferQueueItem, std::allocator<ALbufferQueueItem> >::pop_front() /usr/include/c++/16/bits/stl_deque.h:1651 (test.so+0xcc727f)
    #6 operator() openal-soft/al/source.cpp:3700 (test.so+0xcc727f)
    #7 generate<al::ptr_wrapper<unsigned int*>, alSourceUnqueueBuffersDirect(ALCcontext*, ALuint, ALsizei, ALuint*)::<lambda()> > /usr/include/c++/16/bits/stl_algo.h:4375 (test.so+0xcc727f)
    #8 alSourceUnqueueBuffersDirect openal-soft/al/source.cpp:3691 (test.so+0xcc727f)
    #9 alSourceUnqueueBuffers openal-soft/al/source.cpp:3654 (test.so+0xcd6b13) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)

  Mutex M0 (0x726400177358) created at:
    #0 pthread_mutex_lock <null> (libtsan.so.2+0x1f34a) (BuildId: dc0ba0fbb4c898af5813c254dc49d26023dac0d0)
    #1 __gthread_mutex_lock(pthread_mutex_t*) /usr/include/c++/16/x86_64-redhat-linux/bits/gthr-default.h:795 (test.so+0xcc743a) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)
    #2 std::mutex::lock() /usr/include/c++/16/bits/std_mutex.h:116 (test.so+0xcc743a)
    #3 std::lock_guard<std::mutex>::lock_guard(std::mutex&) /usr/include/c++/16/bits/std_mutex.h:276 (test.so+0xcc743a)
    #4 UpdateAllSourceProps(ALCcontext*) openal-soft/al/source.cpp:3753 (test.so+0xcc743a)
    #5 ALCcontext::applyAllUpdates() openal-soft/alc/context.cpp:328 (test.so+0xd07fab) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)
    #6 ALCcontext::processUpdates() openal-soft/alc/context.h:157 (test.so+0xcd8827) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)
    #7 alProcessUpdatesDirectSOFT openal-soft/al/state.cpp:570 (test.so+0xcd8827)
    #8 alProcessUpdatesSOFT openal-soft/al/state.cpp:566 (test.so+0xcdb4fe) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)

  Mutex M1 (0x726800115110) created at:
    #0 pthread_mutex_init <null> (libtsan.so.2+0x1ecf8) (BuildId: dc0ba0fbb4c898af5813c254dc49d26023dac0d0)
    #1 <null> <null> (libspa-support.so+0x7878) (BuildId: eb1ea4612b561359f6f55592e107eff45b8454ad)
    #2 alcOpenDevice openal-soft/alc/alc.cpp:3028 (test.so+0xceedfe) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)

  Thread T162 'ALSoftP0' (tid=587016, running) created by main thread at:
    #0 pthread_create <null> (libtsan.so.2+0x1fd4f) (BuildId: dc0ba0fbb4c898af5813c254dc49d26023dac0d0)
    #1 <null> <null> (libpipewire-0.3.so.0+0x880ae) (BuildId: 3ab1d87839b33b9efa5f5c5ee1f43f8a296fd9e1)
    #2 alcOpenDevice openal-soft/alc/alc.cpp:3028 (test.so+0xceedfe) (BuildId: 78c559b52be8b64dc4386d9bb04dce8a1a72b898)

SUMMARY: ThreadSanitizer: data race openal-soft/al/source.cpp:3700 in operator()****

this is fixed by using the same refcount logic as GetSourceSampleOffset. Not sure if this is just a false positive that should be silenced in another way though.

[kcat]

By "using the same refcount logic as GetSourceSampleOffset", how do you mean? It should be fairly well protected since it synchronizes with the voice's mCurrentBuffer, and won't delete buffer queue items at or beyond that element, but it's possible I'm missing some nuance or edge case that just happens to work. Is it possible to list out a series of events that hit the race condition?

[maxbachmann]

With maxbachmann@9bd44be the race is no longer reported. I don't understand enough about the code to understand whether this is even an actual issue in the first place. Looking at the file this pattern appears to be used for some of the accesses but not all of them.
The race is reported while removing already processed buffers:

        ALint processed;
        alGetSourcei(source, AL_BUFFERS_PROCESSED, &processed);
        alSourceUnqueueBuffers(source, processed, dummy);

[kcat]

That pattern is used to ensure an unbroken non-atomic read of data. It's essentially a seqlock, and is used in GetSourceSampleOffset and elsewhere to ensure reading the source voice's current buffer, read offset, and fractional offset are in a consistent state (even though all three are atomic reads, as a group it's not an atomic read -- the mixer thread can come along and update the values as the calling thread is reading them, resulting in some values being from before the update with some after -- and the seqlock ensures there wasn't an update while reading the values). This shouldn't be an issue in alSourceUnqueueBuffers though, since the worst that could happen is that mCurrentBuffer ends up slightly out of date if the mixer updates while that call is made, which only means the number of processed buffers is temporarily less than what the mixer is at (which would equally be reflected in the AL_BUFFERS_PROCESSED query, since it does the same thing to count the number of processed buffers).

Though perhaps it thinks there's an issue with mCurrentBuffer not being read with acquire semantics, maybe that confuses it to think the mixer thread could still be accessing the older queue entries by the time it gets to deleting them (even though it can't start deleting them until after it was read, where the entries being deleted are verified to not be in use). Your change may just be pacifying the sanitizer with the additional acquire fences around reading mCurrentBuffer, in both the waitForMix and atomic_thread_fence calls. I just wish I could better understand what the issue is that it's trying to indicate, to know what the proper fix is. Simply pointing to where the data race occurred doesn't show how it got there with reference to the state of other threads that are racing with it.

[maxbachmann]

I think tsans reasoning doesn't mean the two actually happened at the same time. It just means that there is no synchronization (happens before relationship) between the two. At least for us this tsan error comes up reliably when using alSourceUnqueueBuffers and building openal with tsan.

[kcat]

Perhaps it would then be enough to change the voice->mCurrentBuffer.load(std::memory_order_relaxed); to voice->mCurrentBuffer.load(std::memory_order_acquire); in alSourceUnqueueBuffers.

I tried building with the thread sanitizer here and I'm getting odd results. I don't see the error you're seeing, but I see another that doesn't make sense (given the acquire and release atomic operations used, there shouldn't be a way for it to potentially race; all previous writes from the main thread are completed before the mixer thread does its reads and writes). I built it again and I'm seeing more errors that weren't there before, which should also be protected with appropriate atomic operations. It makes me wonder if tsan sees those atomic store/load/exchanges with acquire/release semantics as appropriate synchronization fences, or if it instead only sees mutexes as proper synchronization (the reports do mention mutexes, but they aren't for that). Though if that was the case, I'd expect many more errors to be reported.