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

[Avi Kivity]

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/