Do I need to use atomic<> if already guard by mutex

Question

Given the code in this post, to implement Semaphore using only atomic<> and mutex.

I'm just curious that since count is already guarded by updateMutex, is atomic<> necessary?

struct Semaphore {
    int size;
    atomic<int> count;
    mutex updateMutex;

    Semaphore(int n) : size(n) { count.store(0); }

    void aquire() {
        while (1) {
            while (count >= size) {}
            updateMutex.lock();
            if (count >= size) {
                updateMutex.unlock();
                continue;
            }
            ++count;
            updateMutex.unlock();
            break;
        }
    }

    void release() {
        updateMutex.lock();
        if (count > 0) {
            --count;
        } // else log err
        updateMutex.unlock();
    }
};

Without atomic, I think the constructor would get synchronization problem. Assignment to count might not be visible if other threads are using it right after the construction.

If so, what about size? Does it also need to be protected by atomic<>?

Or the atomic<> is totally useless because both size and count will be visible no matter when other threads use them.

Thanks!

Answer 1

There are multiple questions asked. All require that the underlying concept is understood: you have a data race if one object is written by at least one thread which is accessed (read or written) by another thread and the write and the access are not synchronized. The formal definition of data races is in 1.10 [intro.multithread] paragraph 21:

The execution of a program contains a data race if it contains two conflicting actions in different threads, at least one of which is not atomic, and neither happens before the other. [...]

A program which contains a data race has undefined behavior, i.e., the program needs to make sure that it is data race free. Now on to answering the different questions:

1. Is it necessary to use synchronization in the constructor?

   It depends on whether the object may be access concurrently by different threads while it is still under construction. The only case I can imagine concurrent access to an object under construction is during static initialization where multiple thread are already kicked off accessing the shared object. Due to the weak constraints on the order of construction for global objects I can't imagine that global objects would be used anyway and construction of function local static objects is synchronized by the implementation. Otherwise, I would expect that a reference to the object would shared across threads using a suitably synchronized mechanism. That is, I would design the system such that the constructor doesn't require synchronization.

2. There is a lock already. Does that mean that count doesn't have to be an atomic.

   Since count is accessed in the acquire() function before lock is obtained, it would be an unsynchronized access to an object which is written by another thread, i.e., you'd have a data race and, hence, undefined behavior. The count has to be atomic.

3. Is it necessary for size to be synchronized, too.

   The size member is only modified in the constructor of Semaphore and it might be reasonable to enforce that by actually making it a const member. Assuming the object isn't concurrently accessed during construction (see 1. above) there is no potential for a data race when accessing size.

Note that you shouldn't really make unguarded use of the lock() and unlock() members of the mutex. Instead, you should use std::lock_guard<std::mutex> or std::unique_lock<std::mutex>, potentially with an auxiliary block. These two classes guarantee that an acquired lock will always be released. I'd also question if a busy wait for a semaphore acquiring a lock anyway is the right way to go.