Is there something wrong with my spin lock?

Question

Here is my implementation of a spin lock, but it seems it can not protect the critical code. Is there something wrong with my implementation?

static __inline__ int xchg_asm(int* lock, int val) 
{
  int ret; 
  __asm__ __volatile__(
    LOCK "movl (%1),%%eax; 
    xchg (%1),%2; 
    movl %%eax, %0" :"=m" (ret) :"d"(lock), "c"(val)
  );
  return ret; 
}
void spin_init(spinlock_t* sl) 
{ 
  sl->val = 0; 
} 
void spin_lock(spinlock_t* sl) 
{ 
  int ret; 
  do { 
    ret = xchg_asm(&(sl->val), 1); 
  } while ( ret==0 ); 
}

void spin_unlock(spinlock_t* sl) 
{
  xchg_asm(&(sl->val), 0);
}

Answer 1

Your code equals to:

static __inline__ int xchg_asm(int* lock, int val) {
   int save_old_value_at_eax;

   save_old_value_at_eax = *lock;        /* with a wrong lock prefix */
   xchg *lock with val and discard the original value of *lock.
   return save_old_value_at_eax;           /* but it not the real original value of *lock */
}

You can see from the code, save_old_value_at_eax is no the real original value while the cpu perform xchg. You should get the old/original value by the xchg instruction, not by saving it before perform xchg. ("it is not the real old/original value" means, if another CPU takes the lock after this CPU saves the value but before this CPU performs the xchg instruction, this CPU will get the wrong old value, and it think it took the lock successful, thus, two CPUs enter the C.S. at the same time). You have separated a read-modify-write instruction to three instructions, the whole three instructions are not atomically(even you move the lock prefix to xchg).

I guess you thought the lock prefix will lock the WHOLE three instructions, but actually lock prefix can only be used for the only instruction which it is attached(not all instructions can be attached) And we don't need lock prefix on SMP for xchg. Quote from linux_kernel_src/arch/x86//include/asm/cmpxchg.h

/*
 * Note: no "lock" prefix even on SMP: xchg always implies lock anyway.
 * Since this is generally used to protect other memory information, we
 * use "asm volatile" and "memory" clobbers to prevent gcc from moving
 * information around.
 */

My suggestions:

1. DON'T REPEAT YOURSELF, please use the spin lock of the linux kernel.
2. DON'T REPEAT YOURSELF, please use the xchg(), cmpxchg() of the linux kernel if you do want to implement a spin lock.
3. learn more about instructions. you can also find out how the linux kernel implement it.