Re: [patch] spinlocks: remove 'volatile'

From: Chase Venters
Date: Fri Jul 07 2006 - 18:23:28 EST


On Sat, 8 Jul 2006, J.A. Magallón wrote:

#include <stdint.h>

//volatile
uint32_t spinvar = 1;
uint32_t mtx;

void lock(uint32_t* l)
{
*l = 1;
}

void unlock(uint32_t* l)
{
*l = 0;
}

void spin()
{
uint32_t local;

for (;;)
{
lock(&mtx);
local = spinvar;
unlock(&mtx);
if (!local)
break;
}
}

This is _totally_ incorrect. Your "lock" functions are broken, because they do not introduce syncronization points or locked bus operations. Due to this huge failure, the compiler and/or processor is free to re-order your loads and stores, resulting in totally unpredictable runtime behavior.

without the volatile:

spin:
pushl %ebp
movl spinvar, %eax
movl %esp, %ebp
testl %eax, %eax
je .L7
.L10:
jmp .L10
.L7:
movl $0, mtx
popl %ebp
ret

so the compiler did something like

local = spinvar;
if (local)
for (;;);

(notice the dead lock/unlock inlined code elimination).

...which indicates that your code is wrong.

With the volatile, the code is correct:

spin:
pushl %ebp
movl %esp, %ebp
.p2align 4,,7
.L7:
movl spinvar, %eax
testl %eax, %eax
jne .L7
movl $0, mtx
popl %ebp
ret

Actually, it's not. It's never setting "mtx" to 1, and it's certainly not doing any sync or locked ops.

So think about all you inlined spinlocks, mutexes and so on.

Yes, you got it wrong, and the current code gets it right. (Linus's patch of =m to +m, combined with -volatile, is best)

And if you do

void lock(volatile uint32_t* l)
...
void unlock(volatile uint32_t* l)
...

the code is even better:

spin:
pushl %ebp
movl %esp, %ebp
.p2align 4,,7
.L7:
movl $1, mtx <=========
movl spinvar, %eax
movl $0, mtx <=========
testl %eax, %eax
jne .L7
popl %ebp
ret

NO! It's not better. You're still not syncing or locking the bus! If you refer to the fact that the "movl $1" has magically appeared, that's because you've just PAPERED OVER THE PROBLEM WITH "volatile", which is _exactly_ what Linus is telling you NOT TO DO.

So volatile just means 'dont trust this does not change even you don't see
why'.


No.

Thanks,
Chase