| Message ID | 20191024230327.140935-3-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: call kvm_arch_destroy_vm if vm creation fails | expand |
On Thu, Oct 24, 2019 at 04:03:26PM -0700, Jim Mattson wrote: > This reorganization will allow us to call kvm_arch_destroy_vm in the > event that kvm_create_vm fails after calling kvm_arch_init_vm. > > Suggested-by: Junaid Shahid <junaids@google.com> > Signed-off-by: Jim Mattson <jmattson@google.com> > Reviewed-by: Junaid Shahid <junaids@google.com> > --- > virt/kvm/kvm_main.c | 40 +++++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 19 deletions(-) > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 525e0dbc623f9..77819597d7e8e 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -627,8 +627,9 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd) > > static struct kvm *kvm_create_vm(unsigned long type) > { > - int r, i; > struct kvm *kvm = kvm_arch_alloc_vm(); > + int r = -ENOMEM; > + int i; > > if (!kvm) > return ERR_PTR(-ENOMEM); > @@ -642,6 +643,25 @@ static struct kvm *kvm_create_vm(unsigned long type) > mutex_init(&kvm->slots_lock); > INIT_LIST_HEAD(&kvm->devices); > > + BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX); > + > + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { > + struct kvm_memslots *slots = kvm_alloc_memslots(); > + > + if (!slots) > + goto out_err_no_disable; > + /* Generations must be different for each address space. */ > + slots->generation = i; > + rcu_assign_pointer(kvm->memslots[i], slots); > + } > + > + for (i = 0; i < KVM_NR_BUSES; i++) { > + rcu_assign_pointer(kvm->buses[i], > + kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); > + if (!kvm->buses[i]) > + goto out_err_no_disable; Personally I'd prefer to add labels for each stage of destruction instead of abusing the NULL handling of kfree() and kvm_free_memslots(), especially since not doing so forces the next patch to update these gotos. Inverting the labels to describe what's being done instead of what's not being done might help with the readability and naming. > + } > + > r = kvm_arch_init_vm(kvm, type); > if (r) > goto out_err_no_disable; > @@ -654,28 +674,10 @@ static struct kvm *kvm_create_vm(unsigned long type) > INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list); > #endif > > - BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX); > - > - r = -ENOMEM; > - for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { > - struct kvm_memslots *slots = kvm_alloc_memslots(); > - if (!slots) > - goto out_err_no_srcu; > - /* Generations must be different for each address space. */ > - slots->generation = i; > - rcu_assign_pointer(kvm->memslots[i], slots); > - } > - > if (init_srcu_struct(&kvm->srcu)) > goto out_err_no_srcu; > if (init_srcu_struct(&kvm->irq_srcu)) > goto out_err_no_irq_srcu; > - for (i = 0; i < KVM_NR_BUSES; i++) { > - rcu_assign_pointer(kvm->buses[i], > - kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); > - if (!kvm->buses[i]) > - goto out_err; > - } > > r = kvm_init_mmu_notifier(kvm); > if (r) > -- > 2.24.0.rc0.303.g954a862665-goog >
On 25/10/19 01:28, Sean Christopherson wrote: > Personally I'd prefer to add labels for each stage of destruction instead > of abusing the NULL handling of kfree() and kvm_free_memslots(), especially > since not doing so forces the next patch to update these gotos. I'm not sure the two are related, and the NULL handling is definitely a feature. Regarding naming the gotos positively vs. negatively, I find the negative naming slightly easier to review: init(); if (foo()) /* 1 */ goto out_no_unfoo; /* 1 */ bar(); out: unbar(); /* 2 */ out_no_unbar: /* 2 */ unfoo(); out_no_unfoo; uninit(); vs. init(); /* 1 */ if (foo()) goto out_init; /* 1 */ bar(); out: unbar(); out_foo: unfoo(); out_init: /* 2 */ uninit(); /* 2 */ Perhaps I would name the labels "no_enable" and "no_arch_init_vm", but these patches can be applied first anyway. Paolo
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 525e0dbc623f9..77819597d7e8e 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -627,8 +627,9 @@ static int kvm_create_vm_debugfs(struct kvm *kvm, int fd) static struct kvm *kvm_create_vm(unsigned long type) { - int r, i; struct kvm *kvm = kvm_arch_alloc_vm(); + int r = -ENOMEM; + int i; if (!kvm) return ERR_PTR(-ENOMEM); @@ -642,6 +643,25 @@ static struct kvm *kvm_create_vm(unsigned long type) mutex_init(&kvm->slots_lock); INIT_LIST_HEAD(&kvm->devices); + BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX); + + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { + struct kvm_memslots *slots = kvm_alloc_memslots(); + + if (!slots) + goto out_err_no_disable; + /* Generations must be different for each address space. */ + slots->generation = i; + rcu_assign_pointer(kvm->memslots[i], slots); + } + + for (i = 0; i < KVM_NR_BUSES; i++) { + rcu_assign_pointer(kvm->buses[i], + kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); + if (!kvm->buses[i]) + goto out_err_no_disable; + } + r = kvm_arch_init_vm(kvm, type); if (r) goto out_err_no_disable; @@ -654,28 +674,10 @@ static struct kvm *kvm_create_vm(unsigned long type) INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list); #endif - BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX); - - r = -ENOMEM; - for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { - struct kvm_memslots *slots = kvm_alloc_memslots(); - if (!slots) - goto out_err_no_srcu; - /* Generations must be different for each address space. */ - slots->generation = i; - rcu_assign_pointer(kvm->memslots[i], slots); - } - if (init_srcu_struct(&kvm->srcu)) goto out_err_no_srcu; if (init_srcu_struct(&kvm->irq_srcu)) goto out_err_no_irq_srcu; - for (i = 0; i < KVM_NR_BUSES; i++) { - rcu_assign_pointer(kvm->buses[i], - kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT)); - if (!kvm->buses[i]) - goto out_err; - } r = kvm_init_mmu_notifier(kvm); if (r)