| Message ID | 20220809060627.115847-3-gshan@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kvm/selftests: Two rseq_test fixes | expand |
* Gavin Shan: > sched_getcpu() is glibc dependent and it can simply return the CPU > ID from the registered rseq information, as Florian Weimer pointed. > In this case, it's pointless to compare the return value from > sched_getcpu() and that fetched from the registered rseq information. > > Fix the issue by replacing sched_getcpu() with getcpu(), as Florian > suggested. The comments are modified accordingly. Note that getcpu was added in glibc 2.29, so perhaps you need to perform a direct system call? Thanks, Florian
* Florian Weimer: > * Gavin Shan: > >> sched_getcpu() is glibc dependent and it can simply return the CPU >> ID from the registered rseq information, as Florian Weimer pointed. >> In this case, it's pointless to compare the return value from >> sched_getcpu() and that fetched from the registered rseq information. >> >> Fix the issue by replacing sched_getcpu() with getcpu(), as Florian >> suggested. The comments are modified accordingly. > > Note that getcpu was added in glibc 2.29, so perhaps you need to perform > a direct system call? One more thing: syscall(__NR_getcpu) also has the advantage that it wouldn't have to be changed again if node IDs become available via rseq and getcpu is implemented using that. Thanks, Florian
On 8/9/22 5:17 PM, Florian Weimer wrote: > * Florian Weimer: > >> * Gavin Shan: >> >>> sched_getcpu() is glibc dependent and it can simply return the CPU >>> ID from the registered rseq information, as Florian Weimer pointed. >>> In this case, it's pointless to compare the return value from >>> sched_getcpu() and that fetched from the registered rseq information. >>> >>> Fix the issue by replacing sched_getcpu() with getcpu(), as Florian >>> suggested. The comments are modified accordingly. >> >> Note that getcpu was added in glibc 2.29, so perhaps you need to perform >> a direct system call? > > One more thing: syscall(__NR_getcpu) also has the advantage that it > wouldn't have to be changed again if node IDs become available via rseq > and getcpu is implemented using that. > > Thanks, > Florian > Thanks, Florian. It makes sense to me to use syscall(__NR_getcpu) in next revision. Thanks for your quick review :) I would hold for one or two days to post v2, to see if others have more comments. Thanks, Gavin
On Tue, Aug 09, 2022, Gavin Shan wrote: > On 8/9/22 5:17 PM, Florian Weimer wrote: > > * Florian Weimer: > > > > > * Gavin Shan: > > > > > > > sched_getcpu() is glibc dependent and it can simply return the CPU > > > > ID from the registered rseq information, as Florian Weimer pointed. > > > > In this case, it's pointless to compare the return value from > > > > sched_getcpu() and that fetched from the registered rseq information. > > > > > > > > Fix the issue by replacing sched_getcpu() with getcpu(), as Florian > > > > suggested. The comments are modified accordingly. > > > > > > Note that getcpu was added in glibc 2.29, so perhaps you need to perform > > > a direct system call? > > > > One more thing: syscall(__NR_getcpu) also has the advantage that it > > wouldn't have to be changed again if node IDs become available via rseq > > and getcpu is implemented using that. > > > > Thanks, > > Florian > > > > Thanks, Florian. It makes sense to me to use syscall(__NR_getcpu) in > next revision. Thanks for your quick review :) +1, and definitely add a comment to prevent future "cleanup".
On 8/10/22 6:53 AM, Sean Christopherson wrote: > On Tue, Aug 09, 2022, Gavin Shan wrote: >> On 8/9/22 5:17 PM, Florian Weimer wrote: >>> * Florian Weimer: >>> >>>> * Gavin Shan: >>>> >>>>> sched_getcpu() is glibc dependent and it can simply return the CPU >>>>> ID from the registered rseq information, as Florian Weimer pointed. >>>>> In this case, it's pointless to compare the return value from >>>>> sched_getcpu() and that fetched from the registered rseq information. >>>>> >>>>> Fix the issue by replacing sched_getcpu() with getcpu(), as Florian >>>>> suggested. The comments are modified accordingly. >>>> >>>> Note that getcpu was added in glibc 2.29, so perhaps you need to perform >>>> a direct system call? >>> >>> One more thing: syscall(__NR_getcpu) also has the advantage that it >>> wouldn't have to be changed again if node IDs become available via rseq >>> and getcpu is implemented using that. >>> >>> Thanks, >>> Florian >>> >> >> Thanks, Florian. It makes sense to me to use syscall(__NR_getcpu) in >> next revision. Thanks for your quick review :) > > +1, and definitely add a comment to prevent future "cleanup". > Yep, I will have something like below in next revision: /* * We have to perform direct system call for getcpu() because it's not * available until glic 2.29. */ Thanks, Gavin
diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index acb1bf1f06b3..621b9b15049b 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -126,7 +126,7 @@ static void *migration_worker(void *__rseq_tid) atomic_inc(&seq_cnt); /* - * Ensure the odd count is visible while sched_getcpu() isn't + * Ensure the odd count is visible while getcpu() isn't * stable, i.e. while changing affinity is in-progress. */ smp_wmb(); @@ -167,10 +167,10 @@ static void *migration_worker(void *__rseq_tid) * check completes. * * 3. To ensure the read-side makes efficient forward progress, - * e.g. if sched_getcpu() involves a syscall. Stalling the - * read-side means the test will spend more time waiting for - * sched_getcpu() to stabilize and less time trying to hit - * the timing-dependent bug. + * e.g. if getcpu() involves a syscall. Stalling the read-side + * means the test will spend more time waiting for getcpu() + * to stabilize and less time trying to hit the timing-dependent + * bug. * * Because any bug in this area is likely to be timing-dependent, * run with a range of delays at 1us intervals from 1us to 10us @@ -264,9 +264,9 @@ int main(int argc, char *argv[]) /* * Verify rseq's CPU matches sched's CPU. Ensure migration - * doesn't occur between sched_getcpu() and reading the rseq - * cpu_id by rereading both if the sequence count changes, or - * if the count is odd (migration in-progress). + * doesn't occur between getcpu() and reading the rseq cpu_id + * by rereading both if the sequence count changes, or if the + * count is odd (migration in-progress). */ do { /* @@ -276,15 +276,15 @@ int main(int argc, char *argv[]) snapshot = atomic_read(&seq_cnt) & ~1; /* - * Ensure reading sched_getcpu() and rseq.cpu_id - * complete in a single "no migration" window, i.e. are - * not reordered across the seq_cnt reads. + * Ensure reading getcpu() and rseq.cpu_id complete in + * a single "no migration" window, i.e. are not reordered + * across the seq_cnt reads. */ smp_rmb(); - cpu = sched_getcpu(); + r = getcpu(&cpu, NULL); rseq_cpu = READ_ONCE(__rseq_info->cpu_id); smp_rmb(); - } while (snapshot != atomic_read(&seq_cnt)); + } while (r || snapshot != atomic_read(&seq_cnt)); TEST_ASSERT(rseq_cpu == cpu, "rseq CPU = %d, sched CPU = %d\n", rseq_cpu, cpu); @@ -293,9 +293,9 @@ int main(int argc, char *argv[]) /* * Sanity check that the test was able to enter the guest a reasonable * number of times, e.g. didn't get stalled too often/long waiting for - * sched_getcpu() to stabilize. A 2:1 migration:KVM_RUN ratio is a - * fairly conservative ratio on x86-64, which can do _more_ KVM_RUNs - * than migrations given the 1us+ delay in the migration task. + * getcpu() to stabilize. A 2:1 migration:KVM_RUN ratio is a fairly + * conservative ratio on x86-64, which can do _more_ KVM_RUNs than + * migrations given the 1us+ delay in the migration task. */ TEST_ASSERT(i > (NR_TASK_MIGRATIONS / 2), "Only performed %d KVM_RUNs, task stalled too much?\n", i);
sched_getcpu() is glibc dependent and it can simply return the CPU ID from the registered rseq information, as Florian Weimer pointed. In this case, it's pointless to compare the return value from sched_getcpu() and that fetched from the registered rseq information. Fix the issue by replacing sched_getcpu() with getcpu(), as Florian suggested. The comments are modified accordingly. Reported-by: Yihuang Yu <yihyu@redhat.com> Suggested-by: Florian Weimer <fweimer@redhat.com> Suggested-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Signed-off-by: Gavin Shan <gshan@redhat.com> --- tools/testing/selftests/kvm/rseq_test.c | 32 ++++++++++++------------- 1 file changed, 16 insertions(+), 16 deletions(-)