Messages in this thread | | | Subject | Re: [PATCH v3 05/11] KVM: page track: introduce kvm_page_track_{add,remove}_page | From | Xiao Guangrong <> | Date | Tue, 23 Feb 2016 12:18:13 +0800 |
| |
On 02/19/2016 07:37 PM, Paolo Bonzini wrote: > > > On 14/02/2016 12:31, Xiao Guangrong wrote: >> + /* does tracking count wrap? */ >> + WARN_ON((count > 0) && (val + count < val)); > > This doesn't work, because "val + count" is an int.
val is 'unsigned short val' and count is 'short', so 'val + count' is not int...
> >> + /* the last tracker has already gone? */ >> + WARN_ON((count < 0) && (val < !count)); > > Also, here any underflow should warn. > > You can actually use the fact that val + count is an int like this: > > WARN_ON(val + count < 0 || val + count > USHRT_MAX) >
It looks nice, i will change the type of val to int to simplify the code.
> and also please return if the warning fires. >
Okay.
>> +void kvm_page_track_add_page(struct kvm *kvm, gfn_t gfn, >> + enum kvm_page_track_mode mode) >> +{ >> + struct kvm_memslots *slots; >> + struct kvm_memory_slot *slot; >> + int i; >> + >> + for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) { >> + slots = __kvm_memslots(kvm, i); >> + >> + slot = __gfn_to_memslot(slots, gfn); >> + if (!slot) >> + continue; >> + >> + spin_lock(&kvm->mmu_lock); >> + kvm_slot_page_track_add_page_nolock(kvm, slot, gfn, mode); >> + spin_unlock(&kvm->mmu_lock); >> + } >> +} > > I don't think it is right to walk all address spaces. The good news is
Then we can not track the page in SMM mode, but i think it is not a big problem as SMM is invisible to OS (it is expected to not hurting OS) and current shadow page in normal spaces can not reflect the changes happend in SMM either. So it is okay to only take normal space into account.
> that you're not using kvm_page_track_{add,remove}_page at all as far as > I can see, so you can just remove them.
kvm_page_track_{add,remove}_page, which hides the mmu specifics (e.g. slot, mmu-lock, etc.) and are exported as APIs for other users, are just the small wrappers of kvm_slot_page_track_{add,remove}_page_nolock and the nolock functions are used in the later patch.
If you think it is not a good time to export these APIs, i am okay to export _nolock functions only in the next version.
> > Also, when you will need it, I think it's better to move the > spin_lock/spin_unlock pair outside the for loop. With this change, > perhaps it's better to leave it to the caller completely---but I cannot > say until I see the caller.
I will remove page tracking in SMM address space, so this is no loop in the next version. ;)
> > In the meanwhile, please leave out _nolock from the other functions' name.
I just wanted to warn the user that these functions are not safe as they are not protected by mmu-lock. I will remove these hints if you dislike them.
| |