Xen devel - Oct 2010 - Re: [PATCH 1/5] Add a global synchronization point for pvclock

On 04/15/2010 11:37 AM, 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.
Unfortunately this is breaking Xen save/restore: if you restore on a
host which was booted more recently than the save host, causing the
system time to be smaller.  The effect is that the domain''s time leaps
forward to a fixed point, and stays there until the host catches up to
the source host...

I guess last_time needs to be reset on this type of event.  I guess the
cleanest way would be for pvclock.c to register a sysdev suspend/resume
handler.

    J
> Signed-off-by: Glauber Costa <glommer@redhat.com>
> CC: Jeremy Fitzhardinge <jeremy@goop.org>
> CC: Avi Kivity <avi@redhat.com>
> CC: Marcelo Tosatti <mtosatti@redhat.com>
> CC: Zachary Amsden <zamsden@redhat.com>
> ---
>  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;
> +
>  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));
> +
>  	return ret;
>  }
>  

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/26/2010 01:30 AM, Jeremy Fitzhardinge wrote:
> Unfortunately this is breaking Xen save/restore: if you restore on a
> host which was booted more recently than the save host, causing the
> system time to be smaller.  The effect is that the domain''s time
leaps
> forward to a fixed point, and stays there until the host catches up to
> the source host...
Shouldn''t save/restore also save the timebase?
> I guess last_time needs to be reset on this type of event.  I guess the
> cleanest way would be for pvclock.c to register a sysdev suspend/resume
> handler.
Should be for Xen only; kvm save/restore doesn''t involve the guest.

-- 
error compiling committee.c: too many arguments to function


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, 2010-10-26 at 10:14 +0200, Avi Kivity wrote:
> On 10/26/2010 01:30 AM, Jeremy Fitzhardinge wrote:
> > Unfortunately this is breaking Xen save/restore: if you restore on a
> > host which was booted more recently than the save host, causing the
> > system time to be smaller.  The effect is that the domain''s
time leaps
> > forward to a fixed point, and stays there until the host catches up to
> > the source host...
> 
> Shouldn''t save/restore also save the timebase?
> 
> > I guess last_time needs to be reset on this type of event.  I guess
the
> > cleanest way would be for pvclock.c to register a sysdev
suspend/resume
> > handler.
> 
> Should be for Xen only; kvm save/restore doesn''t involve the
guest.
the suspend/resume path do adjust the time base. Maybe something similar
should be done ?



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 10/26/2010 01:14 AM, Avi Kivity wrote:
>  On 10/26/2010 01:30 AM, Jeremy Fitzhardinge wrote:
>> Unfortunately this is breaking Xen save/restore: if you restore on a
>> host which was booted more recently than the save host, causing the
>> system time to be smaller.  The effect is that the domain''s
time leaps
>> forward to a fixed point, and stays there until the host catches up to
>> the source host...
>
> Shouldn''t save/restore also save the timebase?
Xen doesn''t guarantee the system time is monotonic across those kinds
of
events.  The domain could maintain its own offset to maintain an
illusion of monotonicity, but I think its simpler to just zero
last_value on resume.

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel