| Message ID | 1460389061-19451-1-git-send-email-will.deacon@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Will, On 11/04/2016 16:37, Will Deacon wrote: > Our exit/reboot code is a bit of a mess: > > - Both kvm__reboot and kvm_cpu_exit send SIGKVMEXIT to running vcpus > - When vcpu0 exits, the main thread starts executing destructors > (exitcalls) whilst other vcpus may be running > - The pause_lock isn't always held when inspecting is_running for > a vcpu > > This patch attempts to fix these issues by restricting the exit/reboot > path to vcpu0 and the main thread. In particular, a KVM_SYSTEM_EVENT > will signal SIGKVMEXIT to vcpu0, which will join with the main thread > and then tear down the other vcpus before invoking any destructor code. > > Cc: Julien Grall <julien.grall@arm.com> > Cc: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Will Deacon <will.deacon@arm.com> I've been able to power cycle 1000 guests without any issue. Without this patch it break at ~50 cycles. Tested-by: Julien Grall <julien.grall@arm.com> Regards,
On 12/04/16 01:37, Will Deacon wrote: > Our exit/reboot code is a bit of a mess: > > - Both kvm__reboot and kvm_cpu_exit send SIGKVMEXIT to running vcpus > - When vcpu0 exits, the main thread starts executing destructors > (exitcalls) whilst other vcpus may be running > - The pause_lock isn't always held when inspecting is_running for > a vcpu > > This patch attempts to fix these issues by restricting the exit/reboot > path to vcpu0 and the main thread. In particular, a KVM_SYSTEM_EVENT > will signal SIGKVMEXIT to vcpu0, which will join with the main thread > and then tear down the other vcpus before invoking any destructor code. > > Cc: Julien Grall <julien.grall@arm.com> > Cc: Balbir Singh <bsingharora@gmail.com> > Signed-off-by: Will Deacon <will.deacon@arm.com> > --- > > Balbir -- does this work on ppc? Yes, it does Acked-by: Balbir Singh <bsingharora@gmail.com> My patch for exit race fixes can be dropped in favour of this change Balbir Singh -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 14, 2016 at 11:57:31AM +1000, Balbir Singh wrote: > > > On 12/04/16 01:37, Will Deacon wrote: > > Our exit/reboot code is a bit of a mess: > > > > - Both kvm__reboot and kvm_cpu_exit send SIGKVMEXIT to running vcpus > > - When vcpu0 exits, the main thread starts executing destructors > > (exitcalls) whilst other vcpus may be running > > - The pause_lock isn't always held when inspecting is_running for > > a vcpu > > > > This patch attempts to fix these issues by restricting the exit/reboot > > path to vcpu0 and the main thread. In particular, a KVM_SYSTEM_EVENT > > will signal SIGKVMEXIT to vcpu0, which will join with the main thread > > and then tear down the other vcpus before invoking any destructor code. > > > > Cc: Julien Grall <julien.grall@arm.com> > > Cc: Balbir Singh <bsingharora@gmail.com> > > Signed-off-by: Will Deacon <will.deacon@arm.com> > > --- > > > > Balbir -- does this work on ppc? > > > Yes, it does > > Acked-by: Balbir Singh <bsingharora@gmail.com> > > My patch for exit race fixes can be dropped in favour of this change Thanks, Balbir! I've pushed it out, along with your ppc changes. Will -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/builtin-run.c b/builtin-run.c index edcaf3ecde01..72b878dcff38 100644 --- a/builtin-run.c +++ b/builtin-run.c @@ -630,7 +630,6 @@ static struct kvm *kvm_cmd_run_init(int argc, const char **argv) static int kvm_cmd_run_work(struct kvm *kvm) { int i; - void *ret = NULL; for (i = 0; i < kvm->nrcpus; i++) { if (pthread_create(&kvm->cpus[i]->thread, NULL, kvm_cpu_thread, kvm->cpus[i]) != 0) @@ -638,7 +637,10 @@ static int kvm_cmd_run_work(struct kvm *kvm) } /* Only VCPU #0 is going to exit by itself when shutting down */ - return pthread_join(kvm->cpus[0]->thread, &ret); + if (pthread_join(kvm->cpus[0]->thread, NULL) != 0) + die("unable to join with vcpu 0"); + + return kvm_cpu__exit(kvm); } static void kvm_cmd_run_exit(struct kvm *kvm, int guest_ret) diff --git a/kvm-cpu.c b/kvm-cpu.c index 2af459b34807..cc8385f6143e 100644 --- a/kvm-cpu.c +++ b/kvm-cpu.c @@ -305,6 +305,7 @@ int kvm_cpu__exit(struct kvm *kvm) kvm_cpu__delete(kvm->cpus[0]); kvm->cpus[0] = NULL; + kvm__pause(kvm); for (i = 1; i < kvm->nrcpus; i++) { if (kvm->cpus[i]->is_running) { pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT); @@ -315,6 +316,7 @@ int kvm_cpu__exit(struct kvm *kvm) if (ret == NULL) r = 0; } + kvm__continue(kvm); free(kvm->cpus); @@ -324,4 +326,3 @@ int kvm_cpu__exit(struct kvm *kvm) return r; } -core_exit(kvm_cpu__exit); diff --git a/kvm.c b/kvm.c index 18b46068271e..7fa76f784de7 100644 --- a/kvm.c +++ b/kvm.c @@ -396,22 +396,15 @@ void kvm__dump_mem(struct kvm *kvm, unsigned long addr, unsigned long size, int void kvm__reboot(struct kvm *kvm) { - int i; - /* Check if the guest is running */ if (!kvm->cpus[0] || kvm->cpus[0]->thread == 0) return; - mutex_lock(&pause_lock); - - /* The kvm->cpus array contains a null pointer in the last location */ - for (i = 0; ; i++) { - if (kvm->cpus[i]) - pthread_kill(kvm->cpus[i]->thread, SIGKVMEXIT); - else - break; - } + pthread_kill(kvm->cpus[0]->thread, SIGKVMEXIT); +} +void kvm__continue(struct kvm *kvm) +{ mutex_unlock(&pause_lock); } @@ -419,12 +412,12 @@ void kvm__pause(struct kvm *kvm) { int i, paused_vcpus = 0; + mutex_lock(&pause_lock); + /* Check if the guest is running */ if (!kvm->cpus[0] || kvm->cpus[0]->thread == 0) return; - mutex_lock(&pause_lock); - pause_event = eventfd(0, 0); if (pause_event < 0) die("Failed creating pause notification event"); @@ -445,15 +438,6 @@ void kvm__pause(struct kvm *kvm) close(pause_event); } -void kvm__continue(struct kvm *kvm) -{ - /* Check if the guest is running */ - if (!kvm->cpus[0] || kvm->cpus[0]->thread == 0) - return; - - mutex_unlock(&pause_lock); -} - void kvm__notify_paused(void) { u64 p = 1;
Our exit/reboot code is a bit of a mess: - Both kvm__reboot and kvm_cpu_exit send SIGKVMEXIT to running vcpus - When vcpu0 exits, the main thread starts executing destructors (exitcalls) whilst other vcpus may be running - The pause_lock isn't always held when inspecting is_running for a vcpu This patch attempts to fix these issues by restricting the exit/reboot path to vcpu0 and the main thread. In particular, a KVM_SYSTEM_EVENT will signal SIGKVMEXIT to vcpu0, which will join with the main thread and then tear down the other vcpus before invoking any destructor code. Cc: Julien Grall <julien.grall@arm.com> Cc: Balbir Singh <bsingharora@gmail.com> Signed-off-by: Will Deacon <will.deacon@arm.com> --- Balbir -- does this work on ppc? builtin-run.c | 6 ++++-- kvm-cpu.c | 3 ++- kvm.c | 28 ++++++---------------------- 3 files changed, 12 insertions(+), 25 deletions(-)