Re: [PATCH 2/2] KVM: selftests: add a memslot-related performance benchmark

[Andrew Jones]

On Mon, Feb 01, 2021 at 09:10:57AM +0100, Maciej S. Szmigiero wrote:
[...]
> diff --git a/tools/testing/selftests/kvm/.gitignore b/tools/testing/selftests/kvm/.gitignore
> index ce8f4ad39684..059a655053ca 100644
> --- a/tools/testing/selftests/kvm/.gitignore
> +++ b/tools/testing/selftests/kvm/.gitignore
> @@ -31,3 +31,4 @@
>  /kvm_create_max_vcpus
>  /set_memory_region_test
>  /steal_time
> +/memslot_perf_test
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index e7c6237d7383..2abc9e182c30 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -65,6 +65,7 @@ TEST_GEN_PROGS_x86_64 += dirty_log_perf_test
>  TEST_GEN_PROGS_x86_64 += kvm_create_max_vcpus
>  TEST_GEN_PROGS_x86_64 += set_memory_region_test
>  TEST_GEN_PROGS_x86_64 += steal_time
> +TEST_GEN_PROGS_x86_64 += memslot_perf_test
> 
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list
>  TEST_GEN_PROGS_aarch64 += aarch64/get-reg-list-sve

We've been trying keep the lists in .gitignore and Makefile in alphabetic
order. It's not really important, but seems like we should keep it now
that we've got it. Well, except I see dirty_log_perf_test and
dirty_log_test are out of order already...

[...]
> +static bool prepare_vm(struct vm_data *data, int nslots, uint64_t *maxslots,
> +		       void *guest_code, uint64_t mempages,
> +		       struct timespec *slot_runtime)
> +{
> +	uint32_t max_mem_slots;
> +	uint64_t rempages;
> +	uint64_t guest_addr;
> +	uint32_t slot;
> +	struct timespec tstart;
> +	struct sync_area *sync;
> +
> +	max_mem_slots = kvm_check_cap(KVM_CAP_NR_MEMSLOTS);
> +	TEST_ASSERT(max_mem_slots > 1,
> +		    "KVM_CAP_NR_MEMSLOTS should be greater than 1");
> +	TEST_ASSERT(nslots > 1 || nslots == -1,
> +		    "Slot count cap should be greater than 1");
> +	if (nslots != -1)
> +		max_mem_slots = min(max_mem_slots, (uint32_t)nslots);
> +	pr_info_v("Allowed number of memory slots: %"PRIu32"\n", max_mem_slots);
> +
> +	TEST_ASSERT(mempages > 1,
> +		    "Can't test without any memory");
> +
> +	data->npages = mempages;
> +	data->nslots = max_mem_slots - 1;
> +	data->pages_per_slot = mempages / data->nslots;
> +	if (!data->pages_per_slot) {
> +		*maxslots = mempages + 1;
> +		return false;
> +	}
> +
> +	rempages = mempages % data->nslots;
> +	data->hva_slots = malloc(sizeof(*data->hva_slots) * data->nslots);
> +	TEST_ASSERT(data->hva_slots, "malloc() fail");
> +
> +	data->vm = vm_create_default(VCPU_ID, mempages, guest_code);
> +
> +	vcpu_set_cpuid(data->vm, VCPU_ID, kvm_get_supported_cpuid());

This vcpu_set_cpuid() call, which causes problems for non-x86 builds,
is now embedded in vm_create_default() and therefore redundant here.


Otherwise this looks good to me. I'll try to find some time to test
it on an AArch64 machine configured to use 4k pages on the host.

Reviewed-by: Andrew Jones <drjones@xxxxxxxxxx>

Thanks,
drew