| Message ID | 1452236119-24452-7-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <bharata@linux.vnet.ibm.com> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > --- > 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_mutex_unlock_iothread(); > return NULL; > } > @@ -1171,6 +1173,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) > } > if (remove_cpu) { > qemu_tcg_destroy_vcpu(remove_cpu); > + cpu->created = false; > + qemu_cond_signal(&qemu_cpu_cond); > remove_cpu = NULL; > } > } > @@ -1336,6 +1340,14 @@ void cpu_remove(CPUState *cpu) > qemu_cpu_kick(cpu); > } > > +void cpu_remove_sync(CPUState *cpu) > +{ > + cpu_remove(cpu); > + while (cpu->created) { > + qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); > + } > +} > + > /* For temporary buffers for forming a name */ > #define VCPU_THREAD_NAME_SIZE 16 > > diff --git a/include/qom/cpu.h b/include/qom/cpu.h > index 67e05b0..7fc6696 100644 > --- a/include/qom/cpu.h > +++ b/include/qom/cpu.h > @@ -705,6 +705,14 @@ void cpu_resume(CPUState *cpu); > */ > void cpu_remove(CPUState *cpu); > > + /** > + * cpu_remove_sync: > + * @cpu: The CPU to remove. > + * > + * Requests the CPU to be removed and waits till it is removed. > + */ > +void cpu_remove_sync(CPUState *cpu); > + > /** > * qemu_init_vcpu: > * @cpu: The vCPU to initialize.
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 <bharata@linux.vnet.ibm.com> > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > 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 ? Regards, Bharata.
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 <bharata@linux.vnet.ibm.com> > > > Reviewed-by: David Gibson <david@gibson.dropbear.id.au> > > > --- > > > 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()).
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); qemu_mutex_unlock_iothread(); return NULL; } @@ -1171,6 +1173,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg) } if (remove_cpu) { qemu_tcg_destroy_vcpu(remove_cpu); + cpu->created = false; + qemu_cond_signal(&qemu_cpu_cond); remove_cpu = NULL; } } @@ -1336,6 +1340,14 @@ void cpu_remove(CPUState *cpu) qemu_cpu_kick(cpu); } +void cpu_remove_sync(CPUState *cpu) +{ + cpu_remove(cpu); + while (cpu->created) { + qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex); + } +} + /* For temporary buffers for forming a name */ #define VCPU_THREAD_NAME_SIZE 16 diff --git a/include/qom/cpu.h b/include/qom/cpu.h index 67e05b0..7fc6696 100644 --- a/include/qom/cpu.h +++ b/include/qom/cpu.h @@ -705,6 +705,14 @@ void cpu_resume(CPUState *cpu); */ void cpu_remove(CPUState *cpu); + /** + * cpu_remove_sync: + * @cpu: The CPU to remove. + * + * Requests the CPU to be removed and waits till it is removed. + */ +void cpu_remove_sync(CPUState *cpu); + /** * qemu_init_vcpu: * @cpu: The vCPU to initialize.