On 04.03.2016 07:54, Bharata B Rao wrote:
> From: Gu Zheng <guz.f...@cn.fujitsu.com>
>
> 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/kvm@vger.kernel.org/msg102839.html
>
> Signed-off-by: Chen Fan <chen.fan.f...@cn.fujitsu.com>
> Signed-off-by: Gu Zheng <guz.f...@cn.fujitsu.com>
> Signed-off-by: Zhu Guihua <zhugh.f...@cn.fujitsu.com>
> Signed-off-by: Bharata B Rao <bhar...@linux.vnet.ibm.com>
> [- Explicit CPU_REMOVE() from qemu_kvm/tcg_destroy_vcpu()
> isn't needed as it is done from cpu_exec_exit()
> - Use iothread mutex instead of global mutex during
> destroy
> - Don't cleanup vCPU object from vCPU thread context
> but leave it to the callers (device_add/device_del)]
> Reviewed-by: David Gibson <da...@gibson.dropbear.id.au>
> ---
> cpus.c | 38 +++++++++++++++++++++++++++++++++++
> include/qom/cpu.h | 10 +++++++++
> include/sysemu/kvm.h | 1 +
> kvm-all.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
> kvm-stub.c | 5 +++++
> 5 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/cpus.c b/cpus.c
> index 9592163..07cc054 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -953,6 +953,18 @@ void async_run_on_cpu(CPUState *cpu, void (*func)(void
> *data), void *data)
> qemu_cpu_kick(cpu);
> }
>
> +static void qemu_kvm_destroy_vcpu(CPUState *cpu)
> +{
> + if (kvm_destroy_vcpu(cpu) < 0) {
> + error_report("kvm_destroy_vcpu failed");
> + exit(EXIT_FAILURE);
> + }
> +}
> +
> +static void qemu_tcg_destroy_vcpu(CPUState *cpu)
> +{
> +}
> +
> static void flush_queued_work(CPUState *cpu)
> {
> struct qemu_work_item *wi;
> @@ -1053,6 +1065,11 @@ 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);
> + qemu_mutex_unlock_iothread();
> + return NULL;
> + }
My comment from last time still applies:
You could increase readability of the code by changing the condition of
the loop instead - currently it is a "while (1)" ... you could turn that
into a "do { ... } while (!cpu->exit || cpu_can_run(cpu))" and then
destroy the cpu after the loop.
> }
>
> return NULL;
> @@ -1108,6 +1125,7 @@ static void tcg_exec_all(void);
> static void *qemu_tcg_cpu_thread_fn(void *arg)
> {
> CPUState *cpu = arg;
> + CPUState *remove_cpu = NULL;
>
> rcu_register_thread();
>
> @@ -1145,6 +1163,16 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
> }
> }
> qemu_tcg_wait_io_event(QTAILQ_FIRST(&cpus));
> + CPU_FOREACH(cpu) {
> + if (cpu->exit && !cpu_can_run(cpu)) {
> + remove_cpu = cpu;
> + break;
> + }
> + }
> + if (remove_cpu) {
> + qemu_tcg_destroy_vcpu(remove_cpu);
> + remove_cpu = NULL;
> + }
> }
>
> return NULL;
> @@ -1301,6 +1329,13 @@ void resume_all_vcpus(void)
> }
> }
>
> +void cpu_remove(CPUState *cpu)
> +{
> + cpu->stop = true;
> + cpu->exit = true;
> + qemu_cpu_kick(cpu);
> +}
> +
> /* For temporary buffers for forming a name */
> #define VCPU_THREAD_NAME_SIZE 16
>
> @@ -1517,6 +1552,9 @@ static void tcg_exec_all(void)
> break;
> }
> } else if (cpu->stop || cpu->stopped) {
> + if (cpu->exit) {
> + next_cpu = CPU_NEXT(cpu);
> + }
> break;
> }
> }
> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
> index 7052eee..6e5171b 100644
> --- a/include/qom/cpu.h
> +++ b/include/qom/cpu.h
> @@ -237,6 +237,7 @@ struct kvm_run;
> * @halted: Nonzero if the CPU is in suspended state.
> * @stop: Indicates a pending stop request.
> * @stopped: Indicates the CPU has been artificially stopped.
> + * @exit: Indicates the CPU has exited due to an unplug operation.
> * @crash_occurred: Indicates the OS reported a crash (panic) for this CPU
> * @tcg_exit_req: Set to force TCG to stop executing linked TBs for this
> * CPU and return to its top level loop.
> @@ -289,6 +290,7 @@ struct CPUState {
> bool created;
> bool stop;
> bool stopped;
> + bool exit;
Another comment from last review:
There is also a "exit_request" member in this struct already ... maybe
you could name the new variable differently to avoid confusion?
Something like "remove_request" or "unplug_request" ?
> bool crash_occurred;
> bool exit_request;
> uint32_t interrupt_request;
...
Apart from that, the patch looks fine to me.
Thomas
