KVM MMU: make kvm_mmu_zap_page() return the number of zapped sp in total.

PATCH Applied Gui Jianfeng 3 participants 8 messages Apr 22, 2010 – May 5, 2010

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Gui Jianfeng

author

2010-04-22 09:34 UTC #0 lore raw

PATCH 1/3 KVM MMU: make kvm_mmu_zap_page() return the number of zapped sp in total.

Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.

Signed-off-by Gui Jianfeng <[email protected]>

---
 arch/x86/kvm/mmu.c |   62 +++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 45 insertions(+), 17 deletions(-)

arch/x86/kvm/mmu.c +45 ~~-17~~

index 7a17db1..3fb18dd 100644
@@ -1195,12 +1195,15 @@ static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	--kvm->stat.mmu_unsync;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp);
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted);
 
 static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 {
+	int self_deleted;
+
 	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
 		return 1;
 	}

@@ -1209,7 +1212,7 @@ static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	kvm_unlink_unsync_page(vcpu->kvm, sp);
 	if (vcpu->arch.mmu.sync_page(vcpu, sp)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
 		return 1;
 	}

@@ -1471,6 +1474,7 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 	int i, zapped = 0;
 	struct mmu_page_path parents;
 	struct kvm_mmu_pages pages;
+	int self_deleted;
 
 	if (parent->role.level == PT_PAGE_TABLE_LEVEL)
 		return 0;

@@ -1480,7 +1484,7 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 		struct kvm_mmu_page *sp;
 
 		for_each_sp(pages, sp, parents, i) {
-			kvm_mmu_zap_page(kvm, sp);
+			kvm_mmu_zap_page(kvm, sp, &self_deleted);
 			mmu_pages_clear_parents(&parents);
 			zapped++;
 		}

@@ -1490,10 +1494,12 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 	return zapped;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted)
 {
 	int ret;
 
+	*self_deleted = 0;
 	trace_kvm_mmu_zap_page(sp);
 	++kvm->stat.mmu_shadow_zapped;
 	ret = mmu_zap_unsync_children(kvm, sp);

@@ -1507,11 +1513,15 @@ static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	if (!sp->root_count) {
 		hlist_del(&sp->hash_link);
 		kvm_mmu_free_page(kvm, sp);
+		/* Count self */
+		ret++;
+		*self_deleted = 1;
 	} else {
 		sp->role.invalid = 1;
 		list_move(&sp->link, &kvm->arch.active_mmu_pages);
 		kvm_reload_remote_mmus(kvm);
 	}
+
 	kvm_mmu_reset_last_pte_updated(kvm);
 	return ret;
 }

@@ -1523,6 +1533,7 @@ static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages)
 {
 	int used_pages;
+	int self_deleted;
 
 	used_pages = kvm->arch.n_alloc_mmu_pages - kvm->arch.n_free_mmu_pages;
 	used_pages = max(0, used_pages);

@@ -1540,8 +1551,8 @@ void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages)
 
 			page = container_of(kvm->arch.active_mmu_pages.prev,
 					    struct kvm_mmu_page, link);
-			used_pages -= kvm_mmu_zap_page(kvm, page);
-			used_pages--;
+			used_pages -= kvm_mmu_zap_page(kvm, page,
+						       &self_deleted);
 		}
 		kvm_nr_mmu_pages = used_pages;
 		kvm->arch.n_free_mmu_pages = 0;

@@ -1560,6 +1571,8 @@ static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *n;
 	int r;
+	int self_deleted;
+	int ret;
 
 	pgprintk("%s: looking for gfn %lx\n", __func__, gfn);
 	r = 0;

@@ -1571,7 +1584,8 @@ restart:
 			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
 				 sp->role.word);
 			r = 1;
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	return r;

@@ -1583,6 +1597,8 @@ static void mmu_unshadow(struct kvm *kvm, gfn_t gfn)
 	struct hlist_head *bucket;
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *nn;
+	int ret;
+	int self_deleted;
 
 	index = kvm_page_table_hashfn(gfn);
 	bucket = &kvm->arch.mmu_page_hash[index];

@@ -1592,7 +1608,8 @@ restart:
 		    && !sp->role.invalid) {
 			pgprintk("%s: zap %lx %x\n",
 				 __func__, gfn, sp->role.word);
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	}

