C++ proper atomic memory ordering on a "thread barrier" synchronization pattern

Question

I need to properly synchronize access to some shared resource between a predefined number of worker threads (statically known via application config) and a predefined number of control-plane threads. The control-plane threads receive requests from the outside, and based on that potentially modify the shared resource. Worker threads simply run an infinite loop inside of which the shared resource is read only. To do this in a thread-safe way, and given the actual application use-case (network packet processing, multi data-plane threads and multi control-plane threads), it was decided to implement a "thread barrier" kind of pattern. Here's a snippet for how it's done, assuming the application is configured to spawn 2 worker threads and 2 control-plane threads:

std::atomic_bool barrier{};
std::atomic_uint32_t workers_at_barrier{};

// called by control-plane threads only!
void barrier_lock()
{
    // optimized spinlock implementation: rigtorp.se/spinlock/
    while (true)
    {
        if (!barrier.exchange(true, std::memory_order_acquire))
            break;

        while (barrier.load(std::memory_order_relaxed))
            __builtin_ia32_pause();
    }
    assert(barrier);

    // wait for ALL worker (data-plane) threads to arrive at the barrier!
    while (workers_at_barrier.load() != 2);
    assert(workers_at_barrier.load() == 2);
}

// called by control-plane threads only!
void barrier_unlock()
{
    assert(barrier && workers_at_barrier.load() == 2);
    barrier.store(false, std::memory_order_release);

    // wait for ALL workers to get out of the barrier!
    while (workers_at_barrier.load() != 0);
}

struct barrier_lock_guard
{
    barrier_lock_guard()
    {
        barrier_lock();
    }

    ~barrier_lock_guard()
    {
        barrier_unlock();
    }
};

// control-plane threads receive some requests and handles them here
void handle_stuff()
{
    // ... stuff

    {
        barrier_lock_guard blg;

        // barrier should be set and all workers (2 in this case) should be waiting at the barrier for its release
        assert(barrier && workers_at_barrier.load() == 2);

        // ... writes to shared resource
    }

    // ... stuff
}

// called by worker threads only!
void wait_at_barrier()
{
    // immediately return if barrier is not set
    if (!barrier.load(std::memory_order_acquire))
        return;
    
    ++workers_at_barrier;

    // block at the barrier until it gets released
    while (barrier.load(std::memory_order_acquire));

    --workers_at_barrier;
}

// function run by the worker threads
void workers_stuff()
{
    while (true)
    {
        wait_at_barrier();

        // ... reads from shared resource
    }
}

The problem is that the assert assert(barrier && workers_at_barrier.load() == 2); in handle_stuff() is getting hit. This occurs very very rarely, so there must be something wrong, and I'm trying to understand exactly what and where. Pretty sure though it has something to do with an incorrect use of std::memory_order. Any C++ atomics pro out there that can point me to the exact issue and what the proper fix would be? Thanks in advance.

user_634919 · Answer

This is not a memory ordering issue, just a plain race. I can reproduce it even after upgrading all the memory orderings to sequential consistency. Here is my version on godbolt though I can only reproduce the failure locally (godbolt only runs on one core).

The comment wait for ALL workers to get out of the barrier! in barrier_unlock seems to point to the problem. This loop doesn't force another control thread to wait; that other thread could take the barrier right away.

Alternatively, observing the value workers_at_barrier == 2 in barrier_lock() does not prove that both threads are now waiting at the barrier; they may have already passed it while it was previously down, but not yet gotten around to decrementing the atomic counter.

So imagine the following sequence of events. We have control threads C1,C2, and worker threads W1,W2. C1 has taken the barrier and is just entering barrier_unlock(). C2 is just entering barrier_lock(). W1 and W2 are both spinning in the while(barrier.load()) in wait_at_barrier(), and workers_at_barrier has the value 2.

C1: barrier.store(false)
W1: barrier.load(): false, spin loop exits
C2: barrier.exchange(true): returns false. Break out of loop. Now barrier == true.
C2: assert(barrier) (passes)
C2: workers_at_barrier.load(): 2. The while loop exits immediately.
C2: assert(workers_at_barrier.load() == 2) (passes)
C2 returns from barrier_lock()
W1: --workers_at_barrier: 1
C2 in handle_stuff(): Now barrier == true and workers_at_barrier == 1. The assertion fails.

I'm not sure of the best fix offhand. Perhaps barrier should have a third "draining" state, in which the control thread still owns the barrier but the workers can leave it. Only after they have done so does the control thread fully release the barrier.

C++ proper atomic memory ordering on a "thread barrier" synchronization pattern

1 Answers