Le 18/05/2018 à 01:00, Segher Boessenkool a écrit :
On Fri, May 18, 2018 at 08:30:27AM +1000, Benjamin Herrenschmidt wrote:
On Thu, 2018-05-17 at 14:23 -0500, Segher Boessenkool wrote:
On Thu, May 17, 2018 at 01:06:10PM +1000, Benjamin Herrenschmidt wrote:
The current asm statement in __patch_instruction() for the cache flushes
doesn't have a "volatile" statement and no memory clobber. That means
gcc can potentially move it around (or move the store done by put_user
past the flush).
volatile is completely superfluous here, except maybe as documentation:
any asm without outputs is always volatile.
I wasn't aware of that. I was drilled early on to always stick volatile
in my asm statements if they have any form of side effect :-)
If an asm without output was not marked automatically as having another
side effect, every such asm would be immediately deleted ;-)
Adding volatile as documentation for side effects can be good; it just
doesn't do much (nothing, in fact) for asms without output as far as
the compiler is concerned.
(And the memory clobber does not prevent the compiler from moving the
asm around, or duplicating it, etc., and neither does the volatile).
It prevents load/stores from moving around doesn't it ? I wanted to
make sure the store of the instruction doesn't move in/pass the asm. If
you say that's not needed then ignore the patch.
No, it's fine here, and you want either that or put exactly the memory
you are touching in a constraint (probably overkill here). I just
wanted to say that a "memory" clobber does nothing more than say the
asm touches some unspecified memory; there is no magic other meaning
to it. Your patch is correct, just the "volatile" part isn't needed,
and the explanation was a bit cargo-culty ;-)
Any plan to get that merged ?
Christophe
