C++11 recursive atomic spinlock

Question

Could you please review my spinlock implementation?

class spinlock {
private:
 static const std::thread::id lock_is_free;
 std::atomic<std::thread::id> lock_owner;
 int lock_count;
public:
 spinlock() : lock_owner(lock_is_free), lock_count(0) {
 }
 void lock() {
 static thread_local std::thread::id this_thread_local = std::this_thread::get_id();
 auto this_thread = this_thread_local;
 for (auto id = lock_is_free; ; id = lock_is_free) {
 if (lock_owner.compare_exchange_strong(id, this_thread, std::memory_order_acquire, std::memory_order_relaxed)) { ++lock_count; break; }
 id = this_thread;
 if (lock_owner.compare_exchange_strong(id, this_thread, std::memory_order_acquire, std::memory_order_relaxed)) { ++lock_count; break; }
 }
 }
 void unlock() {
 assert(lock_owner.load(std::memory_order_relaxed) == std::this_thread::get_id());
 if (lock_count > 0 && --lock_count == 0) {
 lock_owner.store(lock_is_free, std::memory_order_release);
 }
 }
};
const std::thread::id spinlock::lock_is_free;

• ratchet freak: It's not reentrant (under which I understand that spLock.lock();spLock.lock(); spLock.unlock();spLock.unlock(); works as expected)
• Felix Bytow: That is not the definition of reentrant. Locks that behave like in your description are usually called "recursive" locks.
• ratchet freak: @FelixBytow the other reentrant definition would be not changing globals to allow it to be called thread-safely and recursively but that is redundant when talking about thread safety and this function is not recursive.
• user77050: yes you're right @ratchetfreak, @felix-bytow it's not a re-entrant but rather recursive indeed. lck.lock(); lck.lock(); works. i think the only problem in my code is - you can unlock a lock held by other thread but i typically use std::lock_guard with it so unless i got the lock i would not call unlock()
• ratchet freak: @dundeclared but then the lock should only unlock after the second unlock() in your code it will unlock at the first

Answer 1

Use of TLS

I'm not completely sold on your use of TLS. This looks like a premature optimization to me, that said if you have measured and it improves your performance then keep it.

Static member

I think it would be better style to not have this static member but this is highly subjective.

Readability

Your code here:

 for (auto id = lock_is_free; ; id = lock_is_free) {
 if (lock_owner.compare_exchange_strong(id, this_thread, std::memory_order_acquire, std::memory_order_relaxed)) { ++lock_count; break; }
 id = this_thread;
 if (lock_owner.compare_exchange_strong(id, this_thread, std::memory_order_acquire, std::memory_order_relaxed)) { ++lock_count; break; }
 }

is very difficult to read for me. In fact I'm struggling to understand what the intended behaviour is. To me it looks like what you really wanted was:

// If we're being locked recursively, this will always compare false
// (insert your desired memory_order here) because no one can fiddle
// with `lock_owner` while we have the lock. If we're not entering
// recursively then this will always compare true because the thread
// doing the checking can only be in one place at one time. 
if(lock_owner.load() != this_thread){
 // Okay so it's not a recursive call.
 do{
 auto id = lock_is_free;
 }while(!lock_owner.compare_exchange_weak(id, this_thread, 
 std::memory_order_acquire, 
 std::memory_order_relaxed));
}
// Okay in any event, recursive or not we have the lock now.
lock_count++;
return;

Note: I change compare_exchange_strong to compare_exchange_weak this is recommended when you call compare_exchange in a loop as per std::atomic::compare_exchange (cppreference.com).

I would much prefer this implementation of unlock()

void unlock(){
 assert(lock_owner.load() == std::this_thread::get_id());
 assert(lock_count != 0);
 --lock_count;
 if(lock_count == 0){
 lock_owner.store(lock_is_free, std::memory_order_release);
 }
}

Following the style of std::mutex::unlock() stating that calling unlock when the calling thread doesn't own the lock is undefined behaviour we can simply satisfy ourselves with the two asserts to aid during debug-time.

• user77050: thanks Emily. do you think compare_exchange_weak is sufficient, strong is not needed?
• Miklas: I think you mean assert(lock_count != 0); in your 2nd unlock assert. Other than that, nice code review!
• Miklas: @dundeclared If you look at the documentation provided by Emily L., you can read "The weak forms of the functions are allowed to fail spuriously. [...] in a loop, the weak version will yield better performance on some platforms." Since we are in a spinloop, you can use the weak version.
• Emily L.: @miklas nice catch! Fixed it.
• Emily L.: @Kaiserludi I overrode them.