Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.
On Thu, Nov 6, 2008 at 4:14 PM, Avi Kivity [EMAIL PROTECTED] wrote:
What happens here if the both free and dont have nonzero, differnt
-userspace_addr values? Is is even possible?
I dont think it can happen in the current kvm code, but I put that test in
order to respect the function behaviour of freeing any memory allocation
pointed to by free and not by dont (as described in the comment).
Also, the call chain is fishy. set_memory_region calls free_physmem_slot
which calls arch_set_memory_region. This is turning into pasta.
I agree, that's why I thought it would be better to put this outside
kvm_free_physmem_slot in my first patch. AFAICT, kvm_free_physmem_slot
is called by kvm_set_memory_region in order to free the memory holding
information regarding the slot but not the actual memory region held
by the slot: precisely because it is the role of kvm_set_memory_region
to free it.
So here is an attempt at something cleaner:
1 - Rename kvm_free_physmem_slot to kvm_free_physmem_slot_info
to indicate that it only frees the memory storing information about the slot
and not the memory region.
2- Make kvm_free_physmem free memory regions through
kvm_set_memory_region and let it free the slot info.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..e59dc10 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -614,11 +614,24 @@ out:
return kvm;
}
+static void kvm_free_physmem_slot(struct kvm *kvm,
+ struct kvm_memory_slot *slot)
+{
+ struct kvm_userspace_memory_region mem = {
+ .slot = memslot_id(kvm, slot),
+ .guest_phys_addr = slot-base_gfn PAGE_SHIFT,
+ .memory_size = 0,
+ .flags = 0,
+ };
+
+ kvm_set_memory_region(kvm, mem, slot-user_alloc);
+}
+
/*
* Free any memory in @free but not in @dont.
*/
-static void kvm_free_physmem_slot(struct kvm_memory_slot *free,
- struct kvm_memory_slot *dont)
+static void kvm_free_physmem_slot_info(struct kvm_memory_slot *free,
+ struct kvm_memory_slot *dont)
{
if (!dont || free-rmap != dont-rmap)
vfree(free-rmap);
@@ -640,7 +653,7 @@ void kvm_free_physmem(struct kvm *kvm)
int i;
for (i = 0; i kvm-nmemslots; ++i)
- kvm_free_physmem_slot(kvm-memslots[i], NULL);
+ kvm_free_physmem_slot(kvm, kvm-memslots[i]);
}
static void kvm_destroy_vm(struct kvm *kvm)
@@ -745,10 +758,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- /* Free page dirty bitmap if unneeded */
+ /* Free any unneeded data */
if (!(new.flags KVM_MEM_LOG_DIRTY_PAGES))
new.dirty_bitmap = NULL;
+ if (!npages) {
+ new.rmap = NULL;
+ new.lpage_info = NULL;
+ }
r = -ENOMEM;
/* Allocate if a slot is being created */
@@ -821,7 +838,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- kvm_free_physmem_slot(old, new);
+ kvm_free_physmem_slot_info(old, new);
#ifdef CONFIG_DMAR
/* map the pages in iommu page table */
r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +848,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
return 0;
out_free:
- kvm_free_physmem_slot(new, old);
+ kvm_free_physmem_slot_info(new, old);
out:
return r;
Also, I've been reading a bit more about the linux mm and I now think
that to be able to use kvm-mm in arch_set_memory_region we need
to increase mm_users instead of mm_count. However, if we do that,
since memory maps wont be cleared when the process exits the kvm
fds which are still mapped in userspace will not be released so we will
have a bigger memory leak.
Any ideas on how to fix this properly?
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.
[Updated the patch taking your comments into account]
Make sure that kvm_free_physmem_slot also frees the VM memory
if it was allocated by the kernel.
Signed-off-by: François Diakhaté [EMAIL PROTECTED]
---
arch/x86/kvm/x86.c | 10 +-
virt/kvm/kvm_main.c | 18 ++
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 883c137..818220b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4179,13 +4179,13 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
if (npages !old.rmap) {
unsigned long userspace_addr;
- down_write(current-mm-mmap_sem);
+ down_write(kvm-mm-mmap_sem);
userspace_addr = do_mmap(NULL, 0,
npages * PAGE_SIZE,
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS,
0);
- up_write(current-mm-mmap_sem);
+ up_write(kvm-mm-mmap_sem);
if (IS_ERR((void *)userspace_addr))
return PTR_ERR((void *)userspace_addr);
@@ -4198,10 +4198,10 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
if (!old.user_alloc old.rmap) {
int ret;
- down_write(current-mm-mmap_sem);
- ret = do_munmap(current-mm, old.userspace_addr,
+ down_write(kvm-mm-mmap_sem);
+ ret = do_munmap(kvm-mm, old.userspace_addr,
old.npages * PAGE_SIZE);
- up_write(current-mm-mmap_sem);
+ up_write(kvm-mm-mmap_sem);
if (ret 0)
printk(KERN_WARNING
kvm_vm_ioctl_set_memory_region:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..b420930 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -617,9 +617,19 @@ out:
/*
* 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-userspace_addr != dont-userspace_addr) {
+ struct kvm_userspace_memory_region mem = {
+ .slot = memslot_id(kvm, free),
+ .guest_phys_addr = free-base_gfn PAGE_SHIFT,
+ .memory_size = 0,
+ .flags = 0,
+ };
+ kvm_arch_set_memory_region(kvm, mem, *free, free-user_alloc);
+ }
+
if (!dont || free-rmap != dont-rmap)
vfree(free-rmap);
@@ -640,7 +650,7 @@ void kvm_free_physmem(struct kvm *kvm)
int i;
for (i = 0; i kvm-nmemslots; ++i)
- kvm_free_physmem_slot(kvm-memslots[i], NULL);
+ kvm_free_physmem_slot(kvm, kvm-memslots[i], NULL);
}
static void kvm_destroy_vm(struct kvm *kvm)
@@ -821,7 +831,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- kvm_free_physmem_slot(old, new);
+ kvm_free_physmem_slot(kvm, old, new);
#ifdef CONFIG_DMAR
/* map the pages in iommu page table */
r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +841,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
return 0;
out_free:
- kvm_free_physmem_slot(new, old);
+ kvm_free_physmem_slot(kvm, new, old);
out:
return r;
--
1.6.0.3
Re: [PATCH 2/2] KVM: Fix kvm_free_physmem_slot memory leak.
[Sorry, I realized I forgot to check style, here is the fixed patch]
Make sure that kvm_free_physmem_slot also frees the VM memory
if it was allocated by the kernel.
Signed-off-by: François Diakhaté [EMAIL PROTECTED]
---
arch/x86/kvm/x86.c | 10 +-
virt/kvm/kvm_main.c | 19 +++
2 files changed, 20 insertions(+), 9 deletions(-)
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 883c137..818220b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -4179,13 +4179,13 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
if (npages !old.rmap) {
unsigned long userspace_addr;
- down_write(current-mm-mmap_sem);
+ down_write(kvm-mm-mmap_sem);
userspace_addr = do_mmap(NULL, 0,
npages * PAGE_SIZE,
PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANONYMOUS,
0);
- up_write(current-mm-mmap_sem);
+ up_write(kvm-mm-mmap_sem);
if (IS_ERR((void *)userspace_addr))
return PTR_ERR((void *)userspace_addr);
@@ -4198,10 +4198,10 @@ int kvm_arch_set_memory_region(struct kvm *kvm,
if (!old.user_alloc old.rmap) {
int ret;
- down_write(current-mm-mmap_sem);
- ret = do_munmap(current-mm, old.userspace_addr,
+ down_write(kvm-mm-mmap_sem);
+ ret = do_munmap(kvm-mm, old.userspace_addr,
old.npages * PAGE_SIZE);
- up_write(current-mm-mmap_sem);
+ up_write(kvm-mm-mmap_sem);
if (ret 0)
printk(KERN_WARNING
kvm_vm_ioctl_set_memory_region:
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a87f45e..c7d6585 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -617,9 +617,20 @@ out:
/*
* 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-userspace_addr != dont-userspace_addr) {
+ struct kvm_userspace_memory_region mem = {
+ .slot = memslot_id(kvm, free),
+ .guest_phys_addr = free-base_gfn PAGE_SHIFT,
+ .memory_size = 0,
+ .flags = 0,
+ };
+ kvm_arch_set_memory_region(kvm, mem, *free, free-user_alloc);
+ }
+
if (!dont || free-rmap != dont-rmap)
vfree(free-rmap);
@@ -640,7 +651,7 @@ void kvm_free_physmem(struct kvm *kvm)
int i;
for (i = 0; i kvm-nmemslots; ++i)
- kvm_free_physmem_slot(kvm-memslots[i], NULL);
+ kvm_free_physmem_slot(kvm, kvm-memslots[i], NULL);
}
static void kvm_destroy_vm(struct kvm *kvm)
@@ -821,7 +832,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
goto out_free;
}
- kvm_free_physmem_slot(old, new);
+ kvm_free_physmem_slot(kvm, old, new);
#ifdef CONFIG_DMAR
/* map the pages in iommu page table */
r = kvm_iommu_map_pages(kvm, base_gfn, npages);
@@ -831,7 +842,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
return 0;
out_free:
- kvm_free_physmem_slot(new, old);
+ kvm_free_physmem_slot(kvm, new, old);
out:
return r;
--
1.6.0.3
N�r��yb�X��ǧv�^�){.n�+h����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z� �w���?��)ߢf
