[libvirt] How to make dest host abort migration safely (was Re: [PATCH 4/4] kvm: Allow migration with invtsc)

Mon Jan 9 10:04:34 UTC 2017

* Eduardo Habkost (ehabkost at redhat.com) wrote:
> On Fri, Jan 06, 2017 at 08:31:27AM -0200, Marcelo Tosatti wrote:
> > On Thu, Jan 05, 2017 at 10:19:50AM -0200, Eduardo Habkost wrote:
> > > On Thu, Jan 05, 2017 at 08:48:32AM -0200, Marcelo Tosatti wrote:
> > > > On Wed, Jan 04, 2017 at 11:36:31PM -0200, Eduardo Habkost wrote:
> > > > > On Wed, Jan 04, 2017 at 08:26:27PM -0200, Marcelo Tosatti wrote:
> > > > > > On Wed, Jan 04, 2017 at 05:59:17PM -0200, Eduardo Habkost wrote:
> > > > > > > On Wed, Jan 04, 2017 at 11:39:16AM -0200, Eduardo Habkost wrote:
> > > > > > > > On Wed, Jan 04, 2017 at 09:56:56AM -0200, Marcelo Tosatti wrote:
> > > > > > > > > On Tue, Dec 27, 2016 at 05:21:20PM -0200, Eduardo Habkost wrote:
> [...]
> > > > > > Can't this be fixed in QEMU? Just check that destination host supports
> > > > > > TSC scaling before migration (or that KVM_GET_TSC_KHZ return value
> > > > > > matches on source and destination).
> > > > > > 
> > > > > 
> > > > > This solves only one use case: where you want to expose the
> > > > > starting host TSC frequency to the guest. It doesn't cover any
> > > > > scenario where the TSC frequency of the starting host isn't the
> > > > > best one. (See the two scenarios I described above)
> > > > 
> > > > True.
> > > > 
> > > > > You seem to have a specific use case in mind. If you describe it,
> > > > > we could decide if it's worth the extra migration code complexity
> > > > > to deal with invtsc migration without explicit tsc-frequency. By
> > > > > now, I am not convinced yet.
> > > > 
> > > > I don't have any specific use case in mind. I'm just trying to
> > > > avoid moving complexity to libvirt which in my experience is a
> > > > the best thing to do.
> > > 
> > > I think both our proposals make things harder for libvirt in
> > > different ways. I just want to make the complexity explicit for
> > > libvirt, instead of giving them the illusion of safety (making
> > > migration break in ways libvirt can't detect).
> > > 
> > > Anyway, your points are still valid and it would be still useful
> > > to do what you propose. I will give it a try on a new version of
> > > patch 4/4 that can abort migration earlier. But note that I
> > > expect some pushback from other maintainers because of the
> > > complexity of the code. If that happens, be aware that I won't be
> > > able to justify the added complexity because I would still think
> > > it's not worth it.
> 
> I tried to move the destination TSC-scaling check to post_load,
> and the bad news is that post_load is also too late to abort
> migration (destination aborts, but source shows migration as
> completed).
> 
> I'm CCing Juan, Amit and David to see if they have any
> suggestion.

Yes, that's not too surprising; we don't have any 'finished' flag in
the stream; it's just a one directional stream where the source claims
to have finished after it's stuffed the last byte into the socket 
(I think before it's closed it).

> Summarizing the problem for them: on some cases we want to allow
> migration only if the destination host has a matching TSC
> frequency or if TSC scaling is supported by the destination host.
> I don't know what's the best way to implement that check. We
> could either:
> 
> * Send TSC frequency/scaling info from the the source host, and
>   make the source host abort migration. Is there a good mechanism
>   for that?

Normal migration is just a 1-directional stream, so we dont have a way
to get the TSC frequency back to the source host.

> * Make the destination host abort migration. I don't know if
>   there's a safe mechanism for that.

The only safe way to do it is to do it early; preferably before the RAM
so that you know (via buffer depths) that the source can't have got to
the end yet (and that's assuming these are constant values that are
checked, not something guest configurable).
We do have a 'global_state' in migration.c - but it's just for migration
state rather than device state.
I don't think we currently have any hook for sending device state early;
the setup phase hooks on devices are only called for iterative devices
(i.e. RAM/block etc); I think to do what you need we need to either
add a subsection into vmstate_globalstate in migration.c (which we somehow
have to make device/machine independent) or we have to find a way to
have setup sections for non-iterative devices.

One worry; is this going to affect loading of snapshots on the same
host?  If I take a snapshot, and then come back on a nice warm day
is the TSC on the host going to be sufficiently different the snapshot
wont load?

Dave

> 
> 
> 
> While we figure that out, I will submit v2 without patches 3/4
> and 4/4, because patch 2/2 (allowing migration if tsc-freq is
> explicitly set) is simple and useful. After that, we can add:
> 
> * The mechanism to query the host TSC frequency (see below)
> * The mechanism you asked for, to allow migration without
>   explicit TSC frequency if we know the destination host supports
>   TSC scaling or has a matching TSC frequency (more complex, and
>   maybe unnecessary if we implement the previous item)
> 
> 
> > > 
> > > > 
> > > > > Maybe your use case just needs a explicit "invtsc-migration=on"
> > > > > command-line flag without a mechanism to abort migration on
> > > > > mismatch? I can't tell.
> > > > 
> > > > Again, there is no special case.
> > > > 
> > > > > Note that even if we follow your suggestion and implement an
> > > > > alternative version of patch 4/4 to cover your use case, I will
> > > > > strongly recommend libvirt developers to support configuring TSC
> > > > > frequency if they want to support invtsc + migration without
> > > > > surprising/unpredictable restrictions on migratability.
> > > > 
> > > > Well, alright. If you make the TSC frequency of the host
> > > > available to mgmt software as you describe, and write the steps mgmt
> > > > software should take, i'm good.
> > > 
> > > I plan to. The problem is that the mechanism to query the host
> > > frequency may be unavailable in the first version.
> > 
> > Well just export KVM_GET_TSC_KHZ in a QMP command right? Its pretty
> > easy.
> 
> I plan to expose it as part of query-cpu-model-expansion (work in
> progress, right now) when querying the "host" CPU model.
> 
> > 
> > Let me know if you need any help coding or testing.
> 
> Thanks!
> 
> -- 
> Eduardo
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK