KVM: register the kvm mmu cache with the shrinker.

KVM: register the kvm mmu cache with the shrinker.

[Izik Eidus]

this is the shrinker patch with all comments applied beside adding aging mechanism
it look like the aging mechanism is not really needed and therefor for now it isn't
implemented.

>From 8503a57ae88ba819e4eac6371172772c98b485f0 Mon Sep 17 00:00:00 2001
From: Izik Eidus <izike@qumranet.com>
Date: Tue, 25 Mar 2008 14:03:27 +0200
Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.

Signed-off-by: Izik Eidus <izike@qumranet.com>
---
 arch/x86/kvm/mmu.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index c67ec62..c42c0db 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1972,6 +1972,57 @@ void kvm_mmu_zap_all(struct kvm *kvm)
 	kvm_flush_remote_tlbs(kvm);
 }
 
+void kvm_mmu_remove_one_alloc_mmu_page(struct kvm *kvm)
+{
+	struct kvm_mmu_page *page;
+
+	page = container_of(kvm->arch.active_mmu_pages.prev,
+			    struct kvm_mmu_page, link);
+	kvm_mmu_zap_page(kvm, page);
+}
+
+static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)
+{
+	struct kvm *kvm;
+	struct kvm *kvm_freed = NULL;
+	int cache_count = 0;
+
+	spin_lock(&kvm_lock);
+	if (list_empty(&vm_list)) {
+		spin_unlock(&kvm_lock);
+		return 0;
+	}
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		int npages;
+
+		spin_lock(&kvm->mmu_lock);
+		npages = kvm->arch.n_alloc_mmu_pages -
+			 kvm->arch.n_free_mmu_pages;
+		cache_count += npages - KVM_MIN_ALLOC_MMU_PAGES;
+		if (!kvm_freed && nr_to_scan > 0 && npages >
+		    KVM_MIN_ALLOC_MMU_PAGES) {
+			kvm_mmu_remove_one_alloc_mmu_page(kvm);
+			cache_count--;
+			kvm_freed = kvm;
+		}
+		nr_to_scan--;
+
+		spin_unlock(&kvm->mmu_lock);
+	}
+	if (kvm_freed) {
+		list_del(&kvm_freed->vm_list);
+		list_add_tail(&kvm_freed->vm_list, &vm_list);
+	}
+	spin_unlock(&kvm_lock);
+
+	return cache_count;
+}
+
+static struct shrinker mmu_shrinker = {
+	.shrink = mmu_shrink,
+	.seeks = DEFAULT_SEEKS * 10,
+};
+
 void kvm_mmu_module_exit(void)
 {
 	if (pte_chain_cache)
@@ -1980,6 +2031,7 @@ void kvm_mmu_module_exit(void)
 		kmem_cache_destroy(rmap_desc_cache);
 	if (mmu_page_header_cache)
 		kmem_cache_destroy(mmu_page_header_cache);

-- 
woof.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: KVM: register the kvm mmu cache with the shrinker.

[Avi Kivity]

Izik Eidus wrote:
> this is the shrinker patch with all comments applied beside adding aging mechanism
> it look like the aging mechanism is not really needed and therefor for now it isn't
> implemented.
>
> From 8503a57ae88ba819e4eac6371172772c98b485f0 Mon Sep 17 00:00:00 2001
> From: Izik Eidus <izike@qumranet.com>
> Date: Tue, 25 Mar 2008 14:03:27 +0200
> Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.
>
> Signed-off-by: Izik Eidus <izike@qumranet.com>
> ---
>  arch/x86/kvm/mmu.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index c67ec62..c42c0db 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1972,6 +1972,57 @@ void kvm_mmu_zap_all(struct kvm *kvm)
>  	kvm_flush_remote_tlbs(kvm);
>  }
> +static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)
> +{
> +	struct kvm *kvm;
> +	struct kvm *kvm_freed = NULL;
> +	int cache_count = 0;
> +
> +	spin_lock(&kvm_lock);
> +	if (list_empty(&vm_list)) {
> +		spin_unlock(&kvm_lock);
> +		return 0;
> +	}
>   

Unnecessary, if the list is empty the loop below doesn't execute and we 
return 0 anyway.

> +	list_for_each_entry(kvm, &vm_list, vm_list) {
> +		int npages;
> +
> +		spin_lock(&kvm->mmu_lock);
> +		npages = kvm->arch.n_alloc_mmu_pages -
> +			 kvm->arch.n_free_mmu_pages;
> +		cache_count += npages - KVM_MIN_ALLOC_MMU_PAGES;
>   

I think we should allow the shrinker to go below 
KVM_MIN_ALLOC_MMU_PAGES; in particular, if the vm is inactive, we should 
be able to shrink it to nothing.

When the VM starts executing again, it will reallocate those pages.

> +		if (!kvm_freed && nr_to_scan > 0 && npages >
> +		    KVM_MIN_ALLOC_MMU_PAGES) {
>   

Don't split an expression like that; the tightly binding expression 
should stay on the same line:

if (!kvm_freed && nr_to_scan > 0 &&
     npages > KVM_MN_ALLOC_MMU_PAGES) {

> +			kvm_mmu_remove_one_alloc_mmu_page(kvm);
> +			cache_count--;
> +			kvm_freed = kvm;
> +		}
> +		nr_to_scan--;
> +
> +		spin_unlock(&kvm->mmu_lock);
> +	}
> +	if (kvm_freed) {
> +		list_del(&kvm_freed->vm_list);
> +		list_add_tail(&kvm_freed->vm_list, &vm_list);
> +	}
>   

list_move_tail()

> +	spin_unlock(&kvm_lock);
> +
> +	return cache_count;
> +}
> +
> +static struct shrinker mmu_shrinker = {
> +	.shrink = mmu_shrink,
> +	.seeks = DEFAULT_SEEKS * 10,
> +};
> +
>  void kvm_mmu_module_exit(void)
>  {
>  	if (pte_chain_cache)
> @@ -1980,6 +2031,7 @@ void kvm_mmu_module_exit(void)
>  		kmem_cache_destroy(rmap_desc_cache);
>  	if (mmu_page_header_cache)
>  		kmem_cache_destroy(mmu_page_header_cache);
>
>   

I believe it is necessary to register the shrinker in order to have any 
observable effect.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: KVM: register the kvm mmu cache with the shrinker.

[Izik Eidus]

Avi Kivity wrote:
> Izik Eidus wrote:
>> this is the shrinker patch with all comments applied beside adding
>> aging mechanism
>> it look like the aging mechanism is not really needed and therefor for
>> now it isn't
>> implemented.
>>
>> From 8503a57ae88ba819e4eac6371172772c98b485f0 Mon Sep 17 00:00:00 2001
>> From: Izik Eidus <izike@qumranet.com>
>> Date: Tue, 25 Mar 2008 14:03:27 +0200
>> Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.
>>
>> Signed-off-by: Izik Eidus <izike@qumranet.com>
>> ---
>>  arch/x86/kvm/mmu.c |   54
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 54 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index c67ec62..c42c0db 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -1972,6 +1972,57 @@ void kvm_mmu_zap_all(struct kvm *kvm)
>>      kvm_flush_remote_tlbs(kvm);
>>  }
>> +static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)
>> +{
>> +    struct kvm *kvm;
>> +    struct kvm *kvm_freed = NULL;
>> +    int cache_count = 0;
>> +
>> +    spin_lock(&kvm_lock);
>> +    if (list_empty(&vm_list)) {
>> +        spin_unlock(&kvm_lock);
>> +        return 0;
>> +    }
>>   
> 
> Unnecessary, if the list is empty the loop below doesn't execute and we
> return 0 anyway.
> 
>> +    list_for_each_entry(kvm, &vm_list, vm_list) {
>> +        int npages;
>> +
>> +        spin_lock(&kvm->mmu_lock);
>> +        npages = kvm->arch.n_alloc_mmu_pages -
>> +             kvm->arch.n_free_mmu_pages;
>> +        cache_count += npages - KVM_MIN_ALLOC_MMU_PAGES;
>>   
> 
> I think we should allow the shrinker to go below
> KVM_MIN_ALLOC_MMU_PAGES; in particular, if the vm is inactive, we should
> be able to shrink it to nothing.
> 
> When the VM starts executing again, it will reallocate those pages.
> 
>> +        if (!kvm_freed && nr_to_scan > 0 && npages >
>> +            KVM_MIN_ALLOC_MMU_PAGES) {
>>   
> 
> Don't split an expression like that; the tightly binding expression
> should stay on the same line:
> 
> if (!kvm_freed && nr_to_scan > 0 &&
>     npages > KVM_MN_ALLOC_MMU_PAGES) {
> 
>> +            kvm_mmu_remove_one_alloc_mmu_page(kvm);
>> +            cache_count--;
>> +            kvm_freed = kvm;
>> +        }
>> +        nr_to_scan--;
>> +
>> +        spin_unlock(&kvm->mmu_lock);
>> +    }
>> +    if (kvm_freed) {
>> +        list_del(&kvm_freed->vm_list);
>> +        list_add_tail(&kvm_freed->vm_list, &vm_list);
>> +    }
>>   
> 
> list_move_tail()
> 
>> +    spin_unlock(&kvm_lock);
>> +
>> +    return cache_count;
>> +}
>> +
>> +static struct shrinker mmu_shrinker = {
>> +    .shrink = mmu_shrink,
>> +    .seeks = DEFAULT_SEEKS * 10,
>> +};
>> +
>>  void kvm_mmu_module_exit(void)
>>  {
>>      if (pte_chain_cache)
>> @@ -1980,6 +2031,7 @@ void kvm_mmu_module_exit(void)
>>          kmem_cache_destroy(rmap_desc_cache);
>>      if (mmu_page_header_cache)
>>          kmem_cache_destroy(mmu_page_header_cache);
>>
>>   
> 
> I believe it is necessary to register the shrinker in order to have any
> observable effect.
> 
sorry, i didnt send the whole patch
here the one with your comments applied.
(btw, i have just saw something weird about the memory, so wait before you applies it)

>From 989c2f8373d8257e9c2b9a8c6ed8d629cd2a9d74 Mon Sep 17 00:00:00 2001
From: Izik Eidus <izike@qumranet.com>
Date: Tue, 25 Mar 2008 14:03:27 +0200
Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.

Signed-off-by: Izik Eidus <izike@qumranet.com>
---
 arch/x86/kvm/mmu.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index c67ec62..b6c1cb2 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1972,6 +1972,52 @@ void kvm_mmu_zap_all(struct kvm *kvm)
 	kvm_flush_remote_tlbs(kvm);
 }
 
+void kvm_mmu_remove_one_alloc_mmu_page(struct kvm *kvm)
+{
+	struct kvm_mmu_page *page;
+
+	page = container_of(kvm->arch.active_mmu_pages.prev,
+			    struct kvm_mmu_page, link);
+	kvm_mmu_zap_page(kvm, page);
+}
+
+static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)
+{
+	struct kvm *kvm;
+	struct kvm *kvm_freed = NULL;
+	int cache_count = 0;
+
+	spin_lock(&kvm_lock);
+
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		int npages;
+
+		spin_lock(&kvm->mmu_lock);
+		npages = kvm->arch.n_alloc_mmu_pages -
+			 kvm->arch.n_free_mmu_pages;
+		cache_count += npages;
+		if (!kvm_freed && nr_to_scan > 0 && npages > 0) {
+			kvm_mmu_remove_one_alloc_mmu_page(kvm);
+			cache_count--;
+			kvm_freed = kvm;
+		}
+		nr_to_scan--;
+
+		spin_unlock(&kvm->mmu_lock);
+	}
+	if (kvm_freed)
+		list_move_tail(&kvm_freed->vm_list, &vm_list);
+
+	spin_unlock(&kvm_lock);
+
+	return cache_count;
+}
+
+static struct shrinker mmu_shrinker = {
+	.shrink = mmu_shrink,
+	.seeks = DEFAULT_SEEKS * 10,
+};
+
 void kvm_mmu_module_exit(void)
 {
 	if (pte_chain_cache)
@@ -1980,6 +2026,7 @@ void kvm_mmu_module_exit(void)
 		kmem_cache_destroy(rmap_desc_cache);
 	if (mmu_page_header_cache)
 		kmem_cache_destroy(mmu_page_header_cache);
+	unregister_shrinker(&mmu_shrinker);
 }
 
 int kvm_mmu_module_init(void)
