Messages in this thread | | | Subject | Re: [PATCH 04/11] KVM: page track: add the framework of guest page tracking | From | Xiao Guangrong <> | Date | Tue, 15 Dec 2015 16:46:47 +0800 |
| |
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.
| |