Re: [patch] spinlocks: remove 'volatile'

Chase Venters
2006-07-07T22:22:26Z

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed, Size: 2380 bytes --]

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