Page MenuHomeFreeBSD

sdt: Prototype implementation of SDT probes using hot-patching
AcceptedPublic

Authored by markj on Mar 23 2024, 6:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, May 20, 7:40 PM
Unknown Object (File)
Sun, May 19, 5:58 AM
Unknown Object (File)
Sat, May 18, 9:43 PM
Unknown Object (File)
Sat, May 18, 9:43 PM
Unknown Object (File)
Sat, May 18, 9:43 PM
Unknown Object (File)
Sat, May 18, 9:43 PM
Unknown Object (File)
Fri, May 17, 8:10 PM
Unknown Object (File)
Fri, May 17, 8:02 PM

Details

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

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

markj requested review of this revision.Mar 23 2024, 6:49 AM

Provide a full implementation.

markj added a reviewer: DTrace.

Any feedback from DTrace? I would like to commit this soon.

This revision is now accepted and ready to land.Apr 20 2024, 4:28 PM

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?

markj edited the summary of this revision. (Show Details)
  • 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.
This revision now requires review to proceed.Wed, May 1, 12:17 PM

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.

Thanks! I'm more or less happy with the current version, so will commit in a day or two unless I hear some objection.

Apply Domagoj's suggestion

This revision is now accepted and ready to land.Thu, May 2, 1:37 PM
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.