@@ -2001,6 +2048,8 @@ int kvm_mmu_module_init(void)
 	if (!mmu_page_header_cache)
 		goto nomem;
 
+	register_shrinker(&mmu_shrinker);
+
 	return 0;
 
 nomem:
-- 
1.5.3.6


-- 
woof.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: KVM: register the kvm mmu cache with the shrinker.

[Avi Kivity]

Izik Eidus wrote:
>  void kvm_mmu_module_exit(void)
>  {
>  	if (pte_chain_cache)
> @@ -1980,6 +2026,7 @@ void kvm_mmu_module_exit(void)
>  		kmem_cache_destroy(rmap_desc_cache);
>  	if (mmu_page_header_cache)
>  		kmem_cache_destroy(mmu_page_header_cache);
> +	unregister_shrinker(&mmu_shrinker);
>  }
>   

kvm_mmu_module_exit() can be called from the nomem: label below, so the 
shrinker will be unregistered despite not being registered.

Suggest renaming kvm_mmu_module_exit() to mmu_destroy_caches(), and 
adding a new kvm_mmu_module_exit() which will call mmu_destroy_caches() 
and unregister_shrinker().

>  
>  int kvm_mmu_module_init(void)
> @@ -2001,6 +2048,8 @@ int kvm_mmu_module_init(void)
>  	if (!mmu_page_header_cache)
>  		goto nomem;
>  
> +	register_shrinker(&mmu_shrinker);
> +
>  	return 0;
>  
>  nomem:
>   


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: KVM: register the kvm mmu cache with the shrinker.

