From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55452) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bMpuz-0002GM-IT for qemu-devel@nongnu.org; Tue, 12 Jul 2016 01:07:39 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bMpuu-00021V-GN for qemu-devel@nongnu.org; Tue, 12 Jul 2016 01:07:36 -0400 Date: Tue, 12 Jul 2016 15:09:02 +1000 From: David Gibson Message-ID: <20160712050902.GU16355@voom.fritz.box> References: <1467903025-13383-1-git-send-email-bharata@linux.vnet.ibm.com> <1467903025-13383-3-git-send-email-bharata@linux.vnet.ibm.com> <20160708051958.GK14675@voom.fritz.box> <20160708131102.73f7e998@nial.brq.redhat.com> <20160711032237.GJ16355@voom.fritz.box> <20160711095821.0fe2344e@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="+tDoj9+U2XbkXuwv" Content-Disposition: inline In-Reply-To: <20160711095821.0fe2344e@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [RFC PATCH v2 2/5] cpu: Introduce CPUState::stable_cpu_id List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: nikunj@linux.vnet.ibm.com, groug@kaod.org, qemu-devel@nongnu.org, qemu-ppc@nongnu.org, Bharata B Rao , pbonzini@redhat.com --+tDoj9+U2XbkXuwv Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Jul 11, 2016 at 09:58:21AM +0200, Igor Mammedov wrote: > On Mon, 11 Jul 2016 13:22:37 +1000 > David Gibson wrote: >=20 > > On Fri, Jul 08, 2016 at 01:11:02PM +0200, Igor Mammedov wrote: > > > On Fri, 8 Jul 2016 15:19:58 +1000 > > > David Gibson wrote: > > > =20 > > > > On Thu, Jul 07, 2016 at 08:20:22PM +0530, Bharata B Rao wrote: =20 > > > > > Add CPUState::stable_cpu_id and use that as instance_id in > > > > > vmstate_register() call. > > > > >=20 > > > > > Introduce has-stable_cpu_id property that allows target machines = to > > > > > optionally switch to using stable_cpu_id instead of cpu_index. > > > > > This will help allow successful migration in cases where holes are > > > > > introduced in cpu_index range after CPU hot removals. > > > > >=20 > > > > > Suggested-by: Igor Mammedov > > > > > Signed-off-by: Bharata B Rao > > > > > --- > > > > > exec.c | 6 ++++-- > > > > > include/qom/cpu.h | 5 +++++ > > > > > qom/cpu.c | 6 ++++++ > > > > > 3 files changed, 15 insertions(+), 2 deletions(-) > > > > >=20 > > > > > diff --git a/exec.c b/exec.c > > > > > index fb73910..3b36fe5 100644 > > > > > --- a/exec.c > > > > > +++ b/exec.c > > > > > @@ -619,12 +619,14 @@ static void cpu_release_index(CPUState *cpu) > > > > > void cpu_vmstate_register(CPUState *cpu) > > > > > { > > > > > CPUClass *cc =3D CPU_GET_CLASS(cpu); > > > > > + int instance_id =3D cpu->has_stable_cpu_id ? cpu->stable_cpu= _id : > > > > > + cpu->cpu_index; > > > > > =20 > > > > > if (qdev_get_vmsd(DEVICE(cpu)) =3D=3D NULL) { > > > > > - vmstate_register(NULL, cpu->cpu_index, &vmstate_cpu_comm= on, cpu); > > > > > + vmstate_register(NULL, instance_id, &vmstate_cpu_common,= cpu); > > > > > } > > > > > if (cc->vmsd !=3D NULL) { > > > > > - vmstate_register(NULL, cpu->cpu_index, cc->vmsd, cpu); > > > > > + vmstate_register(NULL, instance_id, cc->vmsd, cpu); > > > > > } > > > > > } > > > > > =20 > > > > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > > > > > index 331386f..527c021 100644 > > > > > --- a/include/qom/cpu.h > > > > > +++ b/include/qom/cpu.h > > > > > @@ -273,6 +273,9 @@ struct qemu_work_item { > > > > > * @kvm_fd: vCPU file descriptor for KVM. > > > > > * @work_mutex: Lock to prevent multiple access to queued_work_*. > > > > > * @queued_work_first: First asynchronous work pending. > > > > > + * @stable_cpu_id: Use as instance_id argument in cpu vmstate_re= gister calls > > > > > + * @has_stable_cpu_id: Set to enforce the use of @stable_cpu_id > > > > > + * over cpu_index during vmstate registration. > > > > > * > > > > > * State of one CPU core or thread. > > > > > */ > > > > > @@ -360,6 +363,8 @@ struct CPUState { > > > > > (absolute value) offset as small as possible. This reduc= es code > > > > > size, especially for hosts without large memory offsets. = */ > > > > > uint32_t tcg_exit_req; > > > > > + int stable_cpu_id; > > > > > + bool has_stable_cpu_id; > > > > > }; > > > > > =20 > > > > > QTAILQ_HEAD(CPUTailQ, CPUState); > > > > > diff --git a/qom/cpu.c b/qom/cpu.c > > > > > index 1095ea1..bae1bf7 100644 > > > > > --- a/qom/cpu.c > > > > > +++ b/qom/cpu.c > > > > > @@ -363,6 +363,11 @@ static int64_t cpu_common_get_arch_id(CPUSta= te *cpu) > > > > > return cpu->cpu_index; > > > > > } > > > > > =20 > > > > > +static Property cpu_common_properties[] =3D { > > > > > + DEFINE_PROP_BOOL("has-stable-cpu-id", CPUState, has_stable_c= pu_id, false), =20 > > > >=20 > > > > It seems odd to me that stable_cpu_id itself isn't exposed as a > > > > property. Even if we don't need to set it externally for now, it > > > > really should be QOM introspectable. =20 > > > Should it? Why? =20 > >=20 > > Well, for one thing it's really strange to have the boolean flag > > exposed, but not the value itself. > property doesn't always means that it's intended as an external interface >=20 > >=20 > > > It's QEMU internal detail and outside world preferably shouldn't > > > know anything about it. =20 > >=20 > > Hrm.. I guess kinda. But I think it's less an internal detail than > > the existing cpu_index is. > so it' better not to start to advertise it as an external interface. Right. My comments were based on the assumption that this was intended as some sort of generally useful stable CPU id, rather than something narrowly focussed on migration. Having understood things better from our IRC discussion, I withdraw this objection. Things also make more sense to me once this is made a class property instead of an instance property, which you've done in your proposed patches. > Should be add some flag to generic property to mark it as internal? If such a thing exists that would be good. --=20 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 --+tDoj9+U2XbkXuwv Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXhHtuAAoJEGw4ysog2bOST8kP/3qr3fe+3FYgYWnu7ZsJj2Xl OSp82wmNw/LgbBZjBt1LTtoHrC/7Hv0uWKc21/XXw+zjI6/OGvSUev/SwJlgwrlv Farrxn0AvxKIEnEic01NhPlq03XdlYAU0sZGUs/DD5pP0IopYtc10yt/N5YDOXfH B3/9N18XfzUuWRa70pnzNJ6IB6kveZ51hJAtrzntAuLMzs7JPZnMz2fEFDsxJbQU CHHLmVZFHSybN1bVbnGoHXeORfl+GLI/fBZURLw6wpUaZownvh2oMsWPisvYXEPk SVSDERtGsWs/p0Mmuz624RgBdbBBOR0VerUuNisVfSHpVQB7P+OQ7c3VNmy+4ilf zF1O2HP2yDcvixAQ8OPG8PguBUipfCXk9OtZzatnGAf08kGpXo7ksNVva3NhFtae zJRkxqZUztbRhMqY8bxLlXKQ9iNnJuJVpuHQGW6HPBOBQHWcIVR7qxLAaZb+kHz9 Ib0XalQwbrpBtnKw76XYNk3/HVY0XC7DeW2qP1VtQYOE7/O6Une3FDNDay5jcrIw lj7vrAS4GB5d5pt2PjbZKqm9KQphtTIwSxRK4DdDV2qFLImdSesP8Rv59/KM2GLf pwRDGuojK8VIpw71tH7gBXZfjn49c8qOFh1XkbHSD+zgtefTJ9Bs9i/e/vAsOUZc XPQvVkaXFBAqTuo3vpoI =rxxz -----END PGP SIGNATURE----- --+tDoj9+U2XbkXuwv--