GCC asm volatile reorder

[angwer]

Hi all,

I am reading xv6(http://pdos.csail.mit.edu/6.828/2011/xv6.html) source code and something confuses me.

It is the implementation of spinlock. The lock release function is implemented as:

Code: Select all

// Release the lock.
void
release(struct spinlock *lk)
{
  if(!holding(lk))
    panic("release");

  lk->pcs[0] = 0;
  lk->cpu = 0;

  // The xchg serializes, so that reads before release are 
  // not reordered after it.  The 1996 PentiumPro manual (Volume 3,
  // 7.2) says reads can be carried out speculatively and in
  // any order, which implies we need to serialize here.
  // But the 2007 Intel 64 Architecture Memory Ordering White
  // Paper says that Intel 64 and IA-32 will not move a load
  // after a store. So lock->locked = 0 would work here.
  // The xchg being asm volatile ensures gcc emits it after
  // the above assignments (and after the critical section).
  xchg(&lk->locked, 0);

  popcli();
}

And the xchg is implemented as:

Code: Select all

static inline uint
xchg(volatile uint *addr, uint newval)
{
  uint result;
  
  // The + in "+m" denotes a read-modify-write operand.
  asm volatile("lock; xchgl %0, %1" :
               "+m" (*addr), "=a" (result) :
               "1" (newval) :
               "cc");
  return result;
}

What confuses me is that the comment of release tells that:

The xchg being asm volatile ensures gcc emits it after the above assignments (and after the critical section).

But from the gcc manual https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html I find:

Note that the compiler can move even volatile asm instructions relative to other code, including across jump instructions.

I think these two conflicts with each other. I prefer the manual so I think maybe this spinlock release function is wrong if gcc reorders the instructions. Is this a reasonable worry or just redundant? If I am right, does this mean I have to add a memory barrier here?

[bluemoon]

Your concern is reasonable, and without seeing actual code, there is chance that popcli() get moved around as well.


According to http://preshing.com/20120625/memory-ord ... pile-time/

You may want to add a hint to compiler to prevent re-ordering and it seems adds no overhead (except it flush all memory dependency).

Code: Select all

  lk->pcs[0] = 0;
  lk->cpu = 0;
  asm volatile("" ::: "memory");
  xchg(&lk->locked, 0);

[angwer]

Thanks Bluemoon. I will email the maintainer and hope he will give me a reply for conformation.