x86, paravirt: Add a global synchronization point for pvclock
in [Kernel]
|
From: Linus Torvalds on 13 Jul 2010 12:50 On Tue, Jul 13, 2010 at 8:57 AM, Avi Kivity <avi(a)redhat.com> wrote: > On 07/13/2010 05:19 PM, Peter Palfrader wrote: >> >>> So it looks like last_value was placed in a read only section. Please >>> post your System.map somewhere. >>> >> >> weasel(a)intrepid:~$ publish System.map >> >> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map >> weasel(a)intrepid:~$ grep -i last_value System.map >> ffffffff81712e80 r last_value >> ffffffff81b05240 b last_value.26163 >> > > "r" = "read only" > > How does it look in 'nm arch/x86/kernel/pvclock.o'? 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). NOTE NOTE NOTE! This is UNTESTED and INCOMPLETE. We should do cmpxchg too, and the 32-bit versions. I'm adding Ingo and Peter to the cc. Linus
From: Linus Torvalds on 13 Jul 2010 14:00 On Tue, Jul 13, 2010 at 10:25 AM, Peter Palfrader <peter(a)palfrader.org> wrote: > > Linus's patch touches __xchg() whereas we're using __cmpxchg() in this > particular case I think. > > At least, applying it to my 2.6.32.16 tree didn't help, last_value was > still read-only. Or I backported it wrong. No, you didn't back-port it wrong. I just didn't fix the right part. I thought the PV code used xchg, not cmpxchg, so I only patched that. But cmpxchg has the exact same issue. Does this fix it? Again: UNTESTED. Linus
From: Avi Kivity on 13 Jul 2010 14:10 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(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 13 Jul 2010 14:10 On Tue, Jul 13, 2010 at 10:50 AM, Linus Torvalds <torvalds(a)linux-foundation.org> wrote: > > No, you didn't back-port it wrong. I just didn't fix the right part. I > thought the PV code used xchg, not cmpxchg, so I only patched that. > But cmpxchg has the exact same issue. > > Does this fix it? Btw, this second patch was a bit more aggressive than the first one, and actually removes the "memory" clobber entirely, and the fake cast of the target type. That shouldn't matter _except_ if people actually use cmpxchg to implement their own locking, since now the compiler could potentially move unrelated memory references around the lock. Of course, if you use cmpxchg to implement your own locking, you're probably doing something wrong anyway (ie you'll get the wrong memory barriers on various architectures), so it should all be fine. But I thought I'd mention it. And I don't really know how much gcc moves memory accesses around a "asm volatile" - the gcc docs are historically very vague ("volatile asms aren't moved around 'significantly'", whatever 'significant' means) And btw, none of the above should be taken to mean that I have tested the patch or found it to be otherwise good. It might be totally broken for other reasons. Caveat emptor. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Linus Torvalds on 13 Jul 2010 14:20
On Tue, Jul 13, 2010 at 11:04 AM, Avi Kivity <avi(a)redhat.com> wrote: > > Well, current upstream uses "m": Yes, I'd expect current 2.6.34 to also show the same problem. > and works; I also failed to reproduce with 2.6.32.16. �So I expect some > toolchain involvement. And yes, I do believe it's likely a newer gcc that triggers it too, otherwise we'd have seen more reports of it. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |