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
Details
- Reviewers
gnn andrew manu domagoj.stolfa_gmail.com - Group Reviewers
DTrace
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 57499 Build 54387: arc lint + arc unit
Event Timeline
Comment Actions
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? |
Comment Actions
- 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.
Comment Actions
Thanks! I'm more or less happy with the current version, so will commit in a day or two unless I hear some objection.