| Message ID | 20180926024239.26048-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: remove check on nr_mmu_pages in kvm_arch_commit_memory_region() | expand |
On Wed, Sep 26, 2018 at 10:42:39AM +0800, kvm-owner@vger.kernel.org wrote: > * nr_mmu_pages would be non-zero only if kvm->arch.n_requested_mmu_pages is > non-zero. > > * nr_mmu_pages is always non-zero, since kvm_mmu_calculate_mmu_pages() > never return zero. > > Based on these two reasons, we can merge the two *if* clause and use the > return value from kvm_mmu_calculate_mmu_pages() directly. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > --- Might be worth adding a blurb to the changelog to note that this also eliminates code that might lead a reader to incorrectly believe it's possible for nr_mmu_pages to be zero. Another idea would be to rename kvm_mmu_calculate_mmu_pages() to kvm_mmu_calculate_default_mmu_pages() to help communicate that it's designed to always return a sane value and that it's only intended to be employed when userspace hasn't explicitly set the number of pages. Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
On Wed, Sep 26, 2018 at 06:42:12AM -0700, Sean Christopherson wrote: >On Wed, Sep 26, 2018 at 10:42:39AM +0800, kvm-owner@vger.kernel.org wrote: >> * nr_mmu_pages would be non-zero only if kvm->arch.n_requested_mmu_pages is >> non-zero. >> >> * nr_mmu_pages is always non-zero, since kvm_mmu_calculate_mmu_pages() >> never return zero. >> >> Based on these two reasons, we can merge the two *if* clause and use the >> return value from kvm_mmu_calculate_mmu_pages() directly. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> --- > Hi, Sean, Thanks for your comment. >Might be worth adding a blurb to the changelog to note that this also >eliminates code that might lead a reader to incorrectly believe it's >possible for nr_mmu_pages to be zero. Yep, I had this illusion when I read the code. > >Another idea would be to rename kvm_mmu_calculate_mmu_pages() to >kvm_mmu_calculate_default_mmu_pages() to help communicate that it's >designed to always return a sane value and that it's only intended to >be employed when userspace hasn't explicitly set the number of pages. > Will dress these two comment in v2. >Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com>
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 8614199d733b..7efd0ec7abd3 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -9131,13 +9131,9 @@ void kvm_arch_commit_memory_region(struct kvm *kvm, const struct kvm_memory_slot *new, enum kvm_mr_change change) { - int nr_mmu_pages = 0; - if (!kvm->arch.n_requested_mmu_pages) - nr_mmu_pages = kvm_mmu_calculate_mmu_pages(kvm); - - if (nr_mmu_pages) - kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages); + kvm_mmu_change_mmu_pages(kvm, + kvm_mmu_calculate_mmu_pages(kvm)); /* * Dirty logging tracks sptes in 4k granularity, meaning that large
* nr_mmu_pages would be non-zero only if kvm->arch.n_requested_mmu_pages is non-zero. * nr_mmu_pages is always non-zero, since kvm_mmu_calculate_mmu_pages() never return zero. Based on these two reasons, we can merge the two *if* clause and use the return value from kvm_mmu_calculate_mmu_pages() directly. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> --- arch/x86/kvm/x86.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)