Re: [Qemu-devel] [PULL 15/18] spapr: CPU hot unplug support

[Bharata B Rao]

On Thu, Jan 26, 2017 at 12:32:58PM +0100, Igor Mammedov wrote:
> On Fri, 17 Jun 2016 16:36:36 +1000
> David Gibson <address@hidden> wrote:
> 
> > From: Bharata B Rao <address@hidden>
> > 
> > Remove the CPU core device by removing the underlying CPU thread devices.
> > Hot removal of CPU for sPAPR guests is achieved by sending the hot unplug
> > notification to the guest. Release the vCPU object after CPU hot unplug so
> > that vCPU fd can be parked and reused.
> > 
> > Signed-off-by: Bharata B Rao <address@hidden>
> > Signed-off-by: David Gibson <address@hidden>
> [...]
> 
> 
> Bharata,
> 
> Here is some notes I've made while auditing spapr cpu hotplug code.
>   
> spapr_core_release() should be spapr_core_unrealize()
> except of machine related
>  spapr->cores[cc->core_id / smt] = NULL;
> which should go to spapr_core_unplug()

There were some issues in calling cpu_remove_[sync] from unrealize
path. I know that x86 does that way. let me remember and get back on this.

> 
> > +static void spapr_core_release(DeviceState *dev, void *opaque)
> > +{
> > +    sPAPRCPUCore *sc = SPAPR_CPU_CORE(OBJECT(dev));
> > +    const char *typename = object_class_get_name(sc->cpu_class);
> > +    size_t size = object_type_get_instance_size(typename);
> > +    sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    CPUCore *cc = CPU_CORE(dev);
> > +    int smt = kvmppc_smt_threads();
> > +    int i;
> > +
> > +    for (i = 0; i < cc->nr_threads; i++) {
> > +        void *obj = sc->threads + i * size;
> > +        DeviceState *dev = DEVICE(obj);
> > +        CPUState *cs = CPU(dev);
> > +        PowerPCCPU *cpu = POWERPC_CPU(cs);
> > +
> > +        spapr_cpu_destroy(cpu);
> > +        cpu_remove_sync(cs);
> > +        object_unparent(obj);
> > +    }
> > +
> > +    spapr->cores[cc->core_id / smt] = NULL;
> > +
> > +    g_free(core->threads);
> > +    object_unparent(OBJECT(dev));
> > +}
> > +
> 
> spapr_core_[un]plug() functions belong to machine code and should
> be in hw/ppc/spapr.c

That's how the series started, but eventually we consolidated all
core related routines in spapr_cpu_core.c

> 
> > +void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> > +                       Error **errp)
> > +{
> > +    sPAPRCPUCore *core = SPAPR_CPU_CORE(OBJECT(dev));
> > +    PowerPCCPU *cpu = POWERPC_CPU(core->threads);
> > +    int id = ppc_get_vcpu_dt_id(cpu);
> > +    sPAPRDRConnector *drc =
> > +        spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, id);
> > +    sPAPRDRConnectorClass *drck;
> > +    Error *local_err = NULL;
> > +
> > +    g_assert(drc);
> > +
> > +    drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > +    drck->detach(drc, dev, spapr_core_release, NULL, &local_err);
> 
> Could you explain call flow during cpu unplug?

In response to unplug request, spapr_core_unplug() gets called which
does a detach() on the associated DRC object. The detach() registers
a callback (spapr_core_release)  and signals the guest about the unplug
request.

When the guest is ready to let go of the CPU core, DRC subsystem ends up
calling the callback spapr_core_release. For each of the CPU thread objects
of the core, spapr_core_release will call cpu_remove_sync() and waits
for the CPU to be really removed. cpu_remove will result in CPU unrealize
function being called (ppc_cpu_unrealizefn) for each of the removed
CPU.

After we are done waiting for all the threads' removal, the core object is
ready for removal.

> 
> My expectations were that unplug_request() handler asks for CPU removal
> and unplug() handler removes CPU.
> It's obviously messed up somehow.

When we did CPU unplug, we didn't really implement ->unplug_request() for
sPAPR. It was added later when memory unplug came in.

Regards,
Bharata.