Re: [PATCH v6 5/5] KVM: selftests: Add selftests for dirty quota throttling

[Shivam Kumar <shivam.kumar1@nutanix.com>]

On 07/10/22 11:59 pm, Sean Christopherson wrote:
> On Thu, Sep 15, 2022, Shivam Kumar wrote:
>>   #endif /* SELFTEST_KVM_UTIL_BASE_H */
>> diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c
>> index 9889fe0d8919..4c7bd9807d0b 100644
>> --- a/tools/testing/selftests/kvm/lib/kvm_util.c
>> +++ b/tools/testing/selftests/kvm/lib/kvm_util.c
>> @@ -18,6 +18,7 @@
>>   #include <linux/kernel.h>
>>   
>>   #define KVM_UTIL_MIN_PFN	2
>> +#define PML_BUFFER_SIZE	512
> 
> ...
> 
>> +	/*
>> +	 * Due to PML, number of pages dirtied by the vcpu can exceed its dirty
>> +	 * quota by PML buffer size.
>> +	 */
> 
> Buffering for PML is very Intel centric, and even then it's not guaranteed.  In
> x86, PML can and should be detected programmatically:
> 
> bool kvm_is_pml_enabled(void)
> {
> 	return is_intel_cpu() && get_kvm_intel_param_bool("pml");
> }
> Yes, I was looking for something like this. Thanks.
> 
> Not sure if it's worth a generic arch hook to get the CPU buffer size, e.g. the
> test could do:
> 
I can't think of any usecase for a custom buffer as of now. We are 
working on a proposal for dynamically adjusting the PML buffer size in 
order to throttle effectively with dirty quota. A potential usecase.
> 
> 	/*
> 	 * Allow ??? pages of overrun, KVM is allowed to dirty multiple pages
> 	 * before exiting to userspace, e.g. when emulating an instruction that
> 	 * performs multiple memory accesses.
> 	 */
> 	uint64_t buffer = ???;
> 
> 	/*
> 	 * When Intel's Page-Modification Logging (PML) is enabled, the CPU may
> 	 * dirty up to 512 pages (number of entries in the PML buffer) without
> 	 * exiting, thus KVM may effectively dirty that many pages before
> 	 * enforcing the dirty quota.
> 	 */
> #ifdef __x86_64__
> 	if (kvm_is_pml_enabled(void)
> 		buffer = PML_BUFFER_SIZE;
> #endif
> 	
> 
>> +	TEST_ASSERT(count <= quota + PML_BUFFER_SIZE, "Invalid number of pages
> 
> Clarify _why_ the count is invalid.
In the worst case, the vcpu will be able to dirty 512 more pages than 
its dirty quota due to PML.
> 
>> +		dirtied: count=%"PRIu64", quota=%"PRIu64"\n", count, quota);
> 
> Don't split strings unless it's truly necessary.  This is one of those cases where
> running over the 80 char soft limit is preferable to wrapping.  And no need to use
> PRIu64, selftests are 64-bit only.  E.g.
Got it, thanks.
> 
> 	TEST_ASSERT(count <= (quota + buffer),
> 		    "KVM dirtied too many pages: count = %lx, quota = %lx, buffer = %lx",
> 		    count, quota, buffer);
> 
> 
>> +
>> +	TEST_ASSERT(count >= quota, "Dirty quota exit happened with quota yet to
>> +		be exhausted: count=%"PRIu64", quota=%"PRIu64"\n", count, quota);
> 
> Similar comments here.
> 
>> +
>> +	if (count > quota)
>> +		pr_info("Dirty quota exit with unequal quota and count:
>> +			count=%"PRIu64", quota=%"PRIu64"\n", count, quota);
> 
> Not sure I'd bother with this.  Realistically, is anyone ever going to be able to
> do anything useful with this information?  E.g. is this just going to be a "PML is
> enabled!" message?
I thought this information could help detect anomalies when PML is 
disabled. If PML is disabled, I am expecting that the exit would be case 
when the quota equals count and count wouldn't ever exceed quota.

Now, when I think of, I think I should move this statement inside an 
'if' block and make it too an assertion.
	if (!kvm_is_pml_enabled(void))
		TEST_ASSERT(count == quota, "Dirty quota exit with unequal quota and 
count.");

> 
>> +
>> +	run->dirty_quota = count + test_dirty_quota_increment;
>> +}
>> -- 
>> 2.22.3
>>