@@ -2018,6 +2035,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 {
 	int i;
 	struct kvm_mmu_page *sp;
+	int self_deleted;
 
 	if (!VALID_PAGE(vcpu->arch.mmu.root_hpa))
 		return;

@@ -2028,7 +2046,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 		sp = page_header(root);
 		--sp->root_count;
 		if (!sp->root_count && sp->role.invalid)
-			kvm_mmu_zap_page(vcpu->kvm, sp);
+			kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
 		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 		spin_unlock(&vcpu->kvm->mmu_lock);
 		return;

@@ -2041,7 +2059,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 			sp = page_header(root);
 			--sp->root_count;
 			if (!sp->root_count && sp->role.invalid)
-				kvm_mmu_zap_page(vcpu->kvm, sp);
+				kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
 		}
 		vcpu->arch.mmu.pae_root[i] = INVALID_PAGE;
 	}

@@ -2616,6 +2634,8 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	int npte;
 	int r;
 	int invlpg_counter;
+	int ret;
+	int self_deleted;
 
 	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);

@@ -2694,7 +2714,9 @@ restart:
 			 */
 			pgprintk("misaligned: gpa %llx bytes %d role %x\n",
 				 gpa, bytes, sp->role.word);
-			if (kvm_mmu_zap_page(vcpu->kvm, sp))
+
+			ret = kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 			++vcpu->kvm->stat.mmu_flooded;
 			continue;

@@ -2756,13 +2778,15 @@ EXPORT_SYMBOL_GPL(kvm_mmu_unprotect_page_virt);
 
 void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
 {
+	int self_deleted;
+
 	while (vcpu->kvm->arch.n_free_mmu_pages < KVM_REFILL_PAGES &&
 	       !list_empty(&vcpu->kvm->arch.active_mmu_pages)) {
 		struct kvm_mmu_page *sp;
 
 		sp = container_of(vcpu->kvm->arch.active_mmu_pages.prev,
 				  struct kvm_mmu_page, link);
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
 		++vcpu->kvm->stat.mmu_recycled;
 	}
 }

@@ -2902,13 +2926,16 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 void kvm_mmu_zap_all(struct kvm *kvm)
 {
 	struct kvm_mmu_page *sp, *node;
+	int ret;
+	int self_deleted;
 
 	spin_lock(&kvm->mmu_lock);
 restart:
-	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link)
-		if (kvm_mmu_zap_page(kvm, sp))
+	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
+		ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+		if (ret > 1 || (ret == 1 && self_deleted == 0))
 			goto restart;
-
+	}
 	spin_unlock(&kvm->mmu_lock);
 
 	kvm_flush_remote_tlbs(kvm);

@@ -2917,10 +2944,11 @@ restart:
 static void kvm_mmu_remove_one_alloc_mmu_page(struct kvm *kvm)
 {
 	struct kvm_mmu_page *page;
+	int self_deleted;
 
 	page = container_of(kvm->arch.active_mmu_pages.prev,
 			    struct kvm_mmu_page, link);
-	kvm_mmu_zap_page(kvm, page);
+	kvm_mmu_zap_page(kvm, page, &self_deleted);
 }
 
 static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)

Signature · 1 line

1.6.5.2

Xiao Guangrong

2010-04-23 03:52 UTC #1 lore raw

Quoted 5 lines · Gui Jianfeng

Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.

This bug only hurts kvm_mmu_change_mmu_pages() function, we'd better allow 'self_deleted' is NULL, then we can pass NULL at other place.

arch/x86/kvm/mmu.c restart: · line 1589

Quoted 8 lines

arch/x86/kvm/mmu.c +2 -1

@@ -1571,7 +1584,8 @@ restart:
 			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
 				 sp->role.word);
 			r = 1;
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;

Maybe we can keep kvm_mmu_zap_page() returns the number of zapped children, and 'self_deleted' indicates whether self is zapped, then we no need modify those function, just fix kvm_mmu_change_mmu_pages() that is if 'self_deleted == 1', inc 'used_pages'

Xiao

Gui Jianfeng

author

2010-04-23 05:05 UTC #2 lore raw

Quoted 10 lines · Xiao Guangrong

Quoted 5 lines · Gui Jianfeng
Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.
This bug only hurts kvm_mmu_change_mmu_pages() function, we'd better allow 'self_deleted' is NULL, then we can pass NULL at other place.

