Re: [Qemu-devel] [RESEND PATCH v3 5/8] pc-dimm: Add pc_dimm_unrealize() for memory hot unplug support.

[Tang Chen]

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.