Re: [Qemu-devel] [QEMU PATCH v5 1/6] migration: alternative way to set instance_id in SaveStateEntry

[David Gibson]

On Tue, Oct 11, 2016 at 11:17:35AM -0500, Michael Roth wrote:
> Quoting David Gibson (2016-10-10 00:31:20)
> > On Fri, Oct 07, 2016 at 09:07:49AM +0100, Dr. David Alan Gilbert wrote:
> > > * David Gibson (address@hidden) wrote:
> > > > On Wed, Oct 05, 2016 at 09:44:57AM -0700, Jianjun Duan wrote:
> > > > > Please see comments below:
> > > > > 
> > > > > On 10/05/2016 03:12 AM, Dr. David Alan Gilbert wrote:
> > > > > > * Jianjun Duan (address@hidden) wrote:
> > > > > >> In QOM(QEMU Object Model) migrated objects are identified with 
> > > > > >> instance_id
> > > > > >> which is calculated automatically using their path in the QOM 
> > > > > >> composition
> > > > > >> tree. For some objects, this path could change from source to 
> > > > > >> target in
> > > > > >> migration. To migrate such objects, we need to make sure the 
> > > > > >> instance_id does
> > > > > >> not change from source to target. We add a hook in DeviceClass to 
> > > > > >> do customized
> > > > > >> instance_id calculation in such cases.
> > > > > > 
> > > > > > Can you explain a bit about why the path changes from source to 
> > > > > > destination;
> > > > > > the path here should be a feature of the guest state not the host, 
> > > > > > and so I
> > > > > > don't understand why it changes.
> > > > > Please see the discussion with David in the previous versions:
> > > > > http://lists.nongnu.org/archive/html/qemu-ppc/2016-06/msg00062.html
> > > > 
> > > > Um.. your description above really isn't an accurate summary of that
> > > > discussion.
> > > > 
> > > > The point is not that the qom path will vary from source to
> > > > destination for some arbitrary reason, but rather that we anticipate
> > > > future changes in the QOM structure.  Specifically we're considering
> > > > eliminating the DRC objects, and folding their (limited) state into an
> > > > array in the parent object (either the machine or a PCI host bridge).
> > > > 
> > > > That would change the qom paths, and hence the auto-generated instance
> > > > ids, which would break migration between qemu versions before and
> > > > after the restructure.
> > > > 
> > > > I'm not sure that changing the instance ids is enough though, anyway,
> > > > since we're talking about eliminating the object entirely, the
> > > > class/type information in the migration stream also wouldn't match.
> > > > 
> > > > Dave, if you have ideas on how to deal with that, I'd love to hear
> > > > them
> > > 
> > > Eliminating the object entirely would be tricky to deal with;
> > > allowing the structure to change would work if instead of a custom 
> > > instance_id
> > > you had a custom idstr.
> > 
> > Sorry, two misunderstandings here.
> > 
> >   * When I said "structure" I meant in the high-level sense of what
> >     objects exist and are responsible for what things, not in the
> >     'struct WhateverState' sense.
> > 
> >   * In fact right now eliminating the objects would be ok, since they
> >     have no migration state (which causes the problems this series is
> >     trying to address).  But applying this series which adds migration
> >     state would make it difficult to eliminate the objects in future.
> >     That's an awkward constraint given that we've already got some
> >     hints that these objects are not a good idea.
> > 
> > > However, the question then becomes why is the structure changing so much;
> > > ideally things in the migration stream should represent some tangible 
> > > object
> > > that corresponds to something real, but (again ideally) the contents
> > > of the stream should reflect the state of those objects not the current
> > > implementation - so if you want to change the implementation the stream
> > > doesn't change.  Is it your implementation, or the understanding of what
> > > the objects actually represent that's in flux?
> > 
> > So, the objects in question are DRCs - "Dynamic Re-configuration
> > Connector"; silly IBM talk for "a port into which something can be
> > hotplugged", bascially.  These aren't "real" devices, but rather a
> > firmware/hypervisor abstraction which are used to describe a hotplug
> > point.  Each DRC allows either one CPU core, one PCI device, or one
> > LMB (256MiB chunk of RAM) to be hotplugged (or removed).  The PCI DRCs
> > are "owned" by the PCI host bridge to which the device would be
> > connected, the CPU and memory DRCs are owned by the machine.
> > 
> > The state variables which Jianjun Duan is adding to migration are
> > values defined in the PAPR (hypervisor interface) spec, and so are
> > tangible enough to be sensible to migrate.  At the moment, each LMB is
> > represented by a discrete QOM object, but I've been thinking for a
> > while that this may be a mistake.  In particular it's a problem for
> > the LMB DRCs - because each LMB is only 256MiB of memory, we end up
> > with thousands, maybe tens of thousands of DRC objects on a guest with
> > with large maxmem (even if initial memory is small).  The QOM
> > infrastructure doesn't deal terribly well that many object child
> > properties.
> > 
> > The construction of those DRCs used to be an N^3 algorithm, which as
> > you'd imagine caused horrible startup times with large maxmem (minutes
> > to hours above ~512GiB).  We fixed it to be only N^2, so now the
> > startup delays are merely bad (at least once you get to ~2TiB+
> > maxmem).
> > 
> > We could fix that by changing QOM to use a hash or tree for object
> > children, instead of a plain list, but I'm not sure if it makes sense
> > to do that with only this use case.
> 
> Is that still as much an issue as of this patch?
> 
> commit b604a854e843505007c59d68112c654556102a20
> Author: Pavel Fedin <address@hidden>
> Date:   Tue Oct 13 13:37:45 2015 +0100
> 
>     qom: Replace object property list with GHashTable
>     
>     ARM GICv3 systems with large number of CPUs create lots of IRQ pins. Since
>     every pin is represented as a property, number of these properties becomes
>     very large. Every property add first makes sure there's no duplicates.
>     Traversing the list becomes very slow, therefore QEMU initialization takes
>     significant time (several seconds for e. g. 16 CPUs).
>     
>     This patch replaces list with GHashTable, making lookup very fast. The 
> only
>     drawback is that object_child_foreach() and 
> object_child_foreach_recursive()
>     cannot add or remove properties during traversal, since GHashTableIter 
> does
>     not have modify-safe version. However, the code seems not to modify 
> objects
>     via these functions.

