From: Chase Venters <chase.venters@clientec.com>
To: "J.A. Magallón" <jamagallon@ono.com>
Cc: "linux-os \\(Dick Johnson\\)" <linux-os@analogic.com>,
Linus Torvalds <torvalds@osdl.org>,
linux-kernel@vger.kernel.org
Subject: Re: [patch] spinlocks: remove 'volatile'
Date: Fri, 7 Jul 2006 17:22:22 -0500 (CDT) [thread overview]
Message-ID: <Pine.LNX.4.64.0607071718080.23767@turbotaz.ourhouse> (raw)
In-Reply-To: <20060708000531.410cd672@werewolf.auna.net>
[-- 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
next prev parent reply other threads:[~2006-07-07 22:22 UTC|newest]
Thread overview: 163+ messages / expand[flat|nested] mbox.gz Atom feed top
2006-07-05 8:49 [patch] uninline init_waitqueue_*() functions Ingo Molnar
2006-07-05 9:31 ` Andrew Morton
2006-07-05 9:32 ` Ingo Molnar
2006-07-05 9:53 ` Andrew Morton
2006-07-05 10:26 ` Ingo Molnar
2006-07-05 11:30 ` Ingo Molnar
2006-07-05 11:46 ` Ingo Molnar
2006-07-05 17:10 ` Andrew Morton
2006-07-05 19:35 ` Ingo Molnar
2006-07-05 20:18 ` Andrew Morton
2006-07-05 20:36 ` Linus Torvalds
2006-07-05 20:47 ` Ingo Molnar
2006-07-05 21:15 ` Ingo Molnar
2006-07-05 21:21 ` Linus Torvalds
2006-07-05 21:45 ` Ingo Molnar
2006-07-05 21:58 ` Randy.Dunlap
2006-07-05 22:00 ` Ingo Molnar
2006-07-05 22:10 ` Randy.Dunlap
2006-07-05 22:27 ` Ingo Molnar
2006-07-05 23:15 ` Adrian Bunk
2006-07-05 22:09 ` Linus Torvalds
2006-07-05 22:30 ` Ingo Molnar
2006-07-05 23:11 ` Linus Torvalds
2006-07-06 8:16 ` [patch] spinlocks: remove 'volatile' Ingo Molnar
2006-07-06 8:23 ` Ingo Molnar
2006-07-06 9:27 ` Heiko Carstens
2006-07-06 9:34 ` Arjan van de Ven
2006-07-06 11:59 ` linux-os (Dick Johnson)
2006-07-06 12:01 ` Arjan van de Ven
2006-07-06 12:29 ` linux-os (Dick Johnson)
2006-07-06 12:39 ` Arjan van de Ven
2006-07-06 13:39 ` J.A. Magallón
2006-07-06 13:43 ` Arjan van de Ven
2006-07-06 14:05 ` Chase Venters
2006-07-06 14:26 ` Andreas Schwab
2006-07-06 16:40 ` Nick Piggin
2006-07-06 23:19 ` David Schwartz
2006-07-06 18:15 ` Mark Lord
2006-07-06 19:15 ` Linus Torvalds
2006-07-06 19:33 ` Chris Friesen
2006-07-06 19:37 ` Mark Lord
2006-07-06 20:28 ` Chris Friesen
2006-07-06 20:32 ` Linus Torvalds
2006-07-06 19:38 ` Arjan van de Ven
2006-07-06 19:41 ` Måns Rullgård
2006-07-06 19:42 ` Jeff Garzik
2006-07-06 19:58 ` Linus Torvalds
2006-07-06 20:27 ` Mark Lord
2006-07-06 20:40 ` Chris Friesen
2006-07-06 21:00 ` Linus Torvalds
2006-07-08 8:40 ` Avi Kivity
2006-07-08 8:51 ` Arjan van de Ven
2006-07-08 9:20 ` Avi Kivity
2006-07-08 9:51 ` Arjan van de Ven
2006-07-08 10:18 ` Avi Kivity
2006-07-08 10:28 ` Thomas Gleixner
2006-07-09 4:19 ` Benjamin Herrenschmidt
2006-07-09 12:47 ` Avi Kivity
2006-07-09 19:16 ` David Schwartz
2006-07-09 19:51 ` Theodore Tso
2006-07-09 20:40 ` [OT] 'volatile' in userspace Rutger Nijlunsing
2006-07-10 3:42 ` Theodore Tso
2006-07-10 17:00 ` Joshua Hudson
2006-07-10 17:54 ` Nick Piggin
2006-07-11 7:48 ` Jan Engelhardt
2006-07-11 11:53 ` Nick Piggin
2006-07-11 19:09 ` Björn Steinbrink
2006-07-11 20:55 ` Jeff Dike
2006-07-10 18:54 ` Jeff Dike
2006-07-10 20:09 ` Philippe Troin
2006-07-10 20:52 ` Jeff Dike
2006-07-06 16:07 ` [patch] spinlocks: remove 'volatile' Linus Torvalds
2006-07-06 16:13 ` Linus Torvalds
2006-07-06 17:04 ` Jeff Garzik
2006-07-06 17:52 ` linux-os (Dick Johnson)
2006-07-06 18:00 ` Linus Torvalds
2006-07-06 21:02 ` J.A. Magallón
2006-07-06 21:12 ` Linus Torvalds
2006-07-06 18:10 ` Michael Buesch
2006-07-06 18:16 ` Chase Venters
2006-07-07 18:16 ` Krzysztof Halasa
2006-07-07 19:51 ` linux-os (Dick Johnson)
2006-07-07 20:25 ` Linus Torvalds
2006-07-07 21:22 ` linux-os (Dick Johnson)
2006-07-07 21:48 ` Chase Venters
2006-07-08 10:00 ` Krzysztof Halasa
2006-07-08 13:41 ` Chase Venters
2006-07-08 20:09 ` Krzysztof Halasa
2006-07-08 20:40 ` Chase Venters
2006-07-08 20:47 ` Chase Venters
2006-07-09 10:57 ` Krzysztof Halasa
2006-07-07 21:59 ` Linus Torvalds
2006-07-07 22:06 ` Linus Torvalds
2006-07-08 20:49 ` Pavel Machek
2006-07-08 21:43 ` Linus Torvalds
2006-07-07 22:05 ` J.A. Magallón
2006-07-07 22:22 ` Chase Venters [this message]
2006-07-07 22:37 ` J.A. Magallón
2006-07-08 9:33 ` David Schwartz
2006-07-07 22:49 ` J.A. Magallón
2006-07-07 22:59 ` Vadim Lobanov
2006-07-07 23:18 ` Chase Venters
2006-07-07 23:36 ` Davide Libenzi
2006-07-07 22:09 ` Ingo Molnar
2006-07-08 7:36 ` Ingo Molnar
2006-07-07 20:39 ` Krzysztof Halasa
2006-07-07 23:06 ` Björn Steinbrink
2006-07-08 8:36 ` Avi Kivity
2006-07-06 19:32 ` Jan Engelhardt
2006-07-06 20:26 ` Jeremy Fitzhardinge
2006-07-06 20:55 ` Jan Engelhardt
2006-07-06 21:07 ` Linus Torvalds
2006-07-06 19:56 ` Linus Torvalds
2006-07-06 20:34 ` Linus Torvalds
2006-07-08 22:50 ` Ralf Baechle
2006-07-09 3:09 ` Linus Torvalds
2006-07-09 3:07 ` Keith Owens
2006-07-09 3:29 ` Linus Torvalds
2006-07-09 3:43 ` Keith Owens
2006-07-09 3:58 ` Linus Torvalds
2006-07-09 6:13 ` David Miller
2006-07-09 14:28 ` Roman Zippel
2006-07-09 15:27 ` Linus Torvalds
2006-07-06 8:18 ` [patch] lockdep: clean up completion initializer in smpboot.c Ingo Molnar
2006-07-06 8:23 ` [patch] uninline init_waitqueue_*() functions Ingo Molnar
2006-07-06 9:02 ` Andrew Morton
2006-07-06 9:12 ` [patch] uninline init_waitqueue_head() Ingo Molnar
2006-07-05 20:45 ` [patch] uninline init_waitqueue_*() functions Ingo Molnar
2006-07-05 10:37 ` Christoph Hellwig
2006-07-05 10:44 ` Andrew Morton
2006-07-05 10:46 ` Andrew Morton
2006-07-05 10:47 ` Ingo Molnar
2006-07-05 9:54 ` [patch] uninline init_waitqueue_*() functions, fix Ingo Molnar
2006-07-07 10:21 [patch] spinlocks: remove 'volatile' Chuck Ebbert
2006-07-07 17:09 ` Linus Torvalds
2006-07-08 3:54 Albert Cahalan
2006-07-08 5:25 ` Linus Torvalds
2006-07-08 6:39 ` Nick Piggin
2006-07-08 18:53 ` Linus Torvalds
2006-07-08 9:45 ` Joe Korty
2006-07-08 9:52 ` Arjan van de Ven
2006-07-08 9:59 ` David Schwartz
2006-07-08 10:24 ` Thomas Gleixner
2006-07-08 15:49 ` Albert Cahalan
2006-07-08 18:31 ` Thomas Gleixner
2006-07-08 19:33 ` Albert Cahalan
2006-07-08 20:11 ` Linus Torvalds
2006-07-09 21:10 ` Pavel Machek
2006-07-09 22:18 ` Linus Torvalds
2006-07-10 16:25 ` marty fouts
2006-07-08 23:10 ` David Schwartz
2006-07-08 13:57 ` Andi Kleen
2006-07-08 19:13 ` Linus Torvalds
2006-07-08 10:45 ` Krzysztof Halasa
2006-07-08 6:12 trajce nedev
2006-07-08 6:19 ` Chase Venters
2006-07-08 6:45 ` trajce nedev
2006-07-08 6:58 ` Arjan van de Ven
2006-07-08 7:02 ` Vadim Lobanov
2006-07-08 13:46 ` Chase Venters
2006-07-09 4:39 ` Benjamin Herrenschmidt
2006-07-14 3:30 ` Steven Rostedt
2006-07-08 18:23 ` Linus Torvalds
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.64.0607071718080.23767@turbotaz.ourhouse \
--to=chase.venters@clientec.com \
--cc=jamagallon@ono.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-os@analogic.com \
--cc=torvalds@osdl.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.