Ok, will change. Will send a updated version.

Quoted 14 lines

arch/x86/kvm/mmu.c restart: · line 1589
Quoted 9 lines
arch/x86/kvm/mmu.c +2 -1
@@ -1571,7 +1584,8 @@ restart:
 			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
 				 sp->role.word);
 			r = 1;
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
Maybe we can keep kvm_mmu_zap_page() returns the number of zapped children, and 'self_deleted' indicates whether self is zapped, then we no need modify those function, just fix kvm_mmu_change_mmu_pages() that is if 'self_deleted == 1', inc 'used_pages'

I think kvm_mmu_zap_page() returning the total zapped number is more intuitive, so i'd prefer to retain the original code. Thanks.

Gui,
Thanks

Quoted 5 lines

Xiao

Gui Jianfeng

author

2010-04-23 05:59 UTC #3 lore raw

PATCH v2 1/3 KVM MMU: make kvm_mmu_zap_page() return the number of zapped sp in total.

Signed-off-by Gui Jianfeng <[email protected]>

---
 arch/x86/kvm/mmu.c |   53 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 17 deletions(-)

arch/x86/kvm/mmu.c +36 ~~-17~~

index 7a17db1..d0960f1 100644
@@ -1195,12 +1195,13 @@ static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	--kvm->stat.mmu_unsync;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp);
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted);
 
 static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 {
 	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		return 1;
 	}

@@ -1209,7 +1210,7 @@ static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	kvm_unlink_unsync_page(vcpu->kvm, sp);
 	if (vcpu->arch.mmu.sync_page(vcpu, sp)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		return 1;
 	}

@@ -1480,7 +1481,7 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 		struct kvm_mmu_page *sp;
 
 		for_each_sp(pages, sp, parents, i) {
-			kvm_mmu_zap_page(kvm, sp);
+			kvm_mmu_zap_page(kvm, sp, NULL);
 			mmu_pages_clear_parents(&parents);
 			zapped++;
 		}

@@ -1490,7 +1491,8 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 	return zapped;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted)
 {
 	int ret;

@@ -1507,11 +1509,16 @@ static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	if (!sp->root_count) {
 		hlist_del(&sp->hash_link);
 		kvm_mmu_free_page(kvm, sp);
+		/* Count self */
+		ret++;
+		if (self_deleted)
+			*self_deleted = 1;
 	} else {
 		sp->role.invalid = 1;
 		list_move(&sp->link, &kvm->arch.active_mmu_pages);
 		kvm_reload_remote_mmus(kvm);
 	}
+
 	kvm_mmu_reset_last_pte_updated(kvm);
 	return ret;
 }

@@ -1540,8 +1547,7 @@ void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages)
 
 			page = container_of(kvm->arch.active_mmu_pages.prev,
 					    struct kvm_mmu_page, link);
-			used_pages -= kvm_mmu_zap_page(kvm, page);
-			used_pages--;
+			used_pages -= kvm_mmu_zap_page(kvm, page, NULL);
 		}
 		kvm_nr_mmu_pages = used_pages;
 		kvm->arch.n_free_mmu_pages = 0;

@@ -1560,6 +1566,8 @@ static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *n;
 	int r;
+	int self_deleted = 0;
+	int ret;
 
 	pgprintk("%s: looking for gfn %lx\n", __func__, gfn);
 	r = 0;

@@ -1571,7 +1579,8 @@ restart:
 			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
 				 sp->role.word);
 			r = 1;
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	return r;

@@ -1583,6 +1592,8 @@ static void mmu_unshadow(struct kvm *kvm, gfn_t gfn)
 	struct hlist_head *bucket;
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *nn;
+	int ret;
+	int self_deleted = 0;
 
 	index = kvm_page_table_hashfn(gfn);
 	bucket = &kvm->arch.mmu_page_hash[index];

@@ -1592,7 +1603,8 @@ restart:
 		    && !sp->role.invalid) {
 			pgprintk("%s: zap %lx %x\n",
 				 __func__, gfn, sp->role.word);
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	}

@@ -2028,7 +2040,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 		sp = page_header(root);
 		--sp->root_count;
 		if (!sp->root_count && sp->role.invalid)
-			kvm_mmu_zap_page(vcpu->kvm, sp);
+			kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 		spin_unlock(&vcpu->kvm->mmu_lock);
 		return;

