Re: [Qemu-devel] [RFC PATCH v3 17/24] cpus: Reclaim vCPU objects

[David Gibson]

On Wed, May 06, 2015 at 12:07:57PM +0530, Bharata B Rao wrote:
> On Tue, May 05, 2015 at 05:20:04PM +1000, David Gibson wrote:
> > On Fri, Apr 24, 2015 at 12:17:39PM +0530, Bharata B Rao wrote:
> > > From: Gu Zheng <address@hidden>
> > > 
> > > In order to deal well with the kvm vcpus (which can not be removed 
> > > without any
> > > protection), we do not close KVM vcpu fd, just record and mark it as 
> > > stopped
> > > into a list, so that we can reuse it for the appending cpu hot-add 
> > > request if
> > > possible. It is also the approach that kvm guys suggested:
> > > https://www.mail-archive.com/address@hidden/msg102839.html
> > > 
> > > This patch also adds a QOM API object_has_no_children(Object *obj)
> > > that checks whether a given object has any child objects. This API
> > > is needed to release CPU core and socket objects when a vCPU is destroyed.
> > 
> > I'm guessing this commit message needs updating, since you seem to
> > have split this out into the previous patch.
> > 
> > > Signed-off-by: Chen Fan <address@hidden>
> > > Signed-off-by: Gu Zheng <address@hidden>
> > > Signed-off-by: Zhu Guihua <address@hidden>
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > >                [Added core and socket removal bits]
> > > ---
> > >  cpus.c               | 67 
> > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/qom/cpu.h    | 11 +++++++++
> > >  include/sysemu/kvm.h |  1 +
> > >  kvm-all.c            | 57 +++++++++++++++++++++++++++++++++++++++++++-
> > >  kvm-stub.c           |  5 ++++
> > >  5 files changed, 140 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 0fac143..325f8a6 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -858,6 +858,47 @@ void async_run_on_cpu(CPUState *cpu, void 
> > > (*func)(void *data), void *data)
> > >      qemu_cpu_kick(cpu);
> > >  }
> > >  
> > > +static void qemu_destroy_cpu_core(Object *core)
> > > +{
> > > +    Object *socket = core->parent;
> > > +
> > > +    object_unparent(core);
> > > +    if (socket && object_has_no_children(socket)) {
> > > +        object_unparent(socket);
> > > +    }
> > 
> > This seems a bit odd to me.  I thought the general idea of the new
> > approaches to cpu hotplug meant that the hotplug sequence started from
> > the top (the socket or core) and worked down to the threads.  Rather
> > than starting at the thread, and working up to the core and socket
> > level.
> 
> Yes that's true for hotplug as well as hot unplug curently. Plug or
> unplug starts at socket, moves down to cores and threads.
> 
> However when the unplug request comes down to the thread, we have to
> destroy the vCPU and that's when we end up in this part of the code. Here
> the thread (vCPU) unparents itself from the core. The core can't unparent
> untill all its threads have unparented themselves. When all threads of a
> core are done unparenting, core goes ahead and unparents itself from
> its parent socket. Similarly socket can unparent when all cores under
> it have unparented themselves from the socket.

Why can't the core unplug routine propagte the unplug down to the
threads, let that complete, then do the per-core unplug stuff and
remove itself?

Is there an asynchronous callback in here somewhere?

> This is the code that ensures that the socket device object finally
> gets cleared and the id associated with the hot removed socket device
> is available for reuse with next hotplug.
> 
> > 
> > > +}
> > > +
> > > +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
> > > +{
> > > +    Object *thread = OBJECT(cpu);
> > > +    Object *core = thread->parent;
> > > +
> > > +    CPU_REMOVE(cpu);
> > > +
> > > +    if (kvm_destroy_vcpu(cpu) < 0) {
> > > +        error_report("kvm_destroy_vcpu failed.\n");
> > > +        exit(EXIT_FAILURE);
> > > +    }
> > > +
> > > +    object_unparent(thread);
> > > +    if (core && object_has_no_children(core)) {
> > > +        qemu_destroy_cpu_core(core);
> > > +    }
> > > +}
> > > +
> 
> Regards,
> Bharata.
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpc_A1Q86CPD.pgp
Description: PGP signature