[PATCH] KVM: Fix srcu struct leakage
Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif + cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On 11/09/2010 01:41 PM, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Applied and queued for 2.6.37; thanks. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif + cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 kvm_arch_destroy_vm does it. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
Am 09.11.2010 14:53, Marcelo Tosatti wrote: On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif +cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 kvm_arch_destroy_vm does it. Oh, indeed. That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine). Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
Am 09.11.2010 14:59, Jan Kiszka wrote: Am 09.11.2010 14:53, Marcelo Tosatti wrote: On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif + cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 kvm_arch_destroy_vm does it. Oh, indeed. That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine). Oh, the setup/cleanup maze is a tribute to the IA64 zombie. Mmh... Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On Tue, Nov 09, 2010 at 02:59:45PM +0100, Jan Kiszka wrote:
Am 09.11.2010 14:53, Marcelo Tosatti wrote:
On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote:
Clean up the srcu struct on vm destruction.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
virt/kvm/kvm_main.c |1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4111a4b..6ec58d1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
#else
kvm_arch_flush_shadow(kvm);
#endif
+ cleanup_srcu_struct(kvm-srcu);
kvm_arch_destroy_vm(kvm);
hardware_disable_all();
mmdrop(mm);
--
1.7.1
kvm_arch_destroy_vm does it.
Oh, indeed.
That I guess it's time to clean up, move generic allocation and release
into generic code. Having kvm_arch_create/destroy_vm manage the kvm
object allocation does not look very nice (and caused this confusion of
mine).
Jan
kvm_iommu_unmap_guest enters SRCU critical section, and struct kvm is
freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
code.
But sure, it would be nicer if it could be moved to generic code.
void kvm_arch_destroy_vm(struct kvm *kvm)
{
kvm_iommu_unmap_guest(kvm);
kfree(kvm-arch.vpic);
kfree(kvm-arch.vioapic);
kvm_free_vcpus(kvm);
kvm_free_physmem(kvm);
if (kvm-arch.apic_access_page)
put_page(kvm-arch.apic_access_page);
if (kvm-arch.ept_identity_pagetable)
put_page(kvm-arch.ept_identity_pagetable);
cleanup_srcu_struct(kvm-srcu);
kfree(kvm);
}
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On 11/09/2010 04:07 PM, Jan Kiszka wrote:
kvm_iommu_unmap_guest enters SRCU critical section, and struct kvm is
freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
code.
The problem is the arch-specific allocation of the kvm object on IA64.
All others simply call kmalloc/kzalloc for the struct itself, but IA64
embeds it into some larger context. Not sure why, though.
ia64 virt is special - the guest has a virtual address space for host
data. This data is shared with the host address space, but on different
addresses.
I guess to reduce tlb costs ia64 kvm uses a large order allocation for
both struct kvm and other random data, which is why the allocation is
different.
It could be refactored to something like
#ifndef CONFIG_HAVE_SPECIAL_KVM_ALLOC
static inline struct kvm *kvm_arch_alloc_vm(void)
{
return kzalloc(...);
}
static inline void kvm_arch_free_vm(struct kvm *kvm)
{
kfree(kvm);
}
#endif
and have ia64 provide its special stuff. The practice of duplicating
common code just because of one outlier is bad, there are better ways.
(and I will accept untested ia64 patches provided kvm-ia64@ is copied).
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
Am 09.11.2010 15:30, Avi Kivity wrote:
On 11/09/2010 04:07 PM, Jan Kiszka wrote:
kvm_iommu_unmap_guest enters SRCU critical section, and struct kvm is
freed at the end of kvm_arch_destroy_vm. Thats why its not in generic
code.
The problem is the arch-specific allocation of the kvm object on IA64.
All others simply call kmalloc/kzalloc for the struct itself, but IA64
embeds it into some larger context. Not sure why, though.
ia64 virt is special - the guest has a virtual address space for host
data. This data is shared with the host address space, but on different
addresses.
I guess to reduce tlb costs ia64 kvm uses a large order allocation for
both struct kvm and other random data, which is why the allocation is
different.
It could be refactored to something like
#ifndef CONFIG_HAVE_SPECIAL_KVM_ALLOC
static inline struct kvm *kvm_arch_alloc_vm(void)
{
return kzalloc(...);
}
static inline void kvm_arch_free_vm(struct kvm *kvm)
{
kfree(kvm);
}
#endif
Yep, that's what I'm already working on.
and have ia64 provide its special stuff. The practice of duplicating
common code just because of one outlier is bad, there are better ways.
(and I will accept untested ia64 patches provided kvm-ia64@ is copied).
OK, will come. Refactoring will affect all archs, so all need to test
and ack.
Jan
--
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
Am 09.11.2010 15:03, Marcelo Tosatti wrote: On Tue, Nov 09, 2010 at 02:59:45PM +0100, Jan Kiszka wrote: Am 09.11.2010 14:53, Marcelo Tosatti wrote: On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszka jan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif + cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 kvm_arch_destroy_vm does it. Oh, indeed. That I guess it's time to clean up, move generic allocation and release into generic code. Having kvm_arch_create/destroy_vm manage the kvm object allocation does not look very nice (and caused this confusion of mine). Jan kvm_iommu_unmap_guest enters SRCU critical section, and struct kvm is freed at the end of kvm_arch_destroy_vm. Thats why its not in generic code. The problem is the arch-specific allocation of the kvm object on IA64. All others simply call kmalloc/kzalloc for the struct itself, but IA64 embeds it into some larger context. Not sure why, though. Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On 11/09/2010 03:53 PM, Marcelo Tosatti wrote: On Tue, Nov 09, 2010 at 12:41:26PM +0100, Jan Kiszka wrote: Clean up the srcu struct on vm destruction. Signed-off-by: Jan Kiszkajan.kis...@siemens.com --- virt/kvm/kvm_main.c |1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 4111a4b..6ec58d1 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -513,6 +513,7 @@ static void kvm_destroy_vm(struct kvm *kvm) #else kvm_arch_flush_shadow(kvm); #endif + cleanup_srcu_struct(kvm-srcu); kvm_arch_destroy_vm(kvm); hardware_disable_all(); mmdrop(mm); -- 1.7.1 kvm_arch_destroy_vm does it. True. Dropped. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KVM: Fix srcu struct leakage
On 11/09/2010 04:36 PM, Jan Kiszka wrote: and have ia64 provide its special stuff. The practice of duplicating common code just because of one outlier is bad, there are better ways. (and I will accept untested ia64 patches provided kvm-ia64@ is copied). OK, will come. Refactoring will affect all archs, so all need to test and ack. I can at least build on all of them. Should set up a buildbot... -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] KVM: Fix srcu struct leakage
From: Jan Kiszka jan.kis...@siemens.com
Clean up the srcu struct and refactor its release on early errors.
Signed-off-by: Jan Kiszka jan.kis...@siemens.com
---
virt/kvm/kvm_main.c | 15 +++
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 4111a4b..c80a44a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -401,23 +401,19 @@ static struct kvm *kvm_create_vm(void)
r = -ENOMEM;
kvm-memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
if (!kvm-memslots)
- goto out_err;
+ goto out_err_nosrcu;
if (init_srcu_struct(kvm-srcu))
- goto out_err;
+ goto out_err_nosrcu;
for (i = 0; i KVM_NR_BUSES; i++) {
kvm-buses[i] = kzalloc(sizeof(struct kvm_io_bus),
GFP_KERNEL);
- if (!kvm-buses[i]) {
- cleanup_srcu_struct(kvm-srcu);
+ if (!kvm-buses[i])
goto out_err;
- }
}
r = kvm_init_mmu_notifier(kvm);
- if (r) {
- cleanup_srcu_struct(kvm-srcu);
+ if (r)
goto out_err;
- }
kvm-mm = current-mm;
atomic_inc(kvm-mm-mm_count);
@@ -435,6 +431,8 @@ out:
return kvm;
out_err:
+ cleanup_srcu_struct(kvm-srcu);
+out_err_nosrcu:
hardware_disable_all();
out_err_nodisable:
for (i = 0; i KVM_NR_BUSES; i++)
@@ -516,6 +514,7 @@ static void kvm_destroy_vm(struct kvm *kvm)
kvm_arch_destroy_vm(kvm);
hardware_disable_all();
mmdrop(mm);
+ cleanup_srcu_struct(kvm-srcu);
}
void kvm_get_kvm(struct kvm *kvm)
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