@@ -2041,7 +2053,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 			sp = page_header(root);
 			--sp->root_count;
 			if (!sp->root_count && sp->role.invalid)
-				kvm_mmu_zap_page(vcpu->kvm, sp);
+				kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		}
 		vcpu->arch.mmu.pae_root[i] = INVALID_PAGE;
 	}

@@ -2616,6 +2628,8 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	int npte;
 	int r;
 	int invlpg_counter;
+	int ret;
+	int self_deleted = 0;
 
 	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);

@@ -2694,7 +2708,9 @@ restart:
 			 */
 			pgprintk("misaligned: gpa %llx bytes %d role %x\n",
 				 gpa, bytes, sp->role.word);
-			if (kvm_mmu_zap_page(vcpu->kvm, sp))
+
+			ret = kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 			++vcpu->kvm->stat.mmu_flooded;
 			continue;

@@ -2762,7 +2778,7 @@ void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
 
 		sp = container_of(vcpu->kvm->arch.active_mmu_pages.prev,
 				  struct kvm_mmu_page, link);
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		++vcpu->kvm->stat.mmu_recycled;
 	}
 }

@@ -2902,13 +2918,16 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 void kvm_mmu_zap_all(struct kvm *kvm)
 {
 	struct kvm_mmu_page *sp, *node;
+	int ret;
+	int self_deleted = 0;
 
 	spin_lock(&kvm->mmu_lock);
 restart:
-	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link)
-		if (kvm_mmu_zap_page(kvm, sp))
+	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
+		ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+		if (ret > 1 || (ret == 1 && self_deleted == 0))
 			goto restart;
-
+	}
 	spin_unlock(&kvm->mmu_lock);
 
 	kvm_flush_remote_tlbs(kvm);

@@ -2920,7 +2939,7 @@ static void kvm_mmu_remove_one_alloc_mmu_page(struct kvm *kvm)
 
 	page = container_of(kvm->arch.active_mmu_pages.prev,
 			    struct kvm_mmu_page, link);
-	kvm_mmu_zap_page(kvm, page);
+	kvm_mmu_zap_page(kvm, page, NULL);
 }
 
 static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)

Signature · 1 line

1.6.5.2

Marcelo Tosatti

2010-04-26 17:36 UTC #4 lore raw

Quoted 10 lines · Gui Jianfeng

Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.
Signed-off-by Gui Jianfeng <[email protected]>
---
 arch/x86/kvm/mmu.c |   53 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 17 deletions(-)

Gui,

There will be only a few pinned roots, and there is no need for kvm_mmu_change_mmu_pages to be precise at that level (pages will be reclaimed through kvm_unmap_hva eventually).

Gui Jianfeng

author

2010-05-03 13:40 UTC #5 lore raw

Quoted 12 lines · Marcelo Tosatti

Quoted 10 lines · Gui Jianfeng
Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed.
Signed-off-by Gui Jianfeng <[email protected]>
---
 arch/x86/kvm/mmu.c |   53 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 17 deletions(-)

Quoted 5 lines

Gui,
There will be only a few pinned roots, and there is no need for kvm_mmu_change_mmu_pages to be precise at that level (pages will be reclaimed through kvm_unmap_hva eventually).

Hi Marcelo

Actually, it doesn't only affect kvm_mmu_change_mmu_pages() but also affects kvm_mmu_remove_some_alloc_mmu_pages() which is called by mmu shrink routine. This will induce upper layer get a wrong number, so i think this should be fixed. Here is a updated version.

---
From: Gui Jianfeng <[email protected]>

Signed-off-by Gui Jianfeng <[email protected]>

---
 arch/x86/kvm/mmu.c |   53 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 36 insertions(+), 17 deletions(-)

arch/x86/kvm/mmu.c +36 ~~-17~~

index 51eb6d6..e545da8 100644
@@ -1194,12 +1194,13 @@ static void kvm_unlink_unsync_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	--kvm->stat.mmu_unsync;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp);
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted);
 
 static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 {
 	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		return 1;
 	}

@@ -1207,7 +1208,7 @@ static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
 		kvm_flush_remote_tlbs(vcpu->kvm);
 	kvm_unlink_unsync_page(vcpu->kvm, sp);
 	if (vcpu->arch.mmu.sync_page(vcpu, sp)) {
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		return 1;
 	}

