Messages in this thread |  | | | Date | Fri, 7 Jul 2006 17:22:22 -0500 (CDT) | | From | Chase Venters <> | | Subject | Re: [patch] spinlocks: remove 'volatile' | |
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 |  |