* [PATCH 0/2] kvm/selftests: Two rseq_test fixes @ 2022-08-09 6:06 Gavin Shan 2022-08-09 6:06 ` [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 Gavin Shan 2022-08-09 6:06 ` [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test Gavin Shan 0 siblings, 2 replies; 25+ messages in thread From: Gavin Shan @ 2022-08-09 6:06 UTC (permalink / raw) To: kvmarm Cc: fweimer, shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini There are two issues in current rseq_test implementation and the series intends to fix them: - From glibc-2.35, rseq information is registered by TLS. It means rseq_test is unable to register its own rseq information. PATCH[01] fixes the issue by reuse TLS's rseq information if needed. - sched_getcpu() relies on glibc's implementation and it can simply returns the CPU ID cached in the rseq information. In this case, it's pointless to compare the return value from sched_getcpu() and that fetched from rseq information. PATCH[02] fixes the issue by replacing sched_getcpu() with getcpu(). Gavin Shan (2): KVM: selftests: Make rseq compatible with glibc-2.35 KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test tools/testing/selftests/kvm/rseq_test.c | 62 ++++++++++++++++++------- 1 file changed, 44 insertions(+), 18 deletions(-) -- 2.23.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 6:06 [PATCH 0/2] kvm/selftests: Two rseq_test fixes Gavin Shan @ 2022-08-09 6:06 ` Gavin Shan 2022-08-09 6:33 ` Florian Weimer 2022-08-09 6:06 ` [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test Gavin Shan 1 sibling, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-09 6:06 UTC (permalink / raw) To: kvmarm Cc: fweimer, shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini The rseq information is registered by TLS, starting from glibc-2.35. In this case, the test always fails due to syscall(__NR_rseq). For example, on RHEL9.1 where upstream glibc-2.35 features are enabled on downstream glibc-2.34, the test fails like below. # ./rseq_test ==== Test Assertion Failure ==== rseq_test.c:60: !r pid=112043 tid=112043 errno=22 - Invalid argument 1 0x0000000000401973: main at rseq_test.c:226 2 0x0000ffff84b6c79b: ?? ??:0 3 0x0000ffff84b6c86b: ?? ??:0 4 0x0000000000401b6f: _start at ??:? rseq failed, errno = 22 (Invalid argument) # rpm -aq | grep glibc-2 glibc-2.34-39.el9.aarch64 Fix the issue by using the registered rseq information from TLS if it exists. Otherwise, we're going to register our own rseq information as before. 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 | 30 +++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c index a54d4d05a058..acb1bf1f06b3 100644 --- a/tools/testing/selftests/kvm/rseq_test.c +++ b/tools/testing/selftests/kvm/rseq_test.c @@ -9,6 +9,7 @@ #include <string.h> #include <signal.h> #include <syscall.h> +#include <dlfcn.h> #include <sys/ioctl.h> #include <sys/sysinfo.h> #include <asm/barrier.h> @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { */ #define NR_TASK_MIGRATIONS 100000 +static bool __rseq_ownership; +static volatile struct rseq *__rseq_info; static pthread_t migration_thread; static cpu_set_t possible_mask; static int min_cpu, max_cpu; @@ -49,11 +52,33 @@ static void guest_code(void) GUEST_SYNC(0); } +static void sys_rseq_ownership(void) +{ + long *offset; + unsigned int *size, *flags; + + offset = dlsym(RTLD_NEXT, "__rseq_offset"); + size = dlsym(RTLD_NEXT, "__rseq_size"); + flags = dlsym(RTLD_NEXT, "__rseq_flags"); + + if (offset && size && *size && flags) { + __rseq_ownership = false; + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + + *offset); + } else { + __rseq_ownership = true; + __rseq_info = &__rseq; + } +} + static void sys_rseq(int flags) { int r; - r = syscall(__NR_rseq, &__rseq, sizeof(__rseq), flags, RSEQ_SIG); + if (!__rseq_ownership) + return; + + r = syscall(__NR_rseq, __rseq_info, sizeof(*__rseq_info), flags, RSEQ_SIG); TEST_ASSERT(!r, "rseq failed, errno = %d (%s)", errno, strerror(errno)); } @@ -218,6 +243,7 @@ int main(int argc, char *argv[]) calc_min_max_cpu(); + sys_rseq_ownership(); sys_rseq(0); /* @@ -256,7 +282,7 @@ int main(int argc, char *argv[]) */ smp_rmb(); cpu = sched_getcpu(); - rseq_cpu = READ_ONCE(__rseq.cpu_id); + rseq_cpu = READ_ONCE(__rseq_info->cpu_id); smp_rmb(); } while (snapshot != atomic_read(&seq_cnt)); -- 2.23.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 6:06 ` [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 Gavin Shan @ 2022-08-09 6:33 ` Florian Weimer 2022-08-09 8:45 ` Gavin Shan 0 siblings, 1 reply; 25+ messages in thread From: Florian Weimer @ 2022-08-09 6:33 UTC (permalink / raw) To: Gavin Shan Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm * Gavin Shan: > diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c > index a54d4d05a058..acb1bf1f06b3 100644 > --- a/tools/testing/selftests/kvm/rseq_test.c > +++ b/tools/testing/selftests/kvm/rseq_test.c > @@ -9,6 +9,7 @@ > #include <string.h> > #include <signal.h> > #include <syscall.h> > +#include <dlfcn.h> > #include <sys/ioctl.h> > #include <sys/sysinfo.h> > #include <asm/barrier.h> I'm surprised that there isn't a Makefile update to link with -ldl (still required for glibc 2.33 and earlier). > @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { > */ > #define NR_TASK_MIGRATIONS 100000 > > +static bool __rseq_ownership; > +static volatile struct rseq *__rseq_info; > static pthread_t migration_thread; > static cpu_set_t possible_mask; > static int min_cpu, max_cpu; > @@ -49,11 +52,33 @@ static void guest_code(void) > GUEST_SYNC(0); > } > > +static void sys_rseq_ownership(void) > +{ > + long *offset; > + unsigned int *size, *flags; > + > + offset = dlsym(RTLD_NEXT, "__rseq_offset"); > + size = dlsym(RTLD_NEXT, "__rseq_size"); > + flags = dlsym(RTLD_NEXT, "__rseq_flags"); > + > + if (offset && size && *size && flags) { > + __rseq_ownership = false; > + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + > + *offset); __builtin_thread_pointer doesn't work on all architectures/GCC versions. Is this a problem for selftests? Thanks, Florian _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 6:33 ` Florian Weimer @ 2022-08-09 8:45 ` Gavin Shan 2022-08-09 7:16 ` Florian Weimer 0 siblings, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-09 8:45 UTC (permalink / raw) To: Florian Weimer Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm Hi Florian, On 8/9/22 4:33 PM, Florian Weimer wrote: >> diff --git a/tools/testing/selftests/kvm/rseq_test.c b/tools/testing/selftests/kvm/rseq_test.c >> index a54d4d05a058..acb1bf1f06b3 100644 >> --- a/tools/testing/selftests/kvm/rseq_test.c >> +++ b/tools/testing/selftests/kvm/rseq_test.c >> @@ -9,6 +9,7 @@ >> #include <string.h> >> #include <signal.h> >> #include <syscall.h> >> +#include <dlfcn.h> >> #include <sys/ioctl.h> >> #include <sys/sysinfo.h> >> #include <asm/barrier.h> > > I'm surprised that there isn't a Makefile update to link with -ldl > (still required for glibc 2.33 and earlier). > In next revision, I will add '-ldl' into tools/testing/selftests/kvm/Makefile. >> @@ -36,6 +37,8 @@ static __thread volatile struct rseq __rseq = { >> */ >> #define NR_TASK_MIGRATIONS 100000 >> >> +static bool __rseq_ownership; >> +static volatile struct rseq *__rseq_info; >> static pthread_t migration_thread; >> static cpu_set_t possible_mask; >> static int min_cpu, max_cpu; >> @@ -49,11 +52,33 @@ static void guest_code(void) >> GUEST_SYNC(0); >> } >> >> +static void sys_rseq_ownership(void) >> +{ >> + long *offset; >> + unsigned int *size, *flags; >> + >> + offset = dlsym(RTLD_NEXT, "__rseq_offset"); >> + size = dlsym(RTLD_NEXT, "__rseq_size"); >> + flags = dlsym(RTLD_NEXT, "__rseq_flags"); >> + >> + if (offset && size && *size && flags) { >> + __rseq_ownership = false; >> + __rseq_info = (struct rseq *)((uintptr_t)__builtin_thread_pointer() + >> + *offset); > > __builtin_thread_pointer doesn't work on all architectures/GCC versions. > Is this a problem for selftests? > It's a problem as the test case is running on all architectures. I think I need introduce our own __builtin_thread_pointer() for where it's not supported: (1) PowerPC (2) x86 without GCC 11 Please let me know if I still have missed cases where __buitin_thread_pointer() isn't supported? Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 8:45 ` Gavin Shan @ 2022-08-09 7:16 ` Florian Weimer 2022-08-09 9:27 ` Gavin Shan 0 siblings, 1 reply; 25+ messages in thread From: Florian Weimer @ 2022-08-09 7:16 UTC (permalink / raw) To: Gavin Shan Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm * Gavin Shan: >> __builtin_thread_pointer doesn't work on all architectures/GCC >> versions. >> Is this a problem for selftests? >> > > It's a problem as the test case is running on all architectures. I think I > need introduce our own __builtin_thread_pointer() for where it's not > supported: (1) PowerPC (2) x86 without GCC 11 > > Please let me know if I still have missed cases where > __buitin_thread_pointer() isn't supported? As far as I know, these are the two outliers that also have rseq support. The list is a bit longer if we also consider non-rseq architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know about the Linux architectures without glibc support). Thanks, Florian _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 7:16 ` Florian Weimer @ 2022-08-09 9:27 ` Gavin Shan 2022-08-09 12:21 ` Mathieu Desnoyers 0 siblings, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-09 9:27 UTC (permalink / raw) To: Florian Weimer Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm Hi Florian, On 8/9/22 5:16 PM, Florian Weimer wrote: >>> __builtin_thread_pointer doesn't work on all architectures/GCC >>> versions. >>> Is this a problem for selftests? >>> >> >> It's a problem as the test case is running on all architectures. I think I >> need introduce our own __builtin_thread_pointer() for where it's not >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> Please let me know if I still have missed cases where >> __buitin_thread_pointer() isn't supported? > > As far as I know, these are the two outliers that also have rseq > support. The list is a bit longer if we also consider non-rseq > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > about the Linux architectures without glibc support). > For kvm/selftests, there are 3 architectures involved actually. So we just need consider 4 cases: aarch64, x86, s390 and other. For other case, we just use __builtin_thread_pointer() to maintain code's integrity, but it's not called at all. I think kvm/selftest is always relying on glibc if I'm correct. Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 9:27 ` Gavin Shan @ 2022-08-09 12:21 ` Mathieu Desnoyers 2022-08-09 13:44 ` Mathieu Desnoyers 2022-08-10 9:14 ` Paolo Bonzini 0 siblings, 2 replies; 25+ messages in thread From: Mathieu Desnoyers @ 2022-08-09 12:21 UTC (permalink / raw) To: Gavin Shan Cc: Florian Weimer, shan gavin, kvm, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, pbonzini, kvmarm ----- Gavin Shan <gshan@redhat.com> wrote: > Hi Florian, > > On 8/9/22 5:16 PM, Florian Weimer wrote: > >>> __builtin_thread_pointer doesn't work on all architectures/GCC > >>> versions. > >>> Is this a problem for selftests? > >>> > >> > >> It's a problem as the test case is running on all architectures. I think I > >> need introduce our own __builtin_thread_pointer() for where it's not > >> supported: (1) PowerPC (2) x86 without GCC 11 > >> > >> Please let me know if I still have missed cases where > >> __buitin_thread_pointer() isn't supported? > > > > As far as I know, these are the two outliers that also have rseq > > support. The list is a bit longer if we also consider non-rseq > > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > > about the Linux architectures without glibc support). > > > > For kvm/selftests, there are 3 architectures involved actually. So we > just need consider 4 cases: aarch64, x86, s390 and other. For other > case, we just use __builtin_thread_pointer() to maintain code's > integrity, but it's not called at all. > > I think kvm/selftest is always relying on glibc if I'm correct. All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? Thanks, Mathieu > > Thanks, > Gavin > -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 12:21 ` Mathieu Desnoyers @ 2022-08-09 13:44 ` Mathieu Desnoyers 2022-08-09 21:38 ` Sean Christopherson 2022-08-10 9:14 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Mathieu Desnoyers @ 2022-08-09 13:44 UTC (permalink / raw) To: Gavin Shan, shuah Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, Paolo Bonzini, kvmarm ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > ----- Gavin Shan <gshan@redhat.com> wrote: >> Hi Florian, >> >> On 8/9/22 5:16 PM, Florian Weimer wrote: >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC >> >>> versions. >> >>> Is this a problem for selftests? >> >>> >> >> >> >> It's a problem as the test case is running on all architectures. I think I >> >> need introduce our own __builtin_thread_pointer() for where it's not >> >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> >> >> Please let me know if I still have missed cases where >> >> __buitin_thread_pointer() isn't supported? >> > >> > As far as I know, these are the two outliers that also have rseq >> > support. The list is a bit longer if we also consider non-rseq >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >> > about the Linux architectures without glibc support). >> > >> >> For kvm/selftests, there are 3 architectures involved actually. So we >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> case, we just use __builtin_thread_pointer() to maintain code's >> integrity, but it's not called at all. >> >> I think kvm/selftest is always relying on glibc if I'm correct. > > All those are handled in the rseq selftests and in librseq. Why duplicate all > that logic again? More to the point, considering that we have all the relevant rseq registration code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, is there an easy way to get test applications in tools/testing/selftests/kvm and in tools/testing/selftests/rseq to share that common code ? Keeping duplicated compatibility code is bad for long-term maintainability. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 13:44 ` Mathieu Desnoyers @ 2022-08-09 21:38 ` Sean Christopherson 2022-08-10 0:37 ` Gavin Shan 2022-08-10 12:13 ` Mathieu Desnoyers 0 siblings, 2 replies; 25+ messages in thread From: Sean Christopherson @ 2022-08-09 21:38 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, Paolo Bonzini, shuah, kvmarm On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: > ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: > > > ----- Gavin Shan <gshan@redhat.com> wrote: > >> Hi Florian, > >> > >> On 8/9/22 5:16 PM, Florian Weimer wrote: > >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC > >> >>> versions. > >> >>> Is this a problem for selftests? > >> >>> > >> >> > >> >> It's a problem as the test case is running on all architectures. I think I > >> >> need introduce our own __builtin_thread_pointer() for where it's not > >> >> supported: (1) PowerPC (2) x86 without GCC 11 > >> >> > >> >> Please let me know if I still have missed cases where > >> >> __buitin_thread_pointer() isn't supported? > >> > > >> > As far as I know, these are the two outliers that also have rseq > >> > support. The list is a bit longer if we also consider non-rseq > >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know > >> > about the Linux architectures without glibc support). > >> > > >> > >> For kvm/selftests, there are 3 architectures involved actually. So we > >> just need consider 4 cases: aarch64, x86, s390 and other. For other > >> case, we just use __builtin_thread_pointer() to maintain code's > >> integrity, but it's not called at all. > >> > >> I think kvm/selftest is always relying on glibc if I'm correct. > > > > All those are handled in the rseq selftests and in librseq. Why duplicate all > > that logic again? > > More to the point, considering that we have all the relevant rseq registration > code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread > pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, > is there an easy way to get test applications in tools/testing/selftests/kvm > and in tools/testing/selftests/rseq to share that common code ? > > Keeping duplicated compatibility code is bad for long-term maintainability. Any reason not to simply add tools/lib/rseq.c and then expose a helper to get the registered rseq struct? _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 21:38 ` Sean Christopherson @ 2022-08-10 0:37 ` Gavin Shan 2022-08-10 12:29 ` Mathieu Desnoyers 2022-08-10 12:13 ` Mathieu Desnoyers 1 sibling, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-10 0:37 UTC (permalink / raw) To: Sean Christopherson, Mathieu Desnoyers Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, Paolo Bonzini, shuah, kvmarm Hi Mathieu and Sean, On 8/10/22 7:38 AM, Sean Christopherson wrote: > On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote: >>> ----- Gavin Shan <gshan@redhat.com> wrote: >>>> On 8/9/22 5:16 PM, Florian Weimer wrote: >>>>>>> __builtin_thread_pointer doesn't work on all architectures/GCC >>>>>>> versions. >>>>>>> Is this a problem for selftests? >>>>>>> >>>>>> >>>>>> It's a problem as the test case is running on all architectures. I think I >>>>>> need introduce our own __builtin_thread_pointer() for where it's not >>>>>> supported: (1) PowerPC (2) x86 without GCC 11 >>>>>> >>>>>> Please let me know if I still have missed cases where >>>>>> __buitin_thread_pointer() isn't supported? >>>>> >>>>> As far as I know, these are the two outliers that also have rseq >>>>> support. The list is a bit longer if we also consider non-rseq >>>>> architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >>>>> about the Linux architectures without glibc support). >>>>> >>>> >>>> For kvm/selftests, there are 3 architectures involved actually. So we >>>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>>> case, we just use __builtin_thread_pointer() to maintain code's >>>> integrity, but it's not called at all. >>>> >>>> I think kvm/selftest is always relying on glibc if I'm correct. >>> >>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>> that logic again? >> >> More to the point, considering that we have all the relevant rseq registration >> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >> is there an easy way to get test applications in tools/testing/selftests/kvm >> and in tools/testing/selftests/rseq to share that common code ? >> >> Keeping duplicated compatibility code is bad for long-term maintainability. > > Any reason not to simply add tools/lib/rseq.c and then expose a helper to get the > registered rseq struct? > There are couple of reasons, not to share tools/testing/selftests/rseq/librseq.so or add tools/lib/librseq.so. Please let me know if the arguments making sense to you? - By design, selftests/rseq and selftests/kvm are parallel. It's going to introduce unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To me, it makes the maintainability even harder. - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of functionalities, provided by selftests/rseq/librseq.so. - I'm not too much familiar with selftests/rseq, but it seems it need heavy rework before it can become tools/lib/librseq.so. However, I'm not sure if the effort is worthwhile. The newly added library is fully used by testtests/rseq. ~5% of that is going to be used by selftests/kvm. In this case, we still have cross-dependency issue. I personally prefer not to use selftests/rseq/librseq.so or add tools/lib/librseq.so, but I need your feedback. Please share your thoughts. Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 0:37 ` Gavin Shan @ 2022-08-10 12:29 ` Mathieu Desnoyers 2022-08-10 12:35 ` Paolo Bonzini 0 siblings, 1 reply; 25+ messages in thread From: Mathieu Desnoyers @ 2022-08-10 12:29 UTC (permalink / raw) To: Gavin Shan Cc: Florian Weimer, shan gavin, KVM list, linux-kernel, andrew jones, yihyu, linux-kselftest, maz, Paolo Bonzini, shuah, kvmarm ----- On Aug 9, 2022, at 8:37 PM, Gavin Shan gshan@redhat.com wrote: > Hi Mathieu and Sean, > > On 8/10/22 7:38 AM, Sean Christopherson wrote: >> On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >>> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >>> mathieu.desnoyers@efficios.com wrote: >>>> ----- Gavin Shan <gshan@redhat.com> wrote: >>>>> On 8/9/22 5:16 PM, Florian Weimer wrote: >>>>>>>> __builtin_thread_pointer doesn't work on all architectures/GCC >>>>>>>> versions. >>>>>>>> Is this a problem for selftests? >>>>>>>> >>>>>>> >>>>>>> It's a problem as the test case is running on all architectures. I think I >>>>>>> need introduce our own __builtin_thread_pointer() for where it's not >>>>>>> supported: (1) PowerPC (2) x86 without GCC 11 >>>>>>> >>>>>>> Please let me know if I still have missed cases where >>>>>>> __buitin_thread_pointer() isn't supported? >>>>>> >>>>>> As far as I know, these are the two outliers that also have rseq >>>>>> support. The list is a bit longer if we also consider non-rseq >>>>>> architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >>>>>> about the Linux architectures without glibc support). >>>>>> >>>>> >>>>> For kvm/selftests, there are 3 architectures involved actually. So we >>>>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>>>> case, we just use __builtin_thread_pointer() to maintain code's >>>>> integrity, but it's not called at all. >>>>> >>>>> I think kvm/selftest is always relying on glibc if I'm correct. >>>> >>>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>>> that logic again? >>> >>> More to the point, considering that we have all the relevant rseq registration >>> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >>> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >>> is there an easy way to get test applications in tools/testing/selftests/kvm >>> and in tools/testing/selftests/rseq to share that common code ? >>> >>> Keeping duplicated compatibility code is bad for long-term maintainability. >> >> Any reason not to simply add tools/lib/rseq.c and then expose a helper to get >> the >> registered rseq struct? >> > > There are couple of reasons, not to share > tools/testing/selftests/rseq/librseq.so > or add tools/lib/librseq.so. Please let me know if the arguments making sense > to you? > > - By design, selftests/rseq and selftests/kvm are parallel. It's going to > introduce > unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To > me, > it makes the maintainability even harder. In terms of build system, yes, selftests/rseq and selftests/kvm are side-by-side, and I agree it is odd to have a cross-dependency. That's where moving rseq.c to tools/lib/ makes sense. > > - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of > functionalities, provided by selftests/rseq/librseq.so. I've never seen this type of argument used to prevent using a library before, except on extremely memory-constrained devices, which is not our target here. Even if you would only use 1% of the features of a library, it does not justify reimplementing that 1% if that code already sits within the same project (kernel selftests). > > - I'm not too much familiar with selftests/rseq, but it seems it need heavy > rework before it can become tools/lib/librseq.so. However, I'm not sure if > the effort is worthwhile. The newly added library is fully used by > testtests/rseq. ~5% of that is going to be used by selftests/kvm. > In this case, we still have cross-dependency issue. No, it's just moving files around and a bit of Makefile modifications. That's the simple part. > > I personally prefer not to use selftests/rseq/librseq.so or add > tools/lib/librseq.so, > but I need your feedback. Please share your thoughts. I strongly favor that we use a two steps approach: 1) immediate fix: include ../rseq/rseq.c into your test code and use the headers, as proposed by Paolo. 2) I'll move librseq code into tools/lib/ and tools/include/rseq/, and adapt the users accordingly. (after the end of my vacation) Thanks, Mathieu > Thanks, > Gavin -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 12:29 ` Mathieu Desnoyers @ 2022-08-10 12:35 ` Paolo Bonzini 0 siblings, 0 replies; 25+ messages in thread From: Paolo Bonzini @ 2022-08-10 12:35 UTC (permalink / raw) To: Mathieu Desnoyers, Gavin Shan Cc: Florian Weimer, shan gavin, KVM list, linux-kernel, andrew jones, yihyu, linux-kselftest, maz, shuah, kvmarm On 8/10/22 14:29, Mathieu Desnoyers wrote: >> - By design, selftests/rseq and selftests/kvm are parallel. It's going to >> introduce >> unnecessary dependency for selftests/kvm to use selftests/rseq/librseq.so. To >> me, >> it makes the maintainability even harder. > In terms of build system, yes, selftests/rseq and selftests/kvm are side-by-side, > and I agree it is odd to have a cross-dependency. > > That's where moving rseq.c to tools/lib/ makes sense. > >> - What selftests/kvm needs is rseq-thread-pointer.h, which accounts for ~5% of >> functionalities, provided by selftests/rseq/librseq.so. > I've never seen this type of argument used to prevent using a library before, except > on extremely memory-constrained devices, which is not our target here. I agree. To me, the main argument against moving librseq to tools/lib is a variant of the build-system argument, namely that recursive Make sucks[1] and selftests/kvm right now does not use tools/lib. So, for a single-file library, it may be simply not worth the hassle. On the other hand, if "somebody else" does the work, I would have no problem with having selftests/kvm depend on tools/lib, not at all. Thanks, Paolo [1] Kbuild is a marvel that makes it work, but it works because there are no such cross-subdirectory dependencies and anyway tools/testing/selftests does not use Kbuild. _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 21:38 ` Sean Christopherson 2022-08-10 0:37 ` Gavin Shan @ 2022-08-10 12:13 ` Mathieu Desnoyers 2022-08-10 23:52 ` Gavin Shan 1 sibling, 1 reply; 25+ messages in thread From: Mathieu Desnoyers @ 2022-08-10 12:13 UTC (permalink / raw) To: Sean Christopherson Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, Paolo Bonzini, shuah, kvmarm ----- On Aug 9, 2022, at 5:38 PM, Sean Christopherson seanjc@google.com wrote: > On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >> mathieu.desnoyers@efficios.com wrote: >> >> > ----- Gavin Shan <gshan@redhat.com> wrote: >> >> Hi Florian, >> >> >> >> On 8/9/22 5:16 PM, Florian Weimer wrote: >> >> >>> __builtin_thread_pointer doesn't work on all architectures/GCC >> >> >>> versions. >> >> >>> Is this a problem for selftests? >> >> >>> >> >> >> >> >> >> It's a problem as the test case is running on all architectures. I think I >> >> >> need introduce our own __builtin_thread_pointer() for where it's not >> >> >> supported: (1) PowerPC (2) x86 without GCC 11 >> >> >> >> >> >> Please let me know if I still have missed cases where >> >> >> __buitin_thread_pointer() isn't supported? >> >> > >> >> > As far as I know, these are the two outliers that also have rseq >> >> > support. The list is a bit longer if we also consider non-rseq >> >> > architectures (csky, hppa, ia64, m68k, microblaze, sparc, don't know >> >> > about the Linux architectures without glibc support). >> >> > >> >> >> >> For kvm/selftests, there are 3 architectures involved actually. So we >> >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> >> case, we just use __builtin_thread_pointer() to maintain code's >> >> integrity, but it's not called at all. >> >> >> >> I think kvm/selftest is always relying on glibc if I'm correct. >> > >> > All those are handled in the rseq selftests and in librseq. Why duplicate all >> > that logic again? >> >> More to the point, considering that we have all the relevant rseq registration >> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >> is there an easy way to get test applications in tools/testing/selftests/kvm >> and in tools/testing/selftests/rseq to share that common code ? >> >> Keeping duplicated compatibility code is bad for long-term maintainability. > > Any reason not to simply add tools/lib/rseq.c and then expose a helper to get > the > registered rseq struct? Indeed, moving rseq.c to tools/lib/ would allow building a .so from any selftest which needs to use it. And we could move the relevant rseq helper header files to tools/include/rseq/* as well. Thoughts ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 12:13 ` Mathieu Desnoyers @ 2022-08-10 23:52 ` Gavin Shan 0 siblings, 0 replies; 25+ messages in thread From: Gavin Shan @ 2022-08-10 23:52 UTC (permalink / raw) To: Mathieu Desnoyers, Sean Christopherson Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, Paolo Bonzini, shuah, kvmarm Hi Mathieu, On 8/10/22 10:13 PM, Mathieu Desnoyers wrote: > ----- On Aug 9, 2022, at 5:38 PM, Sean Christopherson seanjc@google.com wrote: >> On Tue, Aug 09, 2022, Mathieu Desnoyers wrote: >>> ----- On Aug 9, 2022, at 8:21 AM, Mathieu Desnoyers >>> mathieu.desnoyers@efficios.com wrote: [...] >>>> >>>> All those are handled in the rseq selftests and in librseq. Why duplicate all >>>> that logic again? >>> >>> More to the point, considering that we have all the relevant rseq registration >>> code in tools/testing/selftests/rseq/rseq.c already, and the relevant thread >>> pointer getter code in tools/testing/selftests/rseq/rseq-*thread-pointer.h, >>> is there an easy way to get test applications in tools/testing/selftests/kvm >>> and in tools/testing/selftests/rseq to share that common code ? >>> >>> Keeping duplicated compatibility code is bad for long-term maintainability. >> >> Any reason not to simply add tools/lib/rseq.c and then expose a helper to get >> the >> registered rseq struct? > > Indeed, moving rseq.c to tools/lib/ would allow building a .so from any selftest > which needs to use it. > > And we could move the relevant rseq helper header files to tools/include/rseq/* > as well. > > Thoughts ? > One question is how librseq.so can be built automatically, when I'm going to build tools/testing/selftests/kvm/rseq_test. # cd linux/tools/testing/selftests/kvm # make rseq_test It's not perfect if I have to build tools/lib/librseq.so in advance, in order to build tools/testing/selftests/kvm/rseq_test for the sake of dependency. Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-09 12:21 ` Mathieu Desnoyers 2022-08-09 13:44 ` Mathieu Desnoyers @ 2022-08-10 9:14 ` Paolo Bonzini 2022-08-10 9:59 ` Gavin Shan 2022-08-10 12:17 ` Mathieu Desnoyers 1 sibling, 2 replies; 25+ messages in thread From: Paolo Bonzini @ 2022-08-10 9:14 UTC (permalink / raw) To: Mathieu Desnoyers, Gavin Shan Cc: Florian Weimer, shan gavin, kvm, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, kvmarm On 8/9/22 14:21, Mathieu Desnoyers wrote: >> For kvm/selftests, there are 3 architectures involved actually. So we >> just need consider 4 cases: aarch64, x86, s390 and other. For other >> case, we just use __builtin_thread_pointer() to maintain code's >> integrity, but it's not called at all. >> >> I think kvm/selftest is always relying on glibc if I'm correct. > All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? Yeah, rseq_test should reuse librseq code. The simplest way, if slightly hackish, is to do something like diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile index 690b499c3471..6c192b0ec304 100644 --- a/tools/testing/selftests/kvm/Makefile +++ b/tools/testing/selftests/kvm/Makefile @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) UNAME_M := riscv endif LIBKVM += lib/assert.c LIBKVM += lib/elf.c LIBKVM += lib/guest_modes.c @@ -198,7 +199,7 @@ endif CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) and just #include "../rseq/rseq.c" in rseq_test.c. Thanks, Paolo _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 9:14 ` Paolo Bonzini @ 2022-08-10 9:59 ` Gavin Shan 2022-08-10 12:17 ` Mathieu Desnoyers 1 sibling, 0 replies; 25+ messages in thread From: Gavin Shan @ 2022-08-10 9:59 UTC (permalink / raw) To: Paolo Bonzini, Mathieu Desnoyers Cc: Florian Weimer, shan gavin, kvm, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, kvmarm Hi Paolo, On 8/10/22 7:14 PM, Paolo Bonzini wrote: > On 8/9/22 14:21, Mathieu Desnoyers wrote: >>> For kvm/selftests, there are 3 architectures involved actually. So we >>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>> case, we just use __builtin_thread_pointer() to maintain code's >>> integrity, but it's not called at all. >>> >>> I think kvm/selftest is always relying on glibc if I'm correct. >> All those are handled in the rseq selftests and in librseq. Why duplicate all that logic again? > > Yeah, rseq_test should reuse librseq code. The simplest way, > if slightly hackish, is to do something like > > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile > index 690b499c3471..6c192b0ec304 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) > UNAME_M := riscv > endif > > LIBKVM += lib/assert.c > LIBKVM += lib/elf.c > LIBKVM += lib/guest_modes.c > @@ -198,7 +199,7 @@ endif > CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ > -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ > -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ > - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) > + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq > > no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ > $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) > > > and just #include "../rseq/rseq.c" in rseq_test.c. > Thank you. It's really a nice idea. I think it's best way to share "../rseq/rseq.c". In this way, we needn't to rely on "../rseq/librseq.so", which is compiled by "../rseq/Makefile". I will modify the code accordingly in v2 :) Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 9:14 ` Paolo Bonzini 2022-08-10 9:59 ` Gavin Shan @ 2022-08-10 12:17 ` Mathieu Desnoyers 2022-08-10 12:19 ` Paolo Bonzini 1 sibling, 1 reply; 25+ messages in thread From: Mathieu Desnoyers @ 2022-08-10 12:17 UTC (permalink / raw) To: Paolo Bonzini Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, kvmarm ----- On Aug 10, 2022, at 5:14 AM, Paolo Bonzini pbonzini@redhat.com wrote: > On 8/9/22 14:21, Mathieu Desnoyers wrote: >>> For kvm/selftests, there are 3 architectures involved actually. So we >>> just need consider 4 cases: aarch64, x86, s390 and other. For other >>> case, we just use __builtin_thread_pointer() to maintain code's >>> integrity, but it's not called at all. >>> >>> I think kvm/selftest is always relying on glibc if I'm correct. >> All those are handled in the rseq selftests and in librseq. Why duplicate all >> that logic again? > > Yeah, rseq_test should reuse librseq code. The simplest way, > if slightly hackish, is to do something like > > diff --git a/tools/testing/selftests/kvm/Makefile > b/tools/testing/selftests/kvm/Makefile > index 690b499c3471..6c192b0ec304 100644 > --- a/tools/testing/selftests/kvm/Makefile > +++ b/tools/testing/selftests/kvm/Makefile > @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) > UNAME_M := riscv > endif > > LIBKVM += lib/assert.c > LIBKVM += lib/elf.c > LIBKVM += lib/guest_modes.c > @@ -198,7 +199,7 @@ endif > CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ > -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ > -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ > - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) > + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq > > no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ > $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) > > > and just #include "../rseq/rseq.c" in rseq_test.c. Hi Paolo, Indeed, this hack seems to be a good approach to immediately fix things without moving around all source files and headers. In the longer term, I'd prefer Sean's proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). This can be done in a follow up phase though. I'll put a note on my todo list for after I come back from vacation. I'll be able to do this refactoring on top of this fix. Thanks, Mathieu > > Thanks, > > Paolo -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 12:17 ` Mathieu Desnoyers @ 2022-08-10 12:19 ` Paolo Bonzini 2022-08-10 23:34 ` Gavin Shan 0 siblings, 1 reply; 25+ messages in thread From: Paolo Bonzini @ 2022-08-10 12:19 UTC (permalink / raw) To: Mathieu Desnoyers Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, kvmarm On 8/10/22 14:17, Mathieu Desnoyers wrote: > Indeed, this hack seems to be a good approach to immediately fix things without > moving around all source files and headers. In the longer term, I'd prefer Sean's > proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). > This can be done in a follow up phase though. I'll put a note on my todo list > for after I come back from vacation. Great, Gavin, are you going to repost using librseq? >> Yeah, rseq_test should reuse librseq code. The simplest way, >> if slightly hackish, is to do something like >> >> diff --git a/tools/testing/selftests/kvm/Makefile >> b/tools/testing/selftests/kvm/Makefile >> index 690b499c3471..6c192b0ec304 100644 >> --- a/tools/testing/selftests/kvm/Makefile >> +++ b/tools/testing/selftests/kvm/Makefile >> @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) >> UNAME_M := riscv >> endif >> >> LIBKVM += lib/assert.c >> LIBKVM += lib/elf.c >> LIBKVM += lib/guest_modes.c >> @@ -198,7 +199,7 @@ endif >> CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ >> -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ >> -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ >> - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) >> + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq >> >> no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ >> $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) >> >> >> and just #include "../rseq/rseq.c" in rseq_test.c. Paolo _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 2022-08-10 12:19 ` Paolo Bonzini @ 2022-08-10 23:34 ` Gavin Shan 0 siblings, 0 replies; 25+ messages in thread From: Gavin Shan @ 2022-08-10 23:34 UTC (permalink / raw) To: Paolo Bonzini, Mathieu Desnoyers Cc: Florian Weimer, shan gavin, KVM list, maz, linux-kernel, andrew jones, yihyu, linux-kselftest, kvmarm Hi Paolo and Mathieu, On 8/10/22 10:19 PM, Paolo Bonzini wrote: > On 8/10/22 14:17, Mathieu Desnoyers wrote: >> Indeed, this hack seems to be a good approach to immediately fix things without >> moving around all source files and headers. In the longer term, I'd prefer Sean's >> proposal to move rseq.c to tools/lib/ (and to move rseq headers to tools/include/rseq/). >> This can be done in a follow up phase though. I'll put a note on my todo list >> for after I come back from vacation. > > Great, Gavin, are you going to repost using librseq? > It seems you've merged v2. I will post additional patches to use tools/lib/librseq.so, depending on what Mathieu will have. Mathieu, Please let me know if there are anything I can help. >>> Yeah, rseq_test should reuse librseq code. The simplest way, >>> if slightly hackish, is to do something like >>> >>> diff --git a/tools/testing/selftests/kvm/Makefile >>> b/tools/testing/selftests/kvm/Makefile >>> index 690b499c3471..6c192b0ec304 100644 >>> --- a/tools/testing/selftests/kvm/Makefile >>> +++ b/tools/testing/selftests/kvm/Makefile >>> @@ -37,6 +37,7 @@ ifeq ($(ARCH),riscv) >>> UNAME_M := riscv >>> endif >>> >>> LIBKVM += lib/assert.c >>> LIBKVM += lib/elf.c >>> LIBKVM += lib/guest_modes.c >>> @@ -198,7 +199,7 @@ endif >>> CFLAGS += -Wall -Wstrict-prototypes -Wuninitialized -O2 -g -std=gnu99 \ >>> -fno-stack-protector -fno-PIE -I$(LINUX_TOOL_INCLUDE) \ >>> -I$(LINUX_TOOL_ARCH_INCLUDE) -I$(LINUX_HDR_PATH) -Iinclude \ >>> - -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) >>> + -I$(<D) -Iinclude/$(UNAME_M) -I.. $(EXTRA_CFLAGS) $(KHDR_INCLUDES) -I../rseq >>> >>> no-pie-option := $(call try-run, echo 'int main() { return 0; }' | \ >>> $(CC) -Werror -no-pie -x c - -o "$$TMP", -no-pie) >>> >>> >>> and just #include "../rseq/rseq.c" in rseq_test.c. Thanks, Gavin _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 6:06 [PATCH 0/2] kvm/selftests: Two rseq_test fixes Gavin Shan 2022-08-09 6:06 ` [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 Gavin Shan @ 2022-08-09 6:06 ` Gavin Shan 2022-08-09 6:35 ` Florian Weimer 1 sibling, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-09 6:06 UTC (permalink / raw) To: kvmarm Cc: fweimer, shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini 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(-) 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); -- 2.23.0 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 6:06 ` [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test Gavin Shan @ 2022-08-09 6:35 ` Florian Weimer 2022-08-09 7:17 ` Florian Weimer 0 siblings, 1 reply; 25+ messages in thread From: Florian Weimer @ 2022-08-09 6:35 UTC (permalink / raw) To: Gavin Shan Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm * 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 6:35 ` Florian Weimer @ 2022-08-09 7:17 ` Florian Weimer 2022-08-09 8:46 ` Gavin Shan 0 siblings, 1 reply; 25+ messages in thread From: Florian Weimer @ 2022-08-09 7:17 UTC (permalink / raw) To: Gavin Shan Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm * 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 7:17 ` Florian Weimer @ 2022-08-09 8:46 ` Gavin Shan 2022-08-09 20:53 ` Sean Christopherson 0 siblings, 1 reply; 25+ messages in thread From: Gavin Shan @ 2022-08-09 8:46 UTC (permalink / raw) To: Florian Weimer Cc: shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 8:46 ` Gavin Shan @ 2022-08-09 20:53 ` Sean Christopherson 2022-08-10 0:45 ` Gavin Shan 0 siblings, 1 reply; 25+ messages in thread From: Sean Christopherson @ 2022-08-09 20:53 UTC (permalink / raw) To: Gavin Shan Cc: Florian Weimer, shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm 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". _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test 2022-08-09 20:53 ` Sean Christopherson @ 2022-08-10 0:45 ` Gavin Shan 0 siblings, 0 replies; 25+ messages in thread From: Gavin Shan @ 2022-08-10 0:45 UTC (permalink / raw) To: Sean Christopherson Cc: Florian Weimer, shan.gavin, kvm, maz, linux-kernel, andrew.jones, mathieu.desnoyers, yihyu, linux-kselftest, pbonzini, kvmarm 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 _______________________________________________ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm ^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2022-08-10 23:53 UTC | newest] Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-09 6:06 [PATCH 0/2] kvm/selftests: Two rseq_test fixes Gavin Shan 2022-08-09 6:06 ` [PATCH 1/2] KVM: selftests: Make rseq compatible with glibc-2.35 Gavin Shan 2022-08-09 6:33 ` Florian Weimer 2022-08-09 8:45 ` Gavin Shan 2022-08-09 7:16 ` Florian Weimer 2022-08-09 9:27 ` Gavin Shan 2022-08-09 12:21 ` Mathieu Desnoyers 2022-08-09 13:44 ` Mathieu Desnoyers 2022-08-09 21:38 ` Sean Christopherson 2022-08-10 0:37 ` Gavin Shan 2022-08-10 12:29 ` Mathieu Desnoyers 2022-08-10 12:35 ` Paolo Bonzini 2022-08-10 12:13 ` Mathieu Desnoyers 2022-08-10 23:52 ` Gavin Shan 2022-08-10 9:14 ` Paolo Bonzini 2022-08-10 9:59 ` Gavin Shan 2022-08-10 12:17 ` Mathieu Desnoyers 2022-08-10 12:19 ` Paolo Bonzini 2022-08-10 23:34 ` Gavin Shan 2022-08-09 6:06 ` [PATCH 2/2] KVM: selftests: Use getcpu() instead of sched_getcpu() in rseq_test Gavin Shan 2022-08-09 6:35 ` Florian Weimer 2022-08-09 7:17 ` Florian Weimer 2022-08-09 8:46 ` Gavin Shan 2022-08-09 20:53 ` Sean Christopherson 2022-08-10 0:45 ` Gavin Shan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).