Re: [Qemu-stable] [Qemu-ppc] [PATCH] target/ppc: only save guest timebase once after stopping

[David Gibson]

On Fri, May 04, 2018 at 03:50:28PM +0200, Greg Kurz wrote:
> On Fri, 04 May 2018 07:18:13 -0500
> Michael Roth <address@hidden> wrote:
> 
> > Quoting Greg Kurz (2018-05-04 04:37:24)
> > > On Thu,  3 May 2018 23:20:44 -0500
> > > Michael Roth <address@hidden> wrote:
> > >   
> > > > In some cases (e.g. spapr) we record guest timebase after qmp_stop()
> > > > via a runstate hook so we can restore it on qmp_cont(). If a migration
> > > > occurs in between those events we end up saving it again, this time
> > > > based on the current timebase the guest would be seeing had it been
> > > > running. This has the effect of advancing the guest timebase while
> > > > it is stopped, which is not what the code intends.
> > > >   
> > > 
> > > Hi Mike,
> > > 
> > > The current behavior was introduced by:
> > > 
> > > commit 42043e4f1241eeb77f87f5816b5cf0b6e9583ed7
> > > Author: Laurent Vivier <address@hidden>
> > > Date:   Fri Jan 27 13:24:58 2017 +0100
> > > 
> > >     spapr: clock should count only if vm is running
> > > 
> > > and we have this in the changelog:
> > > 
> > >     We keep timebase_pre_save to reduce the clock difference on
> > >     migration like in:
> > >         6053a86 kvmclock: reduce kvmclock difference on migration
> > > 
> > > 
> > > So your patch totally negates ^^ ? Also, I can't see a case where  
> > 
> > Yah... this is a bit confusing. On one hand, the patch/summary is clearly
> > trying to avoid the guest time from advancing while it is stopped, which
> > is in the spirit of this patch. But at the same time it is trying to
> > compensate for loss of time (relative to host) due to downtime window.
> > 
> 
> Yeah... not sure why Laurent decided to address both in the same patch...
> maybe just because we already had the pre_save hook ?
> 
> > I think the subtlety is in the amount of time... saving at pre_save
> > rather than vm_stop() compensates for the normal downtime window, which
> > is *usually* small (5s is the figure they quote in the notes there and
> > in the motivating 6053a86 "kvmclock: reduce kvmclock difference on
> > migration"). The delays between vm_stop and vm_cont via something like
> > virsh suspend/resume is unbounded, unhowever, hence the rationale for
> > the runstate hook (?).
> > 
> 
> That's my understanding as well.
> 
> > So maybe small jumps are considered okay, and large ones not? If that's
> > the reasoning, then this patch is addressing the later, so it's not
> > necessarily in conflict with that motivation, but the implementation
> > does negate the small jumps we try to avoid via pre_save hook since
> > we'll end up keep the version we saved just after vm_stop instead.
> > 
> > I would note that the downtime window itself, while usually small, can
> > also be quite large. With 1GB hugepages we've seen some guests requiring
> > downtime windows to be set to 25s until QEMU would start cut-over. Also
> > rcu_cpu_stall_timeout is configurable...it's possible if we set it to
> > 5s it could trigger on the jump the guest experiences from pre_save (I
> > haven't tested that though).
> > 
> > Maybe trying to compensate for downtime is a generally bad idea and we
> > should just leave it up to NTP/etc? 
> 
> My understanding of NTP is that it isn't designed to cope with sudden
> time differences, which is exactly what happens in our case.

I think so too.  I mean it will correct things right eventually,
unless the jump is *really* huge, but it's not a good enough solution
that we shouldn't at least try to keep the guest's wall-time clock
correct across the migration.

> > Or maybe we should choose a specific
> > upper bound on how much migration downtime we're willing to compensate
> > for and enforce that directly? E.g. tb->saved becomes tb->saved_time and
> > we check the difference in pre_save before calling timebase_save()
> > again.
> > 
> 
> This would maybe allow to reach a compromise between the current code
> and your patch... but it would still be difficult to come up with
> a sensible value for this upper bound, wouldn't it ?
> 
> > > So your patch totally negates ^^ ? Also, I can't see a case where
> > > timebase_save() could be called from vmstate_save_state() while the
> > > VM is running, ie, you could drop timebase_pre_save()... or am I
> > > *probably* missing something ?  
> > 
> > Yah, I didn't notice that my patch completely negated the pre_save
> > hook... for some reason I was thinking that would continue to function
> > normally if we didn't call qmp_stop() explicitly but that's clearly not
> > the case. So yah, dropping timebase_pre_save() is essentially what my
> > patch is doing...
> > 
> 
> <thinking aloud>
> How does Linux cope with standard software suspend or hibernate ? It also
> causes a downtime and it doesn't generate RCU stalls AFAIK... would it
> be possible/make sense for migration to look like an hibernate ?
> </thinking aloud>

In that case the guest is aware of the stoppage, and knows to a)
correct its internal clock afterwards (usually by looking at the RTC),
and b) not see the sudden clock jump as an error.

By design, guests are not aware of a migration, so they can't do that.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature