| Message ID | 20220719013540.3477946-1-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] KVM: selftests: Fix target thread to be migrated in rseq_test | expand |
On Tue, Jul 19, 2022, Gavin Shan wrote: > --- > v3: Improved changelog (Oliver Upon) Sorry I didn't catch v3, I saw that you waited but just didn't get to this earlier :-/ > --- > tools/testing/selftests/kvm/rseq_test.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index 4158da0da2bb..c83ac7b467f8 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -38,6 +38,7 @@ static __thread volatile struct rseq __rseq = { > */ > #define NR_TASK_MIGRATIONS 100000 > > +static pid_t rseq_tid; > static pthread_t migration_thread; > static cpu_set_t possible_mask; > static int min_cpu, max_cpu; > @@ -106,7 +107,8 @@ static void *migration_worker(void *ign) Pass the target TID to the worker, then there's no need to use a global and no chance of consuming rseq_tid "uninitialized". The casting to convert gettid() to a "void *" is annoying, but not the end of the world. > * stable, i.e. while changing affinity is in-progress. > */ > smp_wmb(); > - r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask); > + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask), > + &allowed_mask); Eh, let this poke out, don't think it's worth wrapping here. E.g. --- tools/testing/selftests/kvm/rseq_test.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index aba7be178dab..a54d4d05a058 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -80,8 +80,9 @@ static int next_cpu(int cpu) return cpu; } -static void *migration_worker(void *ign) +static void *migration_worker(void *__rseq_tid) { + pid_t rseq_tid = (pid_t)(unsigned long)__rseq_tid; cpu_set_t allowed_mask; int r, i, cpu; @@ -104,7 +105,7 @@ static void *migration_worker(void *ign) * stable, i.e. while changing affinity is in-progress. */ smp_wmb(); - r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask); + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask), &allowed_mask); TEST_ASSERT(!r, "sched_setaffinity failed, errno = %d (%s)", errno, strerror(errno)); smp_wmb(); @@ -227,7 +228,8 @@ int main(int argc, char *argv[]) vm = vm_create_with_one_vcpu(&vcpu, guest_code); ucall_init(vm, NULL); - pthread_create(&migration_thread, NULL, migration_worker, 0); + pthread_create(&migration_thread, NULL, migration_worker, + (void *)(unsigned long)gettid()); for (i = 0; !done; i++) { vcpu_run(vcpu); base-commit: ad6cb756bb497997032df2bda7cbdff076e4a66a --
Hi Sean, On 7/19/22 9:46 AM, Sean Christopherson wrote: > On Tue, Jul 19, 2022, Gavin Shan wrote: >> --- >> v3: Improved changelog (Oliver Upon) > > Sorry I didn't catch v3, I saw that you waited but just didn't get to this earlier :-/ > Not a problem at all :) >> --- >> tools/testing/selftests/kvm/rseq_test.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c >> index 4158da0da2bb..c83ac7b467f8 100644 >> --- a/tools/testing/selftests/kvm/rseq_test.c >> +++ b/tools/testing/selftests/kvm/rseq_test.c >> @@ -38,6 +38,7 @@ static __thread volatile struct rseq __rseq = { >> */ >> #define NR_TASK_MIGRATIONS 100000 >> >> +static pid_t rseq_tid; >> static pthread_t migration_thread; >> static cpu_set_t possible_mask; >> static int min_cpu, max_cpu; >> @@ -106,7 +107,8 @@ static void *migration_worker(void *ign) > > Pass the target TID to the worker, then there's no need to use a global and no > chance of consuming rseq_tid "uninitialized". The casting to convert gettid() to > a "void *" is annoying, but not the end of the world. > I was thinking of the scheme, but passing the address of a local variable for the thread ID. Your suggestion also makes sense to me. >> * stable, i.e. while changing affinity is in-progress. >> */ >> smp_wmb(); >> - r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask); >> + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask), >> + &allowed_mask); > > Eh, let this poke out, don't think it's worth wrapping here. > Ok, I was trying to follow rule of 80-characters per line, even it's not strictly needed nowadays. It's also fine not to follow :) I just picked your code and posted v4: https://lore.kernel.org/kvmarm/20220719020830.3479482-1-gshan@redhat.com/T/#u Thanks, Gavin > E.g. > > --- > tools/testing/selftests/kvm/rseq_test.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index aba7be178dab..a54d4d05a058 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -80,8 +80,9 @@ static int next_cpu(int cpu) > return cpu; > } > > -static void *migration_worker(void *ign) > +static void *migration_worker(void *__rseq_tid) > { > + pid_t rseq_tid = (pid_t)(unsigned long)__rseq_tid; > cpu_set_t allowed_mask; > int r, i, cpu; > > @@ -104,7 +105,7 @@ static void *migration_worker(void *ign) > * stable, i.e. while changing affinity is in-progress. > */ > smp_wmb(); > - r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask); > + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask), &allowed_mask); > TEST_ASSERT(!r, "sched_setaffinity failed, errno = %d (%s)", > errno, strerror(errno)); > smp_wmb(); > @@ -227,7 +228,8 @@ int main(int argc, char *argv[]) > vm = vm_create_with_one_vcpu(&vcpu, guest_code); > ucall_init(vm, NULL); > > - pthread_create(&migration_thread, NULL, migration_worker, 0); > + pthread_create(&migration_thread, NULL, migration_worker, > + (void *)(unsigned long)gettid()); > > for (i = 0; !done; i++) { > vcpu_run(vcpu); > > base-commit: ad6cb756bb497997032df2bda7cbdff076e4a66a > -- >
diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index 4158da0da2bb..c83ac7b467f8 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -38,6 +38,7 @@ static __thread volatile struct rseq __rseq = { */ #define NR_TASK_MIGRATIONS 100000 +static pid_t rseq_tid; static pthread_t migration_thread; static cpu_set_t possible_mask; static int min_cpu, max_cpu; @@ -106,7 +107,8 @@ static void *migration_worker(void *ign) * stable, i.e. while changing affinity is in-progress. */ smp_wmb(); - r = sched_setaffinity(0, sizeof(allowed_mask), &allowed_mask); + r = sched_setaffinity(rseq_tid, sizeof(allowed_mask), + &allowed_mask); TEST_ASSERT(!r, "sched_setaffinity failed, errno = %d (%s)", errno, strerror(errno)); smp_wmb(); @@ -231,6 +233,7 @@ int main(int argc, char *argv[]) vm = vm_create_default(VCPU_ID, 0, guest_code); ucall_init(vm, NULL); + rseq_tid = gettid(); pthread_create(&migration_thread, NULL, migration_worker, 0); for (i = 0; !done; i++) {
In rseq_test, there are two threads, which are vCPU thread and migration worker separately. Unfortunately, the test has the wrong PID passed to sched_setaffinity() in the migration worker. It forces migration on the migration worker because zeroed PID represents the calling thread, which is the migration worker itself. It means the vCPU thread is never enforced to migration and it can migrate at any time, which eventually leads to failure as the following logs show. host# uname -r 5.19.0-rc6-gavin+ host# # cat /proc/cpuinfo | grep processor | tail -n 1 processor : 223 host# pwd /home/gavin/sandbox/linux.main/tools/testing/selftests/kvm host# for i in `seq 1 100`; do \ echo "--------> $i"; ./rseq_test; done --------> 1 --------> 2 --------> 3 --------> 4 --------> 5 --------> 6 ==== Test Assertion Failure ==== rseq_test.c:265: rseq_cpu == cpu pid=3925 tid=3925 errno=4 - Interrupted system call 1 0x0000000000401963: main at rseq_test.c:265 (discriminator 2) 2 0x0000ffffb044affb: ?? ??:0 3 0x0000ffffb044b0c7: ?? ??:0 4 0x0000000000401a6f: _start at ??:? rseq CPU = 4, sched CPU = 27 Fix the issue by passing correct parameter, TID of the vCPU thread, to sched_setaffinity() in the migration worker. Fixes: 61e52f1630f5 ("KVM: selftests: Add a test for KVM_RUN+rseq to detect task migration bugs") Signed-off-by: Gavin Shan <gshan@redhat.com> Reviewed-by: Oliver Upton <oliver.upton@linux.dev> --- v3: Improved changelog (Oliver Upon) --- tools/testing/selftests/kvm/rseq_test.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)