----- 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