Re: [PATCH 04/11] KVM: page track: add the framework of guest page tracking

From: Xiao Guangrong
Date: Tue Dec 15 2015 - 03:53:40 EST




On 12/15/2015 03:06 PM, Kai Huang wrote:
Hi Guangrong,

I am starting to review this series, and should have some comments or questions, you can determine
whether they are valuable :)

Thank you very much for your review and breaking the silent on this patchset. ;)


+static void page_track_slot_free(struct kvm_memory_slot *slot)
+{
+ int i;
+
+ for (i = 0; i < KVM_PAGE_TRACK_MAX; i++)
+ if (slot->arch.gfn_track[i]) {
+ kvfree(slot->arch.gfn_track[i]);
+ slot->arch.gfn_track[i] = NULL;
+ }
+}
+
+int kvm_page_track_create_memslot(struct kvm_memory_slot *slot,
+ unsigned long npages)
+{
+ int i, pages = gfn_to_index(slot->base_gfn + npages - 1,
+ slot->base_gfn, PT_PAGE_TABLE_LEVEL) + 1;
+
+ for (i = 0; i < KVM_PAGE_TRACK_MAX; i++) {
+ slot->arch.gfn_track[i] = kvm_kvzalloc(pages *
+ sizeof(*slot->arch.gfn_track[i]));
+ if (!slot->arch.gfn_track[i])
+ goto track_free;
+ }
+
+ return 0;
+
+track_free:
+ page_track_slot_free(slot);
+ return -ENOMEM;
+}
Is it necessary to use the 'unsigned long npages' pareameter? In my understanding you are going to

The type, 'int', is used here as I followed the style of 'struct kvm_lpage_info'.

4 bytes should be enough to track all users and signed type is good to track
overflow.

track all GFNs in the memory slot anyway, right? If you want to keep npages, I think it's better to
add a base_gfn as well otherwise you are assuming you are going to track the npages GFN starting
from slot->base_gfn.

Yes, any page in the memslot may be tracked so that there is a index for every
page.


+
+void kvm_page_track_free_memslot(struct kvm_memory_slot *free,
+ struct kvm_memory_slot *dont)
+{
+ if (!dont || free->arch.gfn_track != dont->arch.gfn_track)
+ page_track_slot_free(free);
+}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c04987e..ad4888a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7838,6 +7838,8 @@ void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *free,
free->arch.lpage_info[i - 1] = NULL;
}
}
+
+ kvm_page_track_free_memslot(free, dont);
}
int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
@@ -7886,6 +7888,9 @@ int kvm_arch_create_memslot(struct kvm *kvm, struct kvm_memory_slot *slot,
}
}
+ if (kvm_page_track_create_memslot(slot, npages))
+ goto out_free;
+
Looks essentially you are allocating one int for all GFNs of the slot unconditionally. In my
understanding for most of memory slots, we are not going to track them, so isn't it going to be
wasteful of memory?


Yes, hmm... maybe we can make the index as "unsigned short" then 1G memory only needs 512k index
buffer. It is not so unacceptable.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/