Re: Subject: [RFC PATCH] kvm/x86: Fix 'lpages' kvm stat for TDM MMU

From: Paolo Bonzini <pbonzini@redhat.com>
To: Sean Christopherson <seanjc@google.com>,
	"Shahin, Md Shahadat Hossain" <shahinmd@amazon.de>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Szczepanek, Bartosz" <bsz@amazon.de>,
	"bgardon@google.com" <bgardon@google.com>
Subject: Re: Subject: [RFC PATCH] kvm/x86: Fix 'lpages' kvm stat for TDM MMU
Date: Thu, 29 Apr 2021 19:04:07 +0200	[thread overview]
Message-ID: <3bdf46db-bd64-0ca7-039a-9c123f5a40f9@redhat.com> (raw)
In-Reply-To: <YIrjiXja3/5e6frs@google.com>

On 29/04/21 18:49, Sean Christopherson wrote:
> On Thu, Apr 29, 2021, Shahin, Md Shahadat Hossain wrote:
>> Large pages not being created properly may result in increased memory
>> access time. The 'lpages' kvm stat used to keep track of the current
>> number of large pages in the system, but with TDP MMU enabled the stat
>> is not showing the correct number.
>>
>> This patch extends the lpages counter to cover the TDP case.
>>
>> Signed-off-by: Md Shahadat Hossain Shahin <shahinmd@amazon.de>
>> Cc: Bartosz Szczepanek <bsz@amazon.de>
>> ---
>>   arch/x86/kvm/mmu/tdp_mmu.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
>> index 34207b874886..1e2a3cb33568 100644
>> --- a/arch/x86/kvm/mmu/tdp_mmu.c
>> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
>> @@ -425,6 +425,12 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
>>   
>>   	if (old_spte == new_spte)
>>   		return;
>> +	
>> +	if (is_large_pte(old_spte))
>> +		--kvm->stat.lpages;
>> +	
>> +	if (is_large_pte(new_spte))
>> +		++kvm->stat.lpages;
> 
> Hrm, kvm->stat.lpages could get corrupted when __handle_changed_spte() is called
> under read lock, e.g. if multiple vCPUs are faulting in memory.

Ouch, indeed!

One way to fix it without needing an atomic operation is to make it a 
per-vcpu stat.  It would be a bit weird for the binary stats because we 
would have to hide this one from the vCPU statistics file descriptor 
(and only aggregate it in the VM statistics).

Alternatively, you can do the atomic_add only if is_large_pte(old_spte) 
!= is_large_pte(new_spte), casting &kvm->stat.lpages to an atomic64_t*.

Paolo