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

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

[Sean Christopherson]

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@oracle.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 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);
 	}
 
-- 
2.33.0.685.g46640cef36-goog

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@oracle.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   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