Recursive spinlock for C using ASM, revision #1

Question

Precursor for this revision here in code-review:

• Recursive spinlock for C using ASM

Improvements have been made thanks to forsvarir.

Now I am trying to optimize this lock so it becomes more practical.

The code in question.

#define gate_Gate volatile int
#define gate_Pass volatile int
extern inline void
gate_Enter
(gate_Gate *gate, gate_Pass *pass)
{
 asm volatile (
 "jmp gate_Enter_check\n"
 "gate_Enter_wait:\n"
 "pause\n"
 "gate_Enter_check:\n"
 "cmp %[lock], %[pass]\n" // Skip if pass >= lock
 "jge gate_Enter_skip\n"
 "mov %[lock], %%eax\n" // Spinlock start
 "cmp %[gate], %%eax\n" // Check if locked without lock cmd
 "je gate_Enter_wait\n" // If locked, wait.
 "lock xchg %%eax, %[gate]\n" // Lock seems to be free, now we attempt to take it.
 "test %%eax, %%eax\n"
 "jnz gate_Enter_wait\n" // Spinlock end
 "gate_Enter_skip:\n"
 "add %[lock], %[pass]\n" // Checkin pass
 : [gate] "=m" (*gate), [pass] "=m" (*pass)
 : [lock] "r" (1)
 : "eax"
 );
}
extern inline int
gate_Leave
(gate_Gate *gate, gate_Pass *pass)
{
 if (*pass <= 0) {
 // Signal underflow for debugging.
 return -1;
 }
 asm volatile (
 // Cleaner solution thanks to forsvarir/stackoverflow
 "add %[checkout], %[pass]\n"
 "jnz gate_Leave_skip\n"
 "mov %[unlock], %[gate]\n"
 "gate_Leave_skip:\n"
 : [gate] "=m" (*gate), [pass] "=m" (*pass)
 : [unlock] "r" (0), [checkout] "r" (-1)
 : "eax"
 );
 return 0;
}
// Example use. Like forsvarir pointed out, we have to be careful and keep pass on the stack.
void exampleUse(int count, gate_Pass pass)
{
 int underflow;
 printf("pass=%d, gate=%d\n", pass, gate);
 gate_Enter(&gate, &pass);
 if (count == 3) {
 // pass = 0; // force deadlock.
 }
 if (count < 5) {
 printf("Count = %d\n", count);
 exampleUse(++count, pass);
 }
 underflow = gate_Leave(&gate, &pass);
 if (!underflow) {
 printf("pass=%d, gate=%d\n", pass, gate);
 } else {
 printf("underflow detected\n");
 }
}
gate_Pass pass = 0;
exampleUse(0, pass);

The question

The intent of this code is a lock that allows recursion and does so efficiently.

• Is there a point for this optimization? Am I right to assume that "lock" operator is very expensive and this approach is justified and right?
• Is my attempt to early? Locking is hard to test. For this reason I strongly believe that spotting an error is a valid candidate for an answer.
• Finally if all is well, there is always something to improve. This is a code review! Feel free to review the code as you see fit.

Answer 1

gate_Leave has a superfluous compiler hint:

 : "eax"

Since gate_Leave returns an int and eax is often (always?) used to return values this is probably costing you extra instructions (although it might not as the function is inline, depending on how clever your compiler is).

Indentation and Labels

Labels are quite important in assembly. I'd consider adding indentation so that they are easier to find, a lot like you would indent the contents of a while loop.

asm volatile (
 "jmp gate_Enter_check\n"
"gate_Enter_wait:\n"
 "pause\n"
"gate_Enter_check:\n"
 "cmp %[lock], %[pass]\n" // Skip if pass >= lock
 "jge gate_Enter_skip\n"
 "mov %[lock], %%eax\n" // Spinlock start
 "cmp %[gate], %%eax\n" // Check if locked without lock cmd
 "je gate_Enter_wait\n" // If locked, wait.
 "lock xchg %%eax, %[gate]\n" // Lock seems to be free, now we attempt to take it.
 "test %%eax, %%eax\n"
 "jnz gate_Enter_wait\n" // Spinlock end
"gate_Enter_skip:\n"
 "add %[lock], %[pass]\n" // Checkin pass
 : [gate] "=m" (*gate), [pass] "=m" (*pass)
 : [lock] "r" (1)
 : "eax"
);

I'd consider minimising the contents of your spin section. Some of the operations only need to be performed once (loading eax, checking pass). I'm not sure you're really getting much benefit from the unlocked gate check, but without running tests I don't know either way:

asm volatile (
 "cmp %[lock], %[pass]\n" // We only need to check this once, if 
 // this changes while we are in the lock,
 // some other thread is messing with it and
 // we are in trouble anyway.
 "jge gate_Held\n" 
 "mov %[lock], %%eax\n" // Only need to load eax once, if it is ever
 // changed, it's to 0 because we've just
 // locked the gate
"gate_Enter_check:\n"
 "lock xchg %%eax, %[gate]\n" // Attempt to take it.
 "test %%eax, %%eax\n"
 "jz gate_Held\n" 
 "pause\n"
 "jmp gate_Enter_check\n"
"gate_Held:\n"
 "add %[lock], %[pass]\n" 
 : [gate] "=m" (*gate), [pass] "=m" (*pass)
 : [lock] "r" (1)
 : "eax"
);