Re: [Qemu-devel] [PATCH v6 06/11] cpu: Add a sync version of cpu_remove()

[David Gibson]

On Tue, Jan 12, 2016 at 12:23:03PM +0530, Bharata B Rao wrote:
> On Tue, Jan 12, 2016 at 03:16:15PM +1100, David Gibson wrote:
> > On Fri, Jan 08, 2016 at 12:25:14PM +0530, Bharata B Rao wrote:
> > > This sync API will be used by the CPU hotplug code to wait for the CPU to
> > > completely get removed before flagging the failure to the device_add
> > > command.
> > > 
> > > Sync version of this call is needed to correctly recover from CPU
> > > realization failures when ->plug() handler fails.
> > > 
> > > Signed-off-by: Bharata B Rao <address@hidden>
> > > Reviewed-by: David Gibson <address@hidden>
> > > ---
> > >  cpus.c            | 12 ++++++++++++
> > >  include/qom/cpu.h |  8 ++++++++
> > >  2 files changed, 20 insertions(+)
> > > 
> > > diff --git a/cpus.c b/cpus.c
> > > index 12374af..c829bd6 100644
> > > --- a/cpus.c
> > > +++ b/cpus.c
> > > @@ -1067,6 +1067,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)
> > >          qemu_kvm_wait_io_event(cpu);
> > >          if (cpu->exit && !cpu_can_run(cpu)) {
> > >              qemu_kvm_destroy_vcpu(cpu);
> > > +            cpu->created = false;
> > > +            qemu_cond_signal(&qemu_cpu_cond);
> > 
> > I know I reviewed this earlier, but I suspect this needs to be
> > qemu_cond_broadcast.  qemu_cpu_cond is global, so there could (at
> > least in theory) be multiple things waiting on it, and we don't know
> > which one is waiting on *this* cpu, rather than another one (or on any
> > other condition handled by this qemu_cpu_cond).
> 
> qemu_cpu_cond condition variable is currently handling the CPU creation
> condition where qemu_cond_signal() is being used. From what I understand,
> only main thread will be waiting on this condition.

> In this patch, I am using the same condition variable to track CPU
> deletion condition too. After this too, I think only main thread will be
> waiting for the deletion to get completed. So using qemu_cond_signal()
> should be ok isn't ?

Ok, if it's just waking the main thread, qemu_cond_signal() should be
fine (in fact, equivalent to qemu_cond_broadcast()).

-- 
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

signature.asc
Description: PGP signature