C++11 recursive atomic spinlock

Could you please review my spinlock implementation?

class spinlock {
private:
    static const std::thread::id lock_is_free;
    std::atomic 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;

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.

StackExchange Code Review Q#95590, answer score: 6