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

[Sean Christopherson <seanjc@google.com>]

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");
}


Not sure if it's worth a generic arch hook to get the CPU buffer size, e.g. the
test could do:


	/*
	 * 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.

> +		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.

	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?

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