@@ -1478,7 +1479,7 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 		struct kvm_mmu_page *sp;
 
 		for_each_sp(pages, sp, parents, i) {
-			kvm_mmu_zap_page(kvm, sp);
+			kvm_mmu_zap_page(kvm, sp, NULL);
 			mmu_pages_clear_parents(&parents);
 			zapped++;
 		}

@@ -1488,7 +1489,8 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 	return zapped;
 }
 
-static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
+static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp,
+			    int *self_deleted)
 {
 	int ret;

@@ -1505,11 +1507,16 @@ static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
 	if (!sp->root_count) {
 		hlist_del(&sp->hash_link);
 		kvm_mmu_free_page(kvm, sp);
+		/* Count self */
+		ret++;
+		if (self_deleted)
+			*self_deleted = 1;
 	} else {
 		sp->role.invalid = 1;
 		list_move(&sp->link, &kvm->arch.active_mmu_pages);
 		kvm_reload_remote_mmus(kvm);
 	}
+
 	kvm_mmu_reset_last_pte_updated(kvm);
 	return ret;
 }

@@ -1538,8 +1545,7 @@ void kvm_mmu_change_mmu_pages(struct kvm *kvm, unsigned int kvm_nr_mmu_pages)
 
 			page = container_of(kvm->arch.active_mmu_pages.prev,
 					    struct kvm_mmu_page, link);
-			used_pages -= kvm_mmu_zap_page(kvm, page);
-			used_pages--;
+			used_pages -= kvm_mmu_zap_page(kvm, page, NULL);
 		}
 		kvm_nr_mmu_pages = used_pages;
 		kvm->arch.n_free_mmu_pages = 0;

@@ -1558,6 +1564,8 @@ static int kvm_mmu_unprotect_page(struct kvm *kvm, gfn_t gfn)
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *n;
 	int r;
+	int self_deleted = 0;
+	int ret;
 
 	pgprintk("%s: looking for gfn %lx\n", __func__, gfn);
 	r = 0;

@@ -1569,7 +1577,8 @@ restart:
 			pgprintk("%s: gfn %lx role %x\n", __func__, gfn,
 				 sp->role.word);
 			r = 1;
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	return r;

@@ -1581,6 +1590,8 @@ static void mmu_unshadow(struct kvm *kvm, gfn_t gfn)
 	struct hlist_head *bucket;
 	struct kvm_mmu_page *sp;
 	struct hlist_node *node, *nn;
+	int ret;
+	int self_deleted = 0;
 
 	index = kvm_page_table_hashfn(gfn);
 	bucket = &kvm->arch.mmu_page_hash[index];

@@ -1590,7 +1601,8 @@ restart:
 		    && !sp->role.invalid) {
 			pgprintk("%s: zap %lx %x\n",
 				 __func__, gfn, sp->role.word);
-			if (kvm_mmu_zap_page(kvm, sp))
+			ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 		}
 	}

@@ -2012,7 +2024,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 		sp = page_header(root);
 		--sp->root_count;
 		if (!sp->root_count && sp->role.invalid)
-			kvm_mmu_zap_page(vcpu->kvm, sp);
+			kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		vcpu->arch.mmu.root_hpa = INVALID_PAGE;
 		spin_unlock(&vcpu->kvm->mmu_lock);
 		return;

@@ -2025,7 +2037,7 @@ static void mmu_free_roots(struct kvm_vcpu *vcpu)
 			sp = page_header(root);
 			--sp->root_count;
 			if (!sp->root_count && sp->role.invalid)
-				kvm_mmu_zap_page(vcpu->kvm, sp);
+				kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		}
 		vcpu->arch.mmu.pae_root[i] = INVALID_PAGE;
 	}

@@ -2604,6 +2616,8 @@ void kvm_mmu_pte_write(struct kvm_vcpu *vcpu, gpa_t gpa,
 	int npte;
 	int r;
 	int invlpg_counter;
+	int ret;
+	int self_deleted = 0;
 
 	pgprintk("%s: gpa %llx bytes %d\n", __func__, gpa, bytes);

@@ -2682,7 +2696,9 @@ restart:
 			 */
 			pgprintk("misaligned: gpa %llx bytes %d role %x\n",
 				 gpa, bytes, sp->role.word);
-			if (kvm_mmu_zap_page(vcpu->kvm, sp))
+
+			ret = kvm_mmu_zap_page(vcpu->kvm, sp, &self_deleted);
+			if (ret > 1 || (ret == 1 && self_deleted == 0))
 				goto restart;
 			++vcpu->kvm->stat.mmu_flooded;
 			continue;

@@ -2750,7 +2766,7 @@ void __kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu)
 
 		sp = container_of(vcpu->kvm->arch.active_mmu_pages.prev,
 				  struct kvm_mmu_page, link);
