Re: [PATCH] KVM: selftests: Ensure all migrations are performed when test is affined

[Paolo Bonzini]

On 30/09/21 01:41, Sean Christopherson wrote:
> Rework the CPU selection in the migration worker to ensure the specified
> number of migrations are performed when the test iteslf is affined to a
> subset of CPUs.  The existing logic skips iterations if the target CPU is
> not in the original set of possible CPUs, which causes the test to fail
> if too many iterations are skipped.
>
>    ==== Test Assertion Failure ====
>    rseq_test.c:228: i > (NR_TASK_MIGRATIONS / 2)
>    pid=10127 tid=10127 errno=4 - Interrupted system call
>       1  0x00000000004018e5: main at rseq_test.c:227
>       2  0x00007fcc8fc66bf6: ?? ??:0
>       3  0x0000000000401959: _start at ??:?
>    Only performed 4 KVM_RUNs, task stalled too much?
>
> Calculate the min/max possible CPUs as a cheap "best effort" to avoid
> high runtimes when the test is affined to a small percentage of CPUs.
> Alternatively, a list or xarray of the possible CPUs could be used, but
> even in a horrendously inefficient setup, such optimizations are not
> needed because the runtime is completely dominated by the cost of
> migrating the task, and the absolute runtime is well under a minute in
> even truly absurd setups, e.g. running on a subset of vCPUs in a VM that
> is heavily overcommited (16 vCPUs per pCPU).
>
> Fixes: 61e52f1630f5 ("KVM: selftests: Add a test for KVM_RUN+rseq to detect task migration bugs")
> Reported-by: Dongli Zhang <dongli.zhang@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>   tools/testing/selftests/kvm/rseq_test.c | 69 +++++++++++++++++++++----
>   1 file changed, 59 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c
> index c5e0dd664a7b..4158da0da2bb 100644
> --- a/tools/testing/selftests/kvm/rseq_test.c
> +++ b/tools/testing/selftests/kvm/rseq_test.c
> @@ -10,6 +10,7 @@
>   #include <signal.h>
>   #include <syscall.h>
>   #include <sys/ioctl.h>
> +#include <sys/sysinfo.h>
>   #include <asm/barrier.h>
>   #include <linux/atomic.h>
>   #include <linux/rseq.h>
> @@ -39,6 +40,7 @@ static __thread volatile struct rseq __rseq = {
>   
>   static pthread_t migration_thread;
>   static cpu_set_t possible_mask;
> +static int min_cpu, max_cpu;
>   static bool done;
>   
>   static atomic_t seq_cnt;
> @@ -57,20 +59,37 @@ static void sys_rseq(int flags)
>   	TEST_ASSERT(!r, "rseq failed, errno = %d (%s)", errno, strerror(errno));
>   }
>   
> +static int next_cpu(int cpu)
> +{
> +	/*
> +	 * Advance to the next CPU, skipping those that weren't in the original
> +	 * affinity set.  Sadly, there is no CPU_SET_FOR_EACH, and cpu_set_t's
> +	 * data storage is considered as opaque.  Note, if this task is pinned
> +	 * to a small set of discontigous CPUs, e.g. 2 and 1023, this loop will
> +	 * burn a lot cycles and the test will take longer than normal to
> +	 * complete.
> +	 */
> +	do {
> +		cpu++;
> +		if (cpu > max_cpu) {
> +			cpu = min_cpu;
> +			TEST_ASSERT(CPU_ISSET(cpu, &possible_mask),
> +				    "Min CPU = %d must always be usable", cpu);
> +			break;
> +		}
> +	} while (!CPU_ISSET(cpu, &possible_mask));
> +
> +	return cpu;
> +}
> +
>   static void *migration_worker(void *ign)
>   {
>   	cpu_set_t allowed_mask;
> -	int r, i, nr_cpus, cpu;
> +	int r, i, cpu;
>   
>   	CPU_ZERO(&allowed_mask);
>   
> -	nr_cpus = CPU_COUNT(&possible_mask);
> -
> -	for (i = 0; i < NR_TASK_MIGRATIONS; i++) {
> -		cpu = i % nr_cpus;
> -		if (!CPU_ISSET(cpu, &possible_mask))
> -			continue;
> -
> +	for (i = 0, cpu = min_cpu; i < NR_TASK_MIGRATIONS; i++, cpu = next_cpu(cpu)) {
>   		CPU_SET(cpu, &allowed_mask);
>   
>   		/*
> @@ -154,6 +173,36 @@ static void *migration_worker(void *ign)
>   	return NULL;
>   }
>   
> +static int calc_min_max_cpu(void)
> +{
> +	int i, cnt, nproc;
> +
> +	if (CPU_COUNT(&possible_mask) < 2)
> +		return -EINVAL;
> +
> +	/*
> +	 * CPU_SET doesn't provide a FOR_EACH helper, get the min/max CPU that
> +	 * this task is affined to in order to reduce the time spent querying
> +	 * unusable CPUs, e.g. if this task is pinned to a small percentage of
> +	 * total CPUs.
> +	 */
> +	nproc = get_nprocs_conf();
> +	min_cpu = -1;
> +	max_cpu = -1;
> +	cnt = 0;
> +
> +	for (i = 0; i < nproc; i++) {
> +		if (!CPU_ISSET(i, &possible_mask))
> +			continue;
> +		if (min_cpu == -1)
> +			min_cpu = i;
> +		max_cpu = i;
> +		cnt++;
> +	}
> +
> +	return (cnt < 2) ? -EINVAL : 0;
> +}
> +
>   int main(int argc, char *argv[])
>   {
>   	int r, i, snapshot;
> @@ -167,8 +216,8 @@ int main(int argc, char *argv[])
>   	TEST_ASSERT(!r, "sched_getaffinity failed, errno = %d (%s)", errno,
>   		    strerror(errno));
>   
> -	if (CPU_COUNT(&possible_mask) < 2) {
> -		print_skip("Only one CPU, task migration not possible\n");
> +	if (calc_min_max_cpu()) {
> +		print_skip("Only one usable CPU, task migration not possible");
>   		exit(KSFT_SKIP);
>   	}
>   

Queued, thanks.

Paolo