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)
Thu, May 9, 1:03 AM
Unknown Object (File)
Tue, May 7, 3:33 AM
Unknown Object (File)
Mon, May 6, 5:42 AM
Unknown Object (File)
Sun, May 5, 5:19 AM
Unknown Object (File)
Wed, May 1, 12:25 PM
Unknown Object (File)
Sat, Apr 27, 10:49 PM
Unknown Object (File)
Sat, Apr 27, 4:07 AM
Unknown Object (File)
Fri, Apr 26, 4:27 AM

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.Sat, Apr 20, 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