-		kvm_mmu_zap_page(vcpu->kvm, sp);
+		kvm_mmu_zap_page(vcpu->kvm, sp, NULL);
 		++vcpu->kvm->stat.mmu_recycled;
 	}
 }

@@ -2890,13 +2906,16 @@ void kvm_mmu_slot_remove_write_access(struct kvm *kvm, int slot)
 void kvm_mmu_zap_all(struct kvm *kvm)
 {
 	struct kvm_mmu_page *sp, *node;
+	int ret;
+	int self_deleted = 0;
 
 	spin_lock(&kvm->mmu_lock);
 restart:
-	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link)
-		if (kvm_mmu_zap_page(kvm, sp))
+	list_for_each_entry_safe(sp, node, &kvm->arch.active_mmu_pages, link) {
+		ret = kvm_mmu_zap_page(kvm, sp, &self_deleted);
+		if (ret > 1 || (ret == 1 && self_deleted == 0))
 			goto restart;
-
+	}
 	spin_unlock(&kvm->mmu_lock);
 
 	kvm_flush_remote_tlbs(kvm);

@@ -2908,7 +2927,7 @@ static int kvm_mmu_remove_some_alloc_mmu_pages(struct kvm *kvm)
 
 	page = container_of(kvm->arch.active_mmu_pages.prev,
 			    struct kvm_mmu_page, link);
-	return kvm_mmu_zap_page(kvm, page) + 1;
+	return kvm_mmu_zap_page(kvm, page, NULL);
 }
 
 static int mmu_shrink(int nr_to_scan, gfp_t gfp_mask)

Signature · 1 line

1.6.5.2

Marcelo Tosatti

2010-05-04 13:25 UTC #6 lore raw

Quoted 9 lines · Gui Jianfeng

Hi Marcelo
Actually, it doesn't only affect kvm_mmu_change_mmu_pages() but also affects kvm_mmu_remove_some_alloc_mmu_pages() which is called by mmu shrink routine. This will induce upper layer get a wrong number, so i think this should be fixed. Here is a updated version.
---
From: Gui Jianfeng <[email protected]>

Quoted 6 lines

Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed. kvm_mmu_remove_some_alloc_mmu_pages() also rely on kvm_mmu_zap_page() to return a total relcaimed number.

Isnt it simpler to have kvm_mmu_zap_page return the number of pages it actually freed? Then always restart the hash walk if return is positive.

Gui Jianfeng

author

2010-05-05 00:20 UTC #7 lore raw

Quoted 17 lines · Marcelo Tosatti

Quoted 9 lines · Gui Jianfeng
Hi Marcelo
Actually, it doesn't only affect kvm_mmu_change_mmu_pages() but also affects kvm_mmu_remove_some_alloc_mmu_pages() which is called by mmu shrink routine. This will induce upper layer get a wrong number, so i think this should be fixed. Here is a updated version.
---
From: Gui Jianfeng <[email protected]>
Quoted 6 lines
Currently, in kvm_mmu_change_mmu_pages(kvm, page), "used_pages--" is performed after calling kvm_mmu_zap_page() in spite of that whether "page" is actually reclaimed. Because root sp won't be reclaimed by kvm_mmu_zap_page(). So making kvm_mmu_zap_page() return total number of reclaimed sp makes more sense. A new flag is put into kvm_mmu_zap_page() to indicate whether the top page is reclaimed. kvm_mmu_remove_some_alloc_mmu_pages() also rely on kvm_mmu_zap_page() to return a total relcaimed number.

Quoted 3 lines

Isnt it simpler to have kvm_mmu_zap_page return the number of pages it actually freed? Then always restart the hash walk if return is positive.

OK, although in some cases we might encounter unneeded hash walk restart, but it's not a big problem. I don't object this solution, will post a new patch.

Thanks,
Gui

Quoted 3 lines