|
From: | Daniel Henrique Barboza |
Subject: | Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/5] hw/ppc: removing spapr_drc_detach_cb opaques |
Date: | Tue, 2 May 2017 04:43:51 -0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
On 05/02/2017 12:40 AM, Bharata B Rao
wrote:
Hi Bharata, I agree, this is definitely a poorer performance than simply decrementing ds->nr_lmbs. The reasons why I went on with it: - hot unplug isn't an operation that happens too often, so it's not terrible to have a delay increase here; - it didn't increased the unplug delay in an human noticeable way, at least in my tests; - apart from migrating the information, there is nothing much we can do in the callback side about it. The callback isn't aware of the current state of the DIMM removal process, so the scanning is required every time. All that said, assuming that the process of DIMM removal will always go through 'spapr_del_lmbs', why do we need this callback? Can't we simply do something like this in spapr_del_lmbs? diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index cd42449..e443fea 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -2734,6 +2734,20 @@ static void spapr_del_lmbs(DeviceState *dev, uint64_t addr_start, uint64_t size, addr += SPAPR_MEMORY_BLOCK_SIZE; } + if (!spapr_all_lmbs_drcs_released(PC_DIMM(dev))) { + // something went wrong in the removal of the LMBs. + // propagate error and return + throw_error_code; + return; + } + + /* + * Now that all the LMBs have been removed by the guest, call the + * pc-dimm unplug handler to cleanup up the pc-dimm device. + */ + hotplug_ctrl = qdev_get_hotplug_handler(dev); + hotplug_handler_unplug(hotplug_ctrl, dev, &error_abort); + drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, addr_start / SPAPR_MEMORY_BLOCK_SIZE); drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc); With this change we run the LMB scanning once at the end of the for loop inside spapr_del_lmbs to make sure everything went fine (something that the current code isn't doing, there are operationsvbeing done afterwards without checking if the LMB removals actually happened). If something went wrong, propagate an error. If not, proceed with the removal of the DIMM device and the remaining spapr_del_lmbs code. spapr_lmb_release can be safely removed from the code after that. What do you think? Daniel
|
[Prev in Thread] | Current Thread | [Next in Thread] |