Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

Avi Kivity
2010-07-13T18:06:21Z

On 07/13/2010 07:34 PM, Linus Torvalds wrote:
>
> I bet it is the same. And I have a suspicion: because the only write
> access to that variable is in an asm that uses the "memory" clobber to
> say it wrote to it (rather than say it writes to it directly), and
> because the variable is marked 'static', gcc decides that nothing ever
> writes to it in that compilation unit, and it can be made read-only.
>
> Look at our definition for "xchg()" in
> arch/x86/include/asm/cmpxchg_64.h. It boils down to
>
> asm volatile("xchgq %0,%1" \
> : "=r" (__x) \
> : "m" (*__xg(ptr)), "0" (__x) \
> : "memory"); \
>
> for the 8-byte case (which is obviously what atomic64_xchg() uses).
> And the _reason_ we do that thing where we use a memory _input_ and
> then a clobber is that older versions of gcc did not accept the thing
> we _want_ to use, namely using "+m" to say that we actually change the
> memory. So the above is "wrong", but has historical reasons - and
> it's apparently never been changed.
>
> However, the "+m" was fixed, and we use it elsewhere, so I think the
> "m" plus memory clobber is now purely historical. Does a patch
> something like the appended fix it? I also suspect we should look at
> some other uses in this area. The atomic64_64.h file uses "=m" and
> "m", which looks like another legacy thing (again, "+m" historically
> wasn't allowed, and then later became the 'correct' way to do things).
>
>

Well, current upstream uses "m":

> case 8: \
> asm volatile(lock "cmpxchgq %1,%2" \
> : "=a"(__ret) \
> : "r"(__new), "m"(*__xg(ptr)), "0"(__old) \
> : "memory"); \
> break; \

and works; I also failed to reproduce with 2.6.32.16. So I expect some
toolchain involvement.

Peter, what gcc are you using?

--
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.