From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53730) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJFUw-0001eb-2M for qemu-devel@nongnu.org; Wed, 13 Jan 2016 02:05:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJFUs-0008FV-Ow for qemu-devel@nongnu.org; Wed, 13 Jan 2016 02:05:37 -0500 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:57723) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJFUs-0008Db-7i for qemu-devel@nongnu.org; Wed, 13 Jan 2016 02:05:34 -0500 Received: from localhost by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 13 Jan 2016 17:05:29 +1000 Date: Wed, 13 Jan 2016 12:34:29 +0530 From: Bharata B Rao Message-ID: <20160113070429.GG11785@in.ibm.com> References: <1452236119-24452-1-git-send-email-bharata@linux.vnet.ibm.com> <1452236119-24452-12-git-send-email-bharata@linux.vnet.ibm.com> <20160112060634.GW22925@voom.redhat.com> <20160113041054.GE11785@in.ibm.com> <20160113045700.GA22925@voom.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160113045700.GA22925@voom.redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 11/11] spapr: CPU hot unplug support Reply-To: bharata@linux.vnet.ibm.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: mdroth@linux.vnet.ibm.com, aik@ozlabs.ru, agraf@suse.de, qemu-devel@nongnu.org, pbonzini@redhat.com, qemu-ppc@nongnu.org, tyreld@linux.vnet.ibm.com, nfont@linux.vnet.ibm.com, imammedo@redhat.com, afaerber@suse.de, ehabkost@redhat.com On Wed, Jan 13, 2016 at 03:57:00PM +1100, David Gibson wrote: > On Wed, Jan 13, 2016 at 09:40:54AM +0530, Bharata B Rao wrote: > > On Tue, Jan 12, 2016 at 05:06:34PM +1100, David Gibson wrote: > > > On Fri, Jan 08, 2016 at 12:25:19PM +0530, Bharata B Rao wrote: > > > > Remove the CPU core device by removing the underlying CPU thread devices. > > > > Support hot removal of CPU for sPAPR guests by sending the hot unplug > > > > notification to the guest via EPOW interrupt. Release the vCPU object > > > > after CPU hot unplug so that vCPU fd can be parked and reused. > > > > > > > > Signed-off-by: Bharata B Rao > > > > --- > > > > hw/ppc/spapr.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/hw/ppc/spapr.h | 6 +++ > > > > 2 files changed, 119 insertions(+) > > > > > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > > > index c2af9ca..43cb726 100644 > > > > --- a/hw/ppc/spapr.c > > > > +++ b/hw/ppc/spapr.c > > > > @@ -93,6 +93,9 @@ > > > > > > > > #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift)) > > > > > > > > +static QLIST_HEAD(cpu_unplug_list, sPAPRCPUUnplug) cpu_unplug_list > > > > + = QLIST_HEAD_INITIALIZER(cpu_unplug_list); > > > > + > > > > static XICSState *try_create_xics(const char *type, int nr_servers, > > > > int nr_irqs, Error **errp) > > > > { > > > > @@ -2432,11 +2435,121 @@ static void spapr_machine_device_plug(HotplugHandler *hotplug_dev, > > > > } > > > > } > > > > > > > > +static void spapr_cpu_destroy(PowerPCCPU *cpu) > > > > +{ > > > > + sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine()); > > > > + > > > > + xics_cpu_destroy(spapr->icp, cpu); > > > > + qemu_unregister_reset(spapr_cpu_reset, cpu); > > > > +} > > > > + > > > > +static void spapr_cpu_core_cleanup(void) > > > > +{ > > > > + sPAPRCPUUnplug *unplug, *next; > > > > + Object *cpu; > > > > + > > > > + QLIST_FOREACH_SAFE(unplug, &cpu_unplug_list, node, next) { > > > > + cpu = unplug->cpu; > > > > + object_unparent(cpu); > > > > + QLIST_REMOVE(unplug, node); > > > > + g_free(unplug); > > > > + } > > > > +} > > > > + > > > > +static void spapr_add_cpu_to_unplug_list(Object *cpu) > > > > +{ > > > > + sPAPRCPUUnplug *unplug = g_malloc(sizeof(*unplug)); > > > > + > > > > + unplug->cpu = cpu; > > > > + QLIST_INSERT_HEAD(&cpu_unplug_list, unplug, node); > > > > +} > > > > + > > > > +static int spapr_cpu_release(Object *obj, void *opaque) > > > > +{ > > > > + DeviceState *dev = DEVICE(obj); > > > > + CPUState *cs = CPU(dev); > > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > > + > > > > + spapr_cpu_destroy(cpu); > > > > + cpu_remove_sync(cs); > > > > + > > > > + /* > > > > + * We are still walking the core object's children list, and > > > > + * hence can't cleanup this CPU thread object just yet. Put > > > > + * it on a list for later removal. > > > > + */ > > > > + spapr_add_cpu_to_unplug_list(obj); > > > > + return 0; > > > > +} > > > > + > > > > +static void spapr_core_release(DeviceState *dev, void *opaque) > > > > +{ > > > > + object_child_foreach(OBJECT(dev), spapr_cpu_release, opaque); > > > > + spapr_cpu_core_cleanup(); > > > > + object_unparent(OBJECT(dev)); > > > > > > I'd prefer to see the unplug_list as a local to this function (passed > > > to spapr_cpu_release via opaque) rather than using a new global. > > > > Will try that in the next iteration. > > > > > > > > > +} > > > > + > > > > +static int spapr_core_detach(Object *obj, void *opaque) > > > > +{ > > > > + sPAPRCoreState *core = opaque; > > > > + DeviceState *dev = DEVICE(obj); > > > > + CPUState *cs = CPU(dev); > > > > + PowerPCCPU *cpu = POWERPC_CPU(cs); > > > > + int id = ppc_get_vcpu_dt_id(cpu); > > > > + int smt = kvmppc_smt_threads(); > > > > + sPAPRDRConnector *drc = > > > > + spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id); > > > > + sPAPRDRConnectorClass *drck; > > > > + Error *local_err = NULL; > > > > + > > > > + /* > > > > + * SMT threads return from here, only main thread (thread 0) will > > > > + * continue and signal hot unplug event to the guest. > > > > + */ > > > > > > This seems silly. spapr_core_unplug() iterates through all the > > > children only to have all of them except thread 0 ignore the call.. > > > > > > Can't you just grab the first thread and do this logic directly from > > > spapr_core_unplug()? > > > > If I purely rely on the children list of the core object like I am doing > > here, I don't see how to grab the first thread object from the list w/o > > doing what I am doing here. However I can store the first thread object > > in the core object during the core object's instance_init and use it here. > > Will give it a try in the next iteration. > > It should be accessible by name as "thread[0]" no? Yes, like this AFAICS: ObjectProperty *prop = object_property_find(OBJECT(core), "thread[0]", &local_err); Object *thread = prop->opaque; Storing the object pointer of the main thread in the core looks simpler to me. Regards, Bharata.