From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWgbf-0003tv-6F for qemu-devel@nongnu.org; Wed, 24 Sep 2014 03:03:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWgba-0000zX-Rc for qemu-devel@nongnu.org; Wed, 24 Sep 2014 03:03:19 -0400 Received: from [59.151.112.132] (port=63759 helo=heian.cn.fujitsu.com) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWgba-0000wc-E5 for qemu-devel@nongnu.org; Wed, 24 Sep 2014 03:03:14 -0400 Message-ID: <54226C99.90508@cn.fujitsu.com> Date: Wed, 24 Sep 2014 15:02:49 +0800 From: Tang Chen MIME-Version: 1.0 References: <1409126919-22233-1-git-send-email-tangchen@cn.fujitsu.com> <1409126919-22233-6-git-send-email-tangchen@cn.fujitsu.com> <20140904152824.6ab1f113@nial.usersys.redhat.com> <541284FC.8050201@huawei.com> <20140912151757.17e128ed@nial.usersys.redhat.com> In-Reply-To: <20140912151757.17e128ed@nial.usersys.redhat.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov , zhanghailiang Cc: zhugh.fnst@cn.fujitsu.com, mst@redhat.com, hutao@cn.fujitsu.com, qemu-devel@nongnu.org, isimatu.yasuaki@jp.fujitsu.com, pbonzini@redhat.com Hi Igor, Zhang, On 09/12/2014 09:17 PM, Igor Mammedov wrote: > ...... > Actually, this patch also fix the bug *when hotplug memory failing in > the place where after pc_dimm_plug but before the end of device_set_realized, > it does not clear the work done by pc_dimm_plug*. > > For there is no callback like pc_dimm_plug_fail_cb for us to call when fail, > Maybe pc_dimm_unrealize is the only place where we can do the clear work... > Looking at device_set_realized() and pc-dimm case in patrticular > there is no point where it could fail after hotplug_handler_plug() is called. > > But even if there where, one should use pc_dimm_unplug() first to > reverse actions performed by successful pc_dimm_plug(). > > The problem here is that currently unplug callback is actually > doing only unplug request part asking guest to eject memory, > but we also have destroy device when guest tells via ACPI to > ejct memory. You are doing it implicitly by unparenting pc-dimm > from ACPI code and pulling in pc-dimm.unrealize() unrelated > stuff that should be done by PCMachine. > > I'm suggesting that we extend hotplug-handler API to handle > this async/split unplug workflow. By converting current > hotplug_handler_unplug() and related code to > hotplug_handler_unplug_request() that would do the first part > of unplug sequence (see/review http://patchwork.ozlabs.org/patch/387018/) I've read the above patch. 1. I think you proposal would help to resolve the following problem: If guest OS failed to handle hoplug sci, QEmu does not know it, and could release resources incorrectly. Would you please have a look at the following patch. https://www.mail-archive.com/qemu-devel%40nongnu.org/msg253025.html I added a pthread wait condition to synchronize : sci request -> guest OS handling -> _OST -> QEmu handling (Of course, according to the previous discussing, _OST is optional.) And IIUC, your proposal may be : hotplug request -> guest OS handling -> real hotplug handling right ? I think it is the same. How do you think synchronize it with a wait condition ? Or you have any better idea ? Since no one has replied the patch, I'm not sure if it is OK. 2. Let's finish memory and cpu hotplug job based on the current framework, shall we ? In the above patch, it renames a lot of functions that are being used in memory and cpu hotplug patches. I think we can push memory and cpu hotplug jobs, and in the next phase, let's improve the framework. And of course, the problem I mentioned above should also be put in the next phase. So I want to submit the next version memory hotplug patches based on the original framework, and help to improve it in the next phase. How do you think ? > > And then on top of it add hotplug_handler_unplug() that would > handle actual device removal when ACPI asks for it. > > I'm working now on doing above for PCIDevices since they have > the same workflow (expect to submit patches next week) and > it looks like we would need to use the same approach for CPU > unplug as well. > > >