The idea here is to avoid a memory access and conditional branch per probe site. Instead, the probe is represented by an "unreachable" unconditional function call. asm goto is used to store the address of the probe site (represented by a no-op sled) and the address of the function call into a tracepoint record. Each SDT probe carries a list of tracepoints. When the probe is enabled, the no-op sled corresponding to each tracepoint is overwritten with a jmp to the corresponding label. The implementation uses smp_rendezvous() to park all other CPUs while the instruction is being overwritten, as this can't be done atomically in general. I verified that llvm 17 moves argument marshalling code and the sdt_probe() function call out-of-line, i.e., to the end of the function. Per gallatin@ in D43504, this approach has less overhead when probes are disabled. To make the implementation simpler, I removed support for probes with 7 arguments; nothing makes use of this except a regression test case. I also didn't implement this for 32-bit powerpc since I wasn't able to figure out how to boot it in QEMU. I have a couple of follow-up patches which take this further: 1. We can now fill out the "function" field of SDT probe names automatically, since we know exactly where each tracepoint is located. 2. We can put additional code between the asm goto target label and the probe itself. This lets us perform some probe-specific argument marshalling without any overhead when the probe is disabled. For example: ``` if (SDT_PROBES_ENABLED()) { int reason = CLD_EXITED; if (WCOREDUMP(signo)) reason = CLD_DUMPED; else if (WIFSIGNALED(signo)) reason = CLD_KILLED; SDT_PROBE1(proc, , , exit, reason); } ``` becomes ``` SDT_PROBE1_EXP(proc, , , exit, reason, int reason; reason = CLD_EXITED; if (WCOREDUMP(signo)) reason = CLD_DUMPED; else if (WIFSIGNALED(signo)) reason = CLD_KILLED; ); ``` In the future I would like to use this mechanism more generally, e.g., to remove branches and marshalling code used by hwpmc, and generally to make it easier to add new tracepoint consumers without having to add more conditional branches to hot code paths.
Details
- Reviewers
gnn andrew manu domagoj.stolfa_gmail.com avg - Group Reviewers
DTrace - Commits
- rGddf0ed09bd8f: sdt: Implement SDT probes using hot-patching
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 57496 Build 54384: arc lint + arc unit
Event Timeline
I ran the tests with this applied on amd64, had a few kmods load/unload concurrently for a while and looked through the concurrency around the patching code. Everything seems to work fine on my end.
sys/cddl/dev/sdt/sdt.c | ||
---|---|---|
374 | Might make sense for this to say: ... for %s:%s:%s:%s\n", ..., tp->probe->prov->name, tp->probe->mod, tp->probe->func, tp->probe->name); to avoid confusion? |
- Rebase.
- Remove license boilerplate, just keep SPDX and copyright lines.
- Fix a problem with DTRACE_PROBE which uses function-static probe structure definitions. To reference them from inline asm, we need to make the structure an input operand since we don't know the symbol name.
- Rename some constants to improve consistency.
- Work around a clang bug/limitation on i386 wherein I can't use the "i" constraint with a global variable for some reason. This works perfectly well if I just reference the symbol directly, so I'm not sure why the backend is rejecting it. Happily, there is an MD constraint ("Ws") which empirically has the behaviour I want.
Thanks! I'm more or less happy with the current version, so will commit in a day or two unless I hear some objection.
sys/conf/files.powerpc | ||
---|---|---|
339 | This machdep file looks fine to me for all powerpc, so I think it's fine to not gate on powerpc64. I don't see any changes in the MI files that would prevent powerpc, either (all atomics are ints, no 64-bit atomics). |
The thing that's holding me back right now is uncertainty around compiler support. asm goto is a reasonably recent feature in LLVM so this would break one's ability to compile GENERIC with older toolchains, but I'm not sure yet whether that's likely to inconvenience anyone. I could provide a fallback implementation, but I'd rather not if it can be avoided, sdt.h is too messy as it is.
sys/conf/files.powerpc | ||
---|---|---|
339 | Hmm, so the instructions written by that file have the same encoding on 32-bit powerpc as well? I still have not yet tried to test this on 32-bit powerpc, too busy with other stuff. |
sys/conf/files.powerpc | ||
---|---|---|
339 | Yeah, encodings are the same for all instructions between 32 and 64-bit. 32-bit is a strict subset of 64-bit ISA, and instructions affect the entire GPR in both 32-bit and 64-bit mode. |
It's been around since LLVM 9, which is longer than I had thought. So this isn't a blocker.