From patchwork Tue May 2 07:43:51 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Henrique Barboza X-Patchwork-Id: 9707547 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2712C60245 for ; Tue, 2 May 2017 07:55:05 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1766423E64 for ; Tue, 2 May 2017 07:55:05 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0B02B25D9E; Tue, 2 May 2017 07:55:05 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6643C23E64 for ; Tue, 2 May 2017 07:55:04 +0000 (UTC) Received: from localhost ([::1]:57256 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d5SeF-0007r6-Fb for patchwork-qemu-devel@patchwork.kernel.org; Tue, 02 May 2017 03:55:03 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58951) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1d5SdM-0007p7-N5 for qemu-devel@nongnu.org; Tue, 02 May 2017 03:54:10 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1d5SdJ-00016A-Il for qemu-devel@nongnu.org; Tue, 02 May 2017 03:54:08 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:47278 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1d5SdJ-00015l-B6 for qemu-devel@nongnu.org; Tue, 02 May 2017 03:54:05 -0400 Received: from pps.filterd (m0098420.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v427TJww128416 for ; Tue, 2 May 2017 03:54:04 -0400 Received: from e24smtp05.br.ibm.com (e24smtp05.br.ibm.com [32.104.18.26]) by mx0b-001b2d01.pphosted.com with ESMTP id 2a6n89kgxk-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Tue, 02 May 2017 03:54:04 -0400 Received: from localhost by e24smtp05.br.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 2 May 2017 04:43:57 -0300 Received: from d24relay04.br.ibm.com (9.18.232.146) by e24smtp05.br.ibm.com (10.172.0.141) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 2 May 2017 04:43:56 -0300 Received: from d24av01.br.ibm.com (d24av01.br.ibm.com [9.8.31.91]) by d24relay04.br.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v427htfq3735948; Tue, 2 May 2017 04:43:56 -0300 Received: from d24av01.br.ibm.com (localhost [127.0.0.1]) by d24av01.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id v427ht16026635; Tue, 2 May 2017 04:43:55 -0300 Received: from [9.85.205.45] ([9.85.205.45]) by d24av01.br.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id v427hqi4026632; Tue, 2 May 2017 04:43:53 -0300 To: Bharata B Rao References: <20170430172547.13415-1-danielhb@linux.vnet.ibm.com> <20170430172547.13415-3-danielhb@linux.vnet.ibm.com> From: Daniel Henrique Barboza 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 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-TM-AS-MML: disable x-cbid: 17050207-0032-0000-0000-0000055A10B2 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17050207-0033-0000-0000-000011DF2CAF Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2017-05-02_04:, , 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-1703280000 definitions=main-1705020045 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] [fuzzy] X-Received-From: 148.163.158.5 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH 2/5] hw/ppc: removing spapr_drc_detach_cb opaques 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: "qemu-ppc@nongnu.org" , "qemu-devel@nongnu.org" , David Gibson Errors-To: qemu-devel-bounces+patchwork-qemu-devel=patchwork.kernel.org@nongnu.org Sender: "Qemu-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 05/02/2017 12:40 AM, Bharata B Rao wrote: > On Sun, Apr 30, 2017 at 10:55 PM, Daniel Henrique Barboza > > wrote: > > Following up the previous detach_cb change, this patch removes the > detach_cb_opaque entirely from the code. > > The reason is that the drc->detach_cb_opaque object can't be > restored in the post load of the upcoming DRC migration and no detach > callbacks actually need this opaque. 'spapr_core_release' is > receiving it as NULL, 'spapr_phb_remove_pci_device_cb' is receiving > a phb object as opaque but is't using it. These were trivial removal > cases. > > However, the LM removal callback 'spapr_lmb_release' is receiving > and using the opaque object, a 'sPAPRDIMMState' struct. This struct > holds the number of LMBs the DIMM object contains and the callback > was using this counter as a countdown to check if all LMB DRCs were > release before proceeding to the DIMM unplug. To remove the need of > this callback we have choices such as: > > - migrate the 'sPAPRDIMMState' struct. This would require creating a > QTAILQ to store all DIMMStates and an additional 'dimm_id' field to > associate the DIMMState with the DIMM object. We could attach this > QTAILQ to the 'sPAPRPHBState' and retrieve it later in the callback. > > - fetch the state of the LMB DRCs directly by scanning the state of > them and, if all of them are released, proceed with the DIMM unplug. > > The second approach was chosen. The new 'spapr_all_lmbs_drcs_released' > function scans all LMBs of a given DIMM device to see if their DRC > state are inactive. If all of them are inactive return 'true', 'false' > otherwise. This function is being called inside the > 'spapr_lmb_release' > callback, replacing the role of the 'sPAPRDIMMState' opaque. The > 'sPAPRDIMMState' struct was removed from the code given that there are > no more uses for it. > > After all these changes, there are no roles left for the > 'detach_cb_opaque' > attribute of the 'sPAPRDRConnector' as well, so we can safely remove > it from the code too. > > Signed-off-by: Daniel Henrique Barboza > > > --- > hw/ppc/spapr.c | 46 > +++++++++++++++++++++++++++++++++------------- > hw/ppc/spapr_drc.c | 16 +++++----------- > hw/ppc/spapr_pci.c | 4 ++-- > include/hw/ppc/spapr_drc.h | 6 ++---- > 4 files changed, 42 insertions(+), 30 deletions(-) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index bc11757..8b9a6cf 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1887,21 +1887,43 @@ static void spapr_drc_reset(void *opaque) > } > } > > -typedef struct sPAPRDIMMState { > - uint32_t nr_lmbs; > -} sPAPRDIMMState; > +static bool spapr_all_lmbs_drcs_released(PCDIMMDevice *dimm) > +{ > + Error *local_err = NULL; > + PCDIMMDeviceClass *ddc = PC_DIMM_GET_CLASS(dimm); > + MemoryRegion *mr = ddc->get_memory_region(dimm); > + uint64_t size = memory_region_size(mr); > + > + uint64_t addr; > + addr = object_property_get_int(OBJECT(dimm), > PC_DIMM_ADDR_PROP, &local_err); > + if (local_err) { > + error_propagate(&error_abort, local_err); > + return false; > + } > + uint32_t nr_lmbs = size / SPAPR_MEMORY_BLOCK_SIZE; > > -static void spapr_lmb_release(DeviceState *dev, void *opaque) > + sPAPRDRConnector *drc; > + int i = 0; > + for (i = 0; i < nr_lmbs; i++) { > + drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_LMB, > + addr / SPAPR_MEMORY_BLOCK_SIZE); > + g_assert(drc); > + if (drc->indicator_state != > SPAPR_DR_INDICATOR_STATE_INACTIVE) { > + return false; > + } > + addr += SPAPR_MEMORY_BLOCK_SIZE; > + } > + return true; > +} > + > +static void spapr_lmb_release(DeviceState *dev) > { > - sPAPRDIMMState *ds = (sPAPRDIMMState *)opaque; > HotplugHandler *hotplug_ctrl; > > - if (--ds->nr_lmbs) { > + if (!spapr_all_lmbs_drcs_released(PC_DIMM(dev))) { > return; > } > > > I am concerned about the number of times we walk the DRC list > corresponding to each DIMM device. When a DIMM device is being > removed, spapr_lmb_release() will be invoked for each of the LMBs of > that DIMM. Now in this scheme, we end up walking through all the DRC > objects of the DIMM from every LMB's release function. 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? 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 > > Regards, > Bharata. 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);