From patchwork Tue Nov 15 23:45:27 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Michael Roth X-Patchwork-Id: 695344 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [IPv6:2001:4830:134:3::11]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3tJPDb0hmzz9t0p for ; Wed, 16 Nov 2016 10:46:39 +1100 (AEDT) Received: from localhost ([::1]:49672 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6nQy-0000L2-3b for incoming@patchwork.ozlabs.org; Tue, 15 Nov 2016 18:46:36 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60377) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c6nQA-0008IT-4A for qemu-devel@nongnu.org; Tue, 15 Nov 2016 18:45:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c6nQ7-0005kE-Bn for qemu-devel@nongnu.org; Tue, 15 Nov 2016 18:45:46 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:41233) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1c6nQ7-0005k0-3D for qemu-devel@nongnu.org; Tue, 15 Nov 2016 18:45:43 -0500 Received: from pps.filterd (m0098409.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.17/8.16.0.17) with SMTP id uAFNd8Jf110300 for ; Tue, 15 Nov 2016 18:45:41 -0500 Received: from e35.co.us.ibm.com (e35.co.us.ibm.com [32.97.110.153]) by mx0a-001b2d01.pphosted.com with ESMTP id 26r4c53yaf-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 15 Nov 2016 18:45:40 -0500 Received: from localhost by e35.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 15 Nov 2016 16:45:40 -0700 Received: from d03dlp01.boulder.ibm.com (9.17.202.177) by e35.co.us.ibm.com (192.168.1.135) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 15 Nov 2016 16:45:33 -0700 Received: from b03cxnp08027.gho.boulder.ibm.com (b03cxnp08027.gho.boulder.ibm.com [9.17.130.19]) by d03dlp01.boulder.ibm.com (Postfix) with ESMTP id 6C2221FF0025; Tue, 15 Nov 2016 16:45:12 -0700 (MST) Received: from b03ledav006.gho.boulder.ibm.com (b03ledav006.gho.boulder.ibm.com [9.17.130.237]) by b03cxnp08027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id uAFNjWjH39452908; Tue, 15 Nov 2016 16:45:32 -0700 Received: from b03ledav006.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C14A5C6057; Tue, 15 Nov 2016 16:45:32 -0700 (MST) Received: from localhost (unknown [9.41.92.211]) by b03ledav006.gho.boulder.ibm.com (Postfix) with ESMTP id 9D37BC604C; Tue, 15 Nov 2016 16:45:32 -0700 (MST) MIME-Version: 1.0 To: David Gibson , "Jianjun Duan" From: Michael Roth In-Reply-To: <20161007025442.GK18490@umbus.fritz.box> References: <1475519097-27611-1-git-send-email-duanj@linux.vnet.ibm.com> <1475519097-27611-2-git-send-email-duanj@linux.vnet.ibm.com> <20161005101254.GD2036@work-vm> <9dc23ca2-9ca6-9b2e-9c22-257d11e234d1@linux.vnet.ibm.com> <20161007025442.GK18490@umbus.fritz.box> User-Agent: alot/0.3.6 Date: Tue, 15 Nov 2016 17:45:27 -0600 X-TM-AS-GCONF: 00 X-Content-Scanned: Fidelis XPS MAILER x-cbid: 16111523-0012-0000-0000-000011267FC8 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006084; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000189; SDB=6.00781200; UDB=6.00376805; IPR=6.00558710; BA=6.00004883; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00013338; XFM=3.00000011; UTC=2016-11-15 23:45:38 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 16111523-0013-0000-0000-000047345F6C Message-Id: <20161115234527.10998.70353@loki> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-11-15_08:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=0 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1609300000 definitions=main-1611150384 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 148.163.156.1 Subject: Re: [Qemu-devel] [QEMU PATCH v5 1/6] migration: alternative way to set instance_id in SaveStateEntry X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: veroniabahaa@gmail.com, peter.maydell@linaro.org, mst@redhat.com, quintela@redhat.com, mark.cave-ayland@ilande.co.uk, "Dr. David Alan Gilbert" , qemu-devel@nongnu.org, blauwirbel@gmail.com, amit.shah@redhat.com, qemu-ppc@nongnu.org, kraxel@redhat.com, kwolf@redhat.com, dmitry@daynix.com, pbonzini@redhat.com, mreitz@redhat.com, leon.alrae@imgtec.com, aurelien@aurel32.net, rth@twiddle.net Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: "Qemu-devel" Quoting David Gibson (2016-10-06 21:54:42) > 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 (duanj@linux.vnet.ibm.com) 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 > > > > > >> As a result, in these cases compat will not be set in the concerned > > >> SaveStateEntry. This will prevent the inconsistent idstr to be sent over in > > >> migration. We could have set alias_id in a similar way. But that will be > > >> overloading the purpose of alias_id. > > >> > > >> The first application will be setting instance_id for DRC using its unique > > >> index. Doing this makes the instance_id of DRC to be consistent across migration > > >> and supports flexible management of DRC objects in migration. > > > > > > Is there a reason to use a custom instance_id rather than a custom idstr > > > > It can be done either way. But it is easier to deal with a integer than > > a string. > > A bit, but I don't think that's a good enough reason to introduce a > second mechanism for overriding instance id allocations. I'm looking at picking up this patch and the next for DRC migration since it turns out it's needed to fix memory unplug after migration. I think the gist of it is that vmstate_register() doesn't provide a way to specify a custom idstr, and instead relies on vmsd->name (or qom-path for Devices), only register_savevm_live() does. Both however provide a way of specifying a custom instance_id. The only real issue is that in the case of Devices we end up ignoring the instance_id. So I propose using the existing interface to set the instance_id, and relegating all knowledge of DeviceClass->dev_get_instance_id() to qdev (and other callers as-needed). Maybe something like this? > > -- > 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 diff --git a/hw/core/qdev.c b/hw/core/qdev.c index d835e62..7323152 100644 --- a/hw/core/qdev.c +++ b/hw/core/qdev.c @@ -933,7 +933,11 @@ static void device_set_realized(Object *obj, bool value, Error **errp) } if (qdev_get_vmsd(dev)) { - vmstate_register_with_alias_id(dev, -1, qdev_get_vmsd(dev), dev, + int instance_id = DEVICE_GET_CLASS(dev)->dev_get_instance_id + ? DEVICE_GET_CLASS(dev)->dev_get_instance_id(dev) + : -1; + + vmstate_register_with_alias_id(dev, instance_id, qdev_get_vmsd(dev), dev, dev->instance_id_alias, dev->alias_required_for_version); } diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h index 2c97347..a012e8e 100644 --- a/include/hw/qdev-core.h +++ b/include/hw/qdev-core.h @@ -139,6 +139,12 @@ typedef struct DeviceClass { qdev_initfn init; /* TODO remove, once users are converted to realize */ qdev_event exit; /* TODO remove, once users are converted to unrealize */ const char *bus_type; + + /* When this field is set, qemu will use it to get an unique instance_id + * instead of calculating an auto idstr and instanc_id for the relevant + * SaveStateEntry + */ + int (*dev_get_instance_id)(DeviceState *dev); } DeviceClass; typedef struct NamedGPIOList NamedGPIOList; diff --git a/migration/savevm.c b/migration/savevm.c index 0363372..a95fff9 100644 --- a/migration/savevm.c +++ b/migration/savevm.c @@ -556,7 +556,7 @@ int register_savevm_live(DeviceState *dev, se->is_ram = 1; } - if (dev) { + if (dev && instance_id == -1) { char *id = qdev_get_dev_path(dev); if (id) { pstrcpy(se->idstr, sizeof(se->idstr), id); @@ -640,7 +640,7 @@ int vmstate_register_with_alias_id(DeviceState *dev, int instance_id, se->vmsd = vmsd; se->alias_id = alias_id; - if (dev) { + if (dev && instance_id == -1) { char *id = qdev_get_dev_path(dev); if (id) { pstrcpy(se->idstr, sizeof(se->idstr), id);