Spinlock for C++ kernel (with x86 ASM) This is some prototype code for a spin-lock for my toy operating system.  Bear in mind this means there are no standard library routines to fall back on.  It's also for C++11 compilation and will only ever be compiled by the GNU G++ compiler.

The primary use for this is in the scheduler and other code that accesses the process table. 

I think there are lots of ways of getting this wrong, and some of them aren't obvious. So it'd be great to have a second pair of eyes on it. 

Firstly the locking code itself in NASM x86 asm: 

; *******************************
; void spin_lock(uint32 * p);
;
global __kspin_lock
__kspin_lock:
    push ebp
    mov ebp, esp        ; we might as well the the stack frame right?

    mov eax, [ebp + 8]  ; eax contains address of uint32 used for the lock
    mov ecx, 1

.tryacquire             ; try and get the lock.
    xchg ecx, [eax]
    test ecx, ecx
    jz .acquired        ; if the lock wasn't 0 (unlocked) repeat.

.pauseloop
    pause               ; see 'About PAUSE' below
    test dword [eax], 1
    jne .pauseloop
    jmp .tryacquire

.acquired
    pop ebp
    ret


And secondly a class that wraps it a little.  (I will eventually then wrap this with an RAII class to release the lock automatically when it goes out of scope).

#pragma once
#include &lt;std_types.h&gt;

extern "C" {
/** These routines probably shouldn't be used directly. */
void __kspin_lock(uint32 *p);
};

/** 
 * SpinLock provides a crude locking mechanism based around an atomic
 * exchange of a variable in memory. 
 *
 * The SpinLock loops, swapping the value 1 for the value in lock. 
 * Each time it checks to see if the value retrieved from lock is now 0, 
 * if it is it knows that lock now contains 1, and it holds the lock.
 *
 * To ensure all spinlocks reside in a unique cache line the spinlock
 * is aligned to 64 bytes and is 64 bytes in size.  This avoids two 
 * processes competing for ownership of the same 64 byte cache line,
 * which could be bad for performance.
 */
class __attribute__((aligned(64))) SpinLock 
{
public:
    SpinLock()
        : lock_value(0)
    {}

    void lock()
    {
        __kspin_lock(&amp;lock_value); 
    }

    void release()
    {
        __kspin_lock_release(&amp;lock_value); 
    }


private:
    static void __kspin_lock_release(uint32 *p) 
    { 
        *p = 0; 
    }

    /* Difficult to know whether it makes sense to copy a spinlock. 
     * It might be practical to copy it but always 0 the lock for
     * the copy. */
    SpinLock(const SpinLock &amp;) = delete; 
    SpinLock &amp; operator=(const SpinLock &amp;) = delete; 
    /* padded to ensure it's on a unique cache line. */
    uint32 lock_value;
    char reserved[64 - sizeof(uint32)];
};


I also think this code could use some through tests, especially on performance, but that might be rather a case of profiling in place in the kernel.  Any suggestions welcome.

Incidentally - I'm also thinking that it's going to be useful to add some additional debugging and profiling features to the code.  For example, and probably only while running a debug build: 


How frequently was the lock contended.
Which thread owns the lock.
Is the lock held by this thread (this would allow functions to check that the lock is held by a calling function properly).
This is largely a duplicate of https://stackoverflow.com/questions/6935442/x86-spinlock-using-cmpxchg, https://stackoverflow.com/questions/11959374/fastest-inline-assembly-spinlock, etc.

(1) Your code works only on 32-bit x86. If you're using 64-bit (x86-64), the calling convention is totally wrong (you need to save and restore %rbp rather than %ebp, and you're looking for the argument in the wrong place). If you're implementing your own OS, I suppose you know what you're doing... but in general I think it's weird to see 32-bit x86 code these days.

(2) Useless trivia of the day: The two instructions test ecx, ecx; jz .acquired could be replaced with the single instruction jecxz .acquired. I don't know whether this is an optimization or a pessimization on your particular hardware. It would be a size optimization, at least.

(2.5) Really, consider using cmpxchg instead of xchg; test. The cmpxchg mnemonic maps more directly onto the "Compare And Swap" idiom that you're using. It will also help you when it comes time to implement "is the lock held by this thread"; you can set the word to current-thread-id instead of just 1.

(3) See 'About PAUSE' below: Unfortunately there is no "About PAUSE" below. However, it seems like you're doing the right thing there. You could definitely tighten up the code; for example I don't know why you're wasting time with that test dword [eax], 1. You should just retry the xchg as soon as the pause is over.

(4) Consider removing the stack frame; it's not doing anything useful (unless it's necessary for your debugger of choice), just slowing things down.