| Message ID | 516F1A35.5090106@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
I don't have a significant objection to freeing the memory in kvm_arch_free_memslot, although I think it's a little harder to understand. I like the idea of being symmetric (memory is allocated by calling kvm_set_memory_region and freed using the same technique). That way if someone changes from vm_mmap to something else it will be obvious that they need to change both. Also, it looks like your patch is based on something several commits behind HEAD on virt/kvm/kvm.git, which significantly affect your patch. In the HEAD version it assumes that user_alloc is always set unless it's a private memslot. This appears to already have been the case and allows a bunch of simplifications, some of which would apply to your patch. > > What about something like this (uncompiled/untested) > > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index 8b3a9c0..6706134 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1563,7 +1563,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > return VM_FAULT_SIGBUS; > } > > -void kvm_arch_free_memslot(struct kvm_memory_slot *free, > +void kvm_arch_free_memslot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont) > { > } > diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c > index 4d213b8..a654580 100644 > --- a/arch/powerpc/kvm/powerpc.c > +++ b/arch/powerpc/kvm/powerpc.c > @@ -299,7 +299,8 @@ long kvm_arch_dev_ioctl(struct file *filp, > return -EINVAL; > } > > -void kvm_arch_free_memslot(struct kvm_memory_slot *free, > +void kvm_arch_free_memslot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont) > { > if (!dont || free->arch.rmap != dont->arch.rmap) { > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index ecced9d..e2159c1 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -912,7 +912,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) > return VM_FAULT_SIGBUS; > } > > -void kvm_arch_free_memslot(struct kvm_memory_slot *free, > +void kvm_arch_free_memslot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont) > { > } > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 224a7e7..f9fa0d1 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6357,11 +6367,26 @@ void kvm_arch_destroy_vm(struct kvm *kvm) > kfree(rcu_dereference_check(kvm->arch.apic_map, 1)); > } > > -void kvm_arch_free_memslot(struct kvm_memory_slot *free, > +void kvm_arch_free_memslot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont) > { > int i; > > + if (current->mm == kvm->mm && free->user_alloc) { I think you mean !free->user_alloc. Also, you could check the memslot->id instead so that we can remove the user_alloc field entirely as it doesn't serve a useful function anymore. > + if (!dont || !dont->user_alloc || > + free->userspace_addr != dont->userspace_addr) { > + int ret; > + > + ret = vm_munmap(free->userspace_addr, > + free->npages * PAGE_SIZE); > + if (ret < 0) > + printk(KERN_WARNING > + "kvm_vm_ioctl_set_memory_region: " > + "failed to munmap memory\n"); > + } > + } > + > for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) { > if (!dont || free->arch.rmap[i] != dont->arch.rmap[i]) { > kvm_kvfree(free->arch.rmap[i]); > @@ -6453,7 +6478,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > *x86 needs to handle !user_alloc case. > */ > if (!user_alloc) { > - if (npages && !old.npages) { > + if (npages != old.npages) { > unsigned long userspace_addr; > > userspace_addr = vm_mmap(NULL, 0, > @@ -6466,7 +6491,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, > return PTR_ERR((void *)userspace_addr); > > memslot->userspace_addr = userspace_addr; > - } > + } else > + memslot->userspace_addr = old.userspace_addr; > } > > > @@ -6481,17 +6507,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, > > int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT; > > - if (!user_alloc && !old.user_alloc && old.npages && !npages) { > - int ret; > - > - ret = vm_munmap(old.userspace_addr, > - old.npages * PAGE_SIZE); > - if (ret < 0) > - printk(KERN_WARNING > - "kvm_vm_ioctl_set_memory_region: " > - "failed to munmap memory\n"); > - } > - > if (!kvm->arch.n_requested_mmu_pages) > nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm); > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index ecc5543..8f2a863 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -436,7 +436,8 @@ int kvm_set_memory_region(struct kvm *kvm, > int __kvm_set_memory_region(struct kvm *kvm, > struct kvm_userspace_memory_region *mem, > int user_alloc); > -void kvm_arch_free_memslot(struct kvm_memory_slot *free, > +void kvm_arch_free_memslot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont); > int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages); > int kvm_arch_prepare_memory_region(struct kvm *kvm, > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index be70035..ea63b9c 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -546,13 +546,14 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot) > /* > * Free any memory in @free but not in @dont. > */ > -static void kvm_free_physmem_slot(struct kvm_memory_slot *free, > +static void kvm_free_physmem_slot(struct kvm *kvm, > + struct kvm_memory_slot *free, > struct kvm_memory_slot *dont) > { > if (!dont || free->dirty_bitmap != dont->dirty_bitmap) > kvm_destroy_dirty_bitmap(free); > > - kvm_arch_free_memslot(free, dont); > + kvm_arch_free_memslot(kvm, free, dont); > > free->npages = 0; > } > @@ -563,7 +564,7 @@ void kvm_free_physmem(struct kvm *kvm) > struct kvm_memory_slot *memslot; > > kvm_for_each_memslot(memslot, slots) > - kvm_free_physmem_slot(memslot, NULL); > + kvm_free_physmem_slot(kvm, memslot, NULL); > > kfree(kvm->memslots); > } > @@ -851,13 +852,13 @@ int __kvm_set_memory_region(struct kvm *kvm, > > kvm_arch_commit_memory_region(kvm, mem, old, user_alloc); > > - kvm_free_physmem_slot(&old, &new); > + kvm_free_physmem_slot(kvm, &old, &new); > kfree(old_memslots); > > return 0; > > out_free: > - kvm_free_physmem_slot(&new, &old); > + kvm_free_physmem_slot(kvm, &new, &old); > out: > return r; > -- 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
Il 18/04/2013 01:03, Andrew Honig ha scritto: > I don't have a significant objection to freeing the memory in > kvm_arch_free_memslot, although I think it's a little harder to > understand. I like the idea of being symmetric (memory is allocated > by calling kvm_set_memory_region and freed using the same technique). > That way if someone changes from vm_mmap to something else it will be > obvious that they need to change both. > > Also, it looks like your patch is based on something several commits > behind HEAD on virt/kvm/kvm.git, Yeah, it was just whatever version I had checked out on the laptop. :) So that maintainers can look at both approaches and see what they prefer. Gleb, Marcelo, wdyt? Paolo > which significantly affect your > patch. In the HEAD version it assumes that user_alloc is always set > unless it's a private memslot. This appears to already have been the > case and allows a bunch of simplifications, some of which would apply > to your patch. -- 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 18, 2013 at 10:20:05AM +0200, Paolo Bonzini wrote: > Il 18/04/2013 01:03, Andrew Honig ha scritto: > > I don't have a significant objection to freeing the memory in > > kvm_arch_free_memslot, although I think it's a little harder to > > understand. I like the idea of being symmetric (memory is allocated > > by calling kvm_set_memory_region and freed using the same technique). > > That way if someone changes from vm_mmap to something else it will be > > obvious that they need to change both. > > > > Also, it looks like your patch is based on something several commits > > behind HEAD on virt/kvm/kvm.git, > > Yeah, it was just whatever version I had checked out on the laptop. :) > So that maintainers can look at both approaches and see what they prefer. > > Gleb, Marcelo, wdyt? > I agree with Andrew. Having kvm_arch_free_memslot() unmap memory, but only for subset of memslots is not cleanest approach. Userspace interface for slot deletion is to "create" the slot with zero size, Andrew patch uses the same, well tested, code path. -- Gleb. -- 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/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c index 8b3a9c0..6706134 100644 --- a/arch/ia64/kvm/kvm-ia64.c +++ b/arch/ia64/kvm/kvm-ia64.c @@ -1563,7 +1563,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) return VM_FAULT_SIGBUS; } -void kvm_arch_free_memslot(struct kvm_memory_slot *free, +void kvm_arch_free_memslot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont) { } diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c index 4d213b8..a654580 100644 --- a/arch/powerpc/kvm/powerpc.c +++ b/arch/powerpc/kvm/powerpc.c @@ -299,7 +299,8 @@ long kvm_arch_dev_ioctl(struct file *filp, return -EINVAL; } -void kvm_arch_free_memslot(struct kvm_memory_slot *free, +void kvm_arch_free_memslot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont) { if (!dont || free->arch.rmap != dont->arch.rmap) { diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index ecced9d..e2159c1 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -912,7 +912,8 @@ int kvm_arch_vcpu_fault(struct kvm_vcpu *vcpu, struct vm_fault *vmf) return VM_FAULT_SIGBUS; } -void kvm_arch_free_memslot(struct kvm_memory_slot *free, +void kvm_arch_free_memslot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont) { } diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 224a7e7..f9fa0d1 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6357,11 +6367,26 @@ void kvm_arch_destroy_vm(struct kvm *kvm) kfree(rcu_dereference_check(kvm->arch.apic_map, 1)); } -void kvm_arch_free_memslot(struct kvm_memory_slot *free, +void kvm_arch_free_memslot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont) { int i; + if (current->mm == kvm->mm && free->user_alloc) { + if (!dont || !dont->user_alloc || + free->userspace_addr != dont->userspace_addr) { + int ret; + + ret = vm_munmap(free->userspace_addr, + free->npages * PAGE_SIZE); + if (ret < 0) + printk(KERN_WARNING + "kvm_vm_ioctl_set_memory_region: " + "failed to munmap memory\n"); + } + } + for (i = 0; i < KVM_NR_PAGE_SIZES; ++i) { if (!dont || free->arch.rmap[i] != dont->arch.rmap[i]) { kvm_kvfree(free->arch.rmap[i]); @@ -6453,7 +6478,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, *x86 needs to handle !user_alloc case. */ if (!user_alloc) { - if (npages && !old.npages) { + if (npages != old.npages) { unsigned long userspace_addr; userspace_addr = vm_mmap(NULL, 0, @@ -6466,7 +6491,8 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm, return PTR_ERR((void *)userspace_addr); memslot->userspace_addr = userspace_addr; - } + } else + memslot->userspace_addr = old.userspace_addr; } @@ -6481,17 +6507,6 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT; - if (!user_alloc && !old.user_alloc && old.npages && !npages) { - int ret; - - ret = vm_munmap(old.userspace_addr, - old.npages * PAGE_SIZE); - if (ret < 0) - printk(KERN_WARNING - "kvm_vm_ioctl_set_memory_region: " - "failed to munmap memory\n"); - } - if (!kvm->arch.n_requested_mmu_pages) nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ecc5543..8f2a863 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -436,7 +436,8 @@ int kvm_set_memory_region(struct kvm *kvm, int __kvm_set_memory_region(struct kvm *kvm, struct kvm_userspace_memory_region *mem, int user_alloc); -void kvm_arch_free_memslot(struct kvm_memory_slot *free, +void kvm_arch_free_memslot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont); int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages); int kvm_arch_prepare_memory_region(struct kvm *kvm, diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index be70035..ea63b9c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -546,13 +546,14 @@ static void kvm_destroy_dirty_bitmap(struct kvm_memory_slot *memslot) /* * Free any memory in @free but not in @dont. */ -static void kvm_free_physmem_slot(struct kvm_memory_slot *free, +static void kvm_free_physmem_slot(struct kvm *kvm, + struct kvm_memory_slot *free, struct kvm_memory_slot *dont) { if (!dont || free->dirty_bitmap != dont->dirty_bitmap) kvm_destroy_dirty_bitmap(free); - kvm_arch_free_memslot(free, dont); + kvm_arch_free_memslot(kvm, free, dont); free->npages = 0; } @@ -563,7 +564,7 @@ void kvm_free_physmem(struct kvm *kvm) struct kvm_memory_slot *memslot; kvm_for_each_memslot(memslot, slots) - kvm_free_physmem_slot(memslot, NULL); + kvm_free_physmem_slot(kvm, memslot, NULL); kfree(kvm->memslots); } @@ -851,13 +852,13 @@ int __kvm_set_memory_region(struct kvm *kvm, kvm_arch_commit_memory_region(kvm, mem, old, user_alloc); - kvm_free_physmem_slot(&old, &new); + kvm_free_physmem_slot(kvm, &old, &new); kfree(old_memslots); return 0; out_free: - kvm_free_physmem_slot(&new, &old); + kvm_free_physmem_slot(kvm, &new, &old); out: return r;