Implementing Mutex Lock in C

Question

I'm trying to make a really simple spinlock mutex in C, and for some reason I get cases where two threads simultaneously get a lock, which should not be possible. It runs on a multiprocessor system, and therefore a problem may occur. Any ideas why it doesn't work?

void mutexLock(mutex_t *mutexlock, pid_t owner) { int failure = 1; while(mutexlock->mx_state == 0 || failure || mutexlock->mx_owner != owner) { failure = 1; if (mutexlock->mx_state == 0) { asm( "movl $0x01,%%eax\n\t" // move 1 to eax "xchg %%eax,%0\n\t" // try to set the lock bit "mov %%eax,%1\n\t" // export our result to a test var :"=r"(mutexlock->mx_state),"=r"(failure) :"r"(mutexlock->mx_state) :"%eax" ); } if (failure == 0) { mutexlock->mx_owner = owner; //test to see if we got the lock bit } } } 

Answer 1

Well, for starters, you test an uninitialized variable ( failure ) the first time you execute the while() condition.

Your actual problem is that you tell gcc to use the register for mx_state - which will obviously not work for spinlock. Try:

  asm volatile ( "movl $0x01,%%eax\n\t" // move 1 to eax "xchg %%eax,%0\n\t" // try to set the lock bit "mov %%eax,%1\n\t" // export our result to a test var :"=m"(mutexlock->mx_state),"=r"(failure) :"m"(mutexlock->mx_state) :"%eax" ); 

Note that asm volatile is also important here to ensure that it will not be pulled out of the while loop.

Answer 2

The problem is that you load mx_state into the register (the restriction is “r”) and then exchange with the registers only by writing the result to mx_state at the end of the asm code. What you want is more like

 asm( "movl $0x01,%%eax\n\t" // move 1 to eax "xchg %%eax,%1\n\t" // try to set the lock bit "mov %%eax,%0\n\t" // export our result to a test var :"=r"(failure) :"m" (mutexlock->mx_state) :"%eax" ); 

Even this is somewhat dangerous, since theoretically the compiler can load mx_state, spill it into the slot of the local temporary package and make xchg there. It is also somewhat inefficient, as it has hard gestures that may not be needed, but cannot be eliminated by the optimizer. You better use a simpler asm that expands to a single command, e.g.

 failure = 1; asm("xchg %0,0(%1)" : "=r" (failure) : "r" (&mutex->mx_state), "0" (failure)); 

Notice how we force the use of mx_state in place using its address rather than its value.