[Izik Eidus]

Avi Kivity wrote:
> Izik Eidus wrote:
>>  void kvm_mmu_module_exit(void)
>>  {
>>      if (pte_chain_cache)
>> @@ -1980,6 +2026,7 @@ void kvm_mmu_module_exit(void)
>>          kmem_cache_destroy(rmap_desc_cache);
>>      if (mmu_page_header_cache)
>>          kmem_cache_destroy(mmu_page_header_cache);
>> +    unregister_shrinker(&mmu_shrinker);
>>  }
>>   
> 
> kvm_mmu_module_exit() can be called from the nomem: label below, so the
> shrinker will be unregistered despite not being registered.
> 
> Suggest renaming kvm_mmu_module_exit() to mmu_destroy_caches(), and
> adding a new kvm_mmu_module_exit() which will call mmu_destroy_caches()
> and unregister_shrinker().
> 
>>  
>>  int kvm_mmu_module_init(void)
>> @@ -2001,6 +2048,8 @@ int kvm_mmu_module_init(void)
>>      if (!mmu_page_header_cache)
>>          goto nomem;
>>  
>> +    register_shrinker(&mmu_shrinker);
>> +
>>      return 0;
>>  
>>  nomem:
>>   
> 
> 


ok, this should address all the issues.

>From a4cd289ab2502172bde994612636f84bec5e7a60 Mon Sep 17 00:00:00 2001
From: Izik Eidus <izike@qumranet.com>
Date: Sun, 30 Mar 2008 15:13:47 +0300
Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.

