From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932420Ab0DPUYc (ORCPT ); Fri, 16 Apr 2010 16:24:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10116 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932381Ab0DPUYb (ORCPT ); Fri, 16 Apr 2010 16:24:31 -0400 Date: Fri, 16 Apr 2010 17:23:56 -0300 From: Marcelo Tosatti To: Glauber Costa Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, avi@redhat.com, Jeremy Fitzhardinge , Zachary Amsden Subject: Re: [PATCH 1/5] Add a global synchronization point for pvclock Message-ID: <20100416202356.GA17552@amt.cnet> References: <1271356648-5108-1-git-send-email-glommer@redhat.com> <1271356648-5108-2-git-send-email-glommer@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1271356648-5108-2-git-send-email-glommer@redhat.com> User-Agent: Mutt/1.5.20 (2009-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Apr 15, 2010 at 02:37:24PM -0400, Glauber Costa wrote: > In recent stress tests, it was found that pvclock-based systems > could seriously warp in smp systems. Using ingo's time-warp-test.c, > I could trigger a scenario as bad as 1.5mi warps a minute in some systems. > (to be fair, it wasn't that bad in most of them). Investigating further, I > found out that such warps were caused by the very offset-based calculation > pvclock is based on. > > This happens even on some machines that report constant_tsc in its tsc flags, > specially on multi-socket ones. > > Two reads of the same kernel timestamp at approx the same time, will likely > have tsc timestamped in different occasions too. This means the delta we > calculate is unpredictable at best, and can probably be smaller in a cpu > that is legitimately reading clock in a forward ocasion. > > Some adjustments on the host could make this window less likely to happen, > but still, it pretty much poses as an intrinsic problem of the mechanism. > > A while ago, I though about using a shared variable anyway, to hold clock > last state, but gave up due to the high contention locking was likely > to introduce, possibly rendering the thing useless on big machines. I argue, > however, that locking is not necessary. > > We do a read-and-return sequence in pvclock, and between read and return, > the global value can have changed. However, it can only have changed > by means of an addition of a positive value. So if we detected that our > clock timestamp is less than the current global, we know that we need to > return a higher one, even though it is not exactly the one we compared to. > > OTOH, if we detect we're greater than the current time source, we atomically > replace the value with our new readings. This do causes contention on big > boxes (but big here means *BIG*), but it seems like a good trade off, since > it provide us with a time source guaranteed to be stable wrt time warps. > > After this patch is applied, I don't see a single warp in time during 5 days > of execution, in any of the machines I saw them before. > > Signed-off-by: Glauber Costa > CC: Jeremy Fitzhardinge > CC: Avi Kivity > CC: Marcelo Tosatti > CC: Zachary Amsden > --- > arch/x86/kernel/pvclock.c | 23 +++++++++++++++++++++++ > 1 files changed, 23 insertions(+), 0 deletions(-) > > diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c > index 03801f2..b7de0e6 100644 > --- a/arch/x86/kernel/pvclock.c > +++ b/arch/x86/kernel/pvclock.c > @@ -109,11 +109,14 @@ unsigned long pvclock_tsc_khz(struct pvclock_vcpu_time_info *src) > return pv_tsc_khz; > } > > +static u64 last_value = 0; > + __cacheline_aligned_in_smp to avoid other data from sharing the cacheline. > cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src) > { > struct pvclock_shadow_time shadow; > unsigned version; > cycle_t ret, offset; > + u64 last; > > do { > version = pvclock_get_time_values(&shadow, src); > @@ -123,6 +126,26 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src) > barrier(); > } while (version != src->version); > > + /* > + * Assumption here is that last_value, a global accumulator, always goes > + * forward. If we are less than that, we should not be much smaller. > + * We assume there is an error marging we're inside, and then the correction > + * does not sacrifice accuracy. > + * > + * For reads: global may have changed between test and return, > + * but this means someone else updated poked the clock at a later time. > + * We just need to make sure we are not seeing a backwards event. > + * > + * For updates: last_value = ret is not enough, since two vcpus could be > + * updating at the same time, and one of them could be slightly behind, > + * making the assumption that last_value always go forward fail to hold. > + */ > + do { > + last = last_value; > + if (ret < last) > + return last; > + } while (unlikely(cmpxchg64(&last_value, last, ret) != ret)); > + Don't you need to handle wrap-around?