Huh.. good point.  I'd completely forgotten that change had already
happened.

> > So, for some time, I've been considering rewriting the DRC stuff to be
> > implemented via QOM interface on the "owner" object, rather than
> > separate objects for every single connector.
> 
> At a high-level I think I'd prefer improving QOM over working around it's
> performance issues. It seems like those limits are bound to be pushed
> eventually given the wide range of hardware it's meant to support
> in theory.

Yeah, given we already have that hash, it probably makes sense to keep
the DRCs as "real" objects.

> But identifying a DRC state in the migration stream with a stable value based
> on DRC index makes sense either way. If I understand this patch it seems
> like we have that: if a custom instance_id getter is available, we avoid
> using the QOM path for idstr and use the default of vmsd->name, then use
> the instance_id from the getter. If we dropped DRCs as objects, we'd need
> a mechanism other than DeviceClass->dev_get_instance_id() to provide this
> custom instance_id, but otherwise so long as the state was the same, and
> the VMSD was the same, it seems like it should be doable in theory.

So, as noted in other comments, I'm not sure we actually need a new
mechanism for overriding the instance id.  But regardless of the
mechanism once we have a stable id based on the DRC index, yes it
should be ok to migrate the state variables of each DRC this way.

> 
> > 
> > So the difficulty is that if we add the state migration to the DRC
> > objects as they stand, we prevent ourselves from doing a refactor
> > that's probably a good idea.  But no-one's yet had time to actually
> > thrash out such a refactor. :/
> > 
> 

-- 
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