Signed-off-by: Izik Eidus <izike@qumranet.com>
---
 arch/x86/kvm/mmu.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index a0be4f5..8a6a4f9 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1963,7 +1963,53 @@ void kvm_mmu_zap_all(struct kvm *kvm)
 	kvm_flush_remote_tlbs(kvm);
 }
 
-void kvm_mmu_module_exit(void)
+void kvm_mmu_remove_one_alloc_mmu_page(struct kvm *kvm)
+{
+	struct kvm_mmu_page *page;
+
+	page = container_of(kvm->arch.active_mmu_pages.prev,
+			    struct kvm_mmu_page, link);
+	kvm_mmu_zap_page(kvm, page);
+}
+
+static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)
+{
+	struct kvm *kvm;
+	struct kvm *kvm_freed = NULL;
+	int cache_count = 0;
+
+	spin_lock(&kvm_lock);
+
+	list_for_each_entry(kvm, &vm_list, vm_list) {
+		int npages;
+
+		spin_lock(&kvm->mmu_lock);
+		npages = kvm->arch.n_alloc_mmu_pages -
+			 kvm->arch.n_free_mmu_pages;
+		cache_count += npages;
+		if (!kvm_freed && nr_to_scan > 0 && npages > 0) {
+			kvm_mmu_remove_one_alloc_mmu_page(kvm);
+			cache_count--;
+			kvm_freed = kvm;
+		}
+		nr_to_scan--;
+
+		spin_unlock(&kvm->mmu_lock);
+	}
+	if (kvm_freed)
+		list_move_tail(&kvm_freed->vm_list, &vm_list);
+
+	spin_unlock(&kvm_lock);
+
+	return cache_count;
+}
+
+static struct shrinker mmu_shrinker = {
+	.shrink = mmu_shrink,
+	.seeks = DEFAULT_SEEKS * 10,
+};
+
+void mmu_destroy_caches(void)
 {
 	if (pte_chain_cache)
 		kmem_cache_destroy(pte_chain_cache);
@@ -1973,6 +2019,12 @@ void kvm_mmu_module_exit(void)
 		kmem_cache_destroy(mmu_page_header_cache);
 }
 
+void kvm_mmu_module_exit(void)
+{
+	mmu_destroy_caches();
+	unregister_shrinker(&mmu_shrinker);
+}
+
 int kvm_mmu_module_init(void)
 {
 	pte_chain_cache = kmem_cache_create("kvm_pte_chain",
@@ -1992,10 +2044,12 @@ int kvm_mmu_module_init(void)
 	if (!mmu_page_header_cache)
 		goto nomem;
 
+	register_shrinker(&mmu_shrinker);
+
 	return 0;
 
 nomem:
-	kvm_mmu_module_exit();
+	mmu_destroy_caches();
 	return -ENOMEM;
 }
 
-- 
1.5.3.6


-- 
woof.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace

Re: KVM: register the kvm mmu cache with the shrinker.

[Avi Kivity]

Izik Eidus wrote:
> ok, this should address all the issues.
>
> >From a4cd289ab2502172bde994612636f84bec5e7a60 Mon Sep 17 00:00:00 2001
> From: Izik Eidus <izike@qumranet.com>
> Date: Sun, 30 Mar 2008 15:13:47 +0300
> Subject: [PATCH] KVM: register the kvm mmu cache with the shrinker.
>
>   

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace