## `lock_upgrade()` is not thread-safe Imagine two threads calling `lock_upgrade()` at the same time. Both threads think there are only readers at that point, so they both set the state to `rip_parallel::upgrade_state`, they both decrement `m_readers`, then increment `m_upgraders`, `m_mutex.readers_count()` will return `0` for both threads, and then they both call `m_mutex.upgrade_lock()`. Now you have two threads thinking they have exclusive access. It doesn't matter that `m_readers` and `m_upgraders` are atomic here! ## Don't use verbs for class and variable names You named your class `upgrade_mutex`. This sounds like an action. It is best to use nouns for class and variable names. In this case, you could use `upgradable_mutex`. ## Mirror C++11 mutex classes wherever possible Make your class look and behave existing mutex classes as much as possible. It's the principle of least surprise for the users of your class. For example, C++11 mutexes are held by a `lock_guard<>`, which again is a noun. Change the class `upgrade_lock` accordingly. Also, `lock_guard<>` locks the mutex by default, and this is what users who already know regular mutexes expect. So make the default `initial_state = rip_parallel::shared_state`. ## Make internal functions `private` or `protected` Your class has functions like `wait_read()` which, I assume, are not meant to be used directly by application code. So they should be hidden. Also, some functions in the`upgrade_mutex` class, like `upgrade()`, are misnamed; they don't really upgrade the lock, they only implement part of the functionality needed for upgrading it. ## Move as much logic as possible to the mutex class The `upgrade_lock` class is there to provide RAII-style locking. The only thing it should do is lock in the constructor, unlock in the destructor, and pass any useful function like upgrading and downgrading to the mutex. ## Move constant initialization of member variables out of the constructors Instead of making the constructor like this: upgrade_mutex(void): m_readers(0), m_upgraders(0) { ... } Move the initialization of `m_readers` and `m_upgraders` to their declaration: private: volatile std::atomic<int> m_readers = 0; volatile std::atomic<int> m_upgraders = 0; The benefit of this becomes clear when you are writing classes with more than one constructor, or if you have many variables to initialize in your class.