Add a global synchronization point for pvclock
in [Kernel]
|
Prev: Sound not transferred to docking station after resume from StR
Next: Process-shared futexes on hugepages puts the kernel in an infinite loop in 2.6.32.11; is this fixed now?
From: Glauber Costa on 19 Apr 2010 10:30 On Mon, Apr 19, 2010 at 02:10:54PM +0300, Avi Kivity wrote: > On 04/19/2010 02:05 PM, Peter Zijlstra wrote: > >> > >>>ACCESS_ONCE() is your friend. > >>> > >>I think it's implied with atomic64_read(). > >Yes it would be. I was merely trying to point out that > > > > last = ACCESS_ONCE(last_value); > > > >Is a narrower way of writing: > > > > last = last_value; > > barrier(); > > > >In that it need not clobber all memory locations and makes it instantly > >clear what we want the barrier for. > > Oh yes, just trying to avoid a patch with both atomic64_read() and > ACCESS_ONCE(). you're mixing the private version of the patch you saw with this one. there isn't any atomic reads in here. I'll use a barrier then -- 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: Avi Kivity on 20 Apr 2010 15:00 On 04/20/2010 09:23 PM, Jeremy Fitzhardinge wrote: > On 04/20/2010 02:31 AM, Avi Kivity wrote: > >> btw, do you want this code in pvclock.c, or shall we keep it kvmclock >> specific? >> > I think its a pvclock-level fix. I'd been hoping to avoid having > something like this, but I think its ultimately necessary. > Did you observe drift on Xen, or is this "ultimately" pointing at the future? -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. -- 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: Avi Kivity on 19 Apr 2010 10:40 On 04/19/2010 05:21 PM, Glauber Costa wrote: > >> Oh yes, just trying to avoid a patch with both atomic64_read() and >> ACCESS_ONCE(). >> > you're mixing the private version of the patch you saw with this one. > there isn't any atomic reads in here. I'll use a barrier then > This patch writes last_value atomically, but reads it non-atomically. A barrier is insufficient. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. -- 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: Glauber Costa on 19 Apr 2010 10:40 On Mon, Apr 19, 2010 at 01:19:43PM +0200, Peter Zijlstra wrote: > On Mon, 2010-04-19 at 14:13 +0300, Avi Kivity wrote: > > On 04/19/2010 01:56 PM, Peter Zijlstra wrote: > > > > > > > > >>> Right, do bear in mind that the x86 implementation of atomic64_read() is > > >>> terrifyingly expensive, it is better to not do that read and simply use > > >>> the result of the cmpxchg. > > >>> > > >>> > > >>> > > >> atomic64_read() _is_ cmpxchg64b. Are you thinking of some clever > > >> implementation for smp i386? > > >> > > > > > > No, what I was suggesting was to rewrite that loop no to need the > > > initial read but use the cmpxchg result of the previous iteration. > > > > > > Something like: > > > > > > u64 last = 0; > > > > > > /* more stuff */ > > > > > > do { > > > if (ret< last) > > > return last; > > > last = cmpxchg64(&last_value, last, ret); > > > } while (last != ret); > > > > > > That only has a single cmpxchg8 in there per loop instead of two > > > (avoiding the atomic64_read() one). > > > > > > > Still have two cmpxchgs in the common case. The first iteration will > > fail, fetching last_value, the second will work. > > > > It will be better when we have contention, though, so it's worthwhile. > > Right, another option is to put the initial read outside of the loop, > that way you'll have the best of all cases, a single LOCK'ed op in the > loop, and only a single LOCK'ed op for the fast path on sensible > architectures ;-) > > last = atomic64_read(&last_value); isn't a barrier enough here? -- 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: Avi Kivity on 19 Apr 2010 10:40
On 04/19/2010 05:32 PM, Glauber Costa wrote: > >> Right, another option is to put the initial read outside of the loop, >> that way you'll have the best of all cases, a single LOCK'ed op in the >> loop, and only a single LOCK'ed op for the fast path on sensible >> architectures ;-) >> >> last = atomic64_read(&last_value); >> > isn't a barrier enough here? > > No. On i386, the statement last = last_value; will be split by the compiler into two 32-bit loads. If a write (atomic, using cmpxchg) on another cpu happens between those two loads, then the variable last will have a corrupted value. -- Do not meddle in the internals of kernels, for they are subtle and quick to panic. -- 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/ |