[DTrace-devel] [PATCH v2 10/19] Remove the is-enabled provider
Kris Van Hees
kris.van.hees at oracle.com
Sat Oct 26 01:51:27 UTC 2024
On Fri, Oct 25, 2024 at 09:16:19PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
>
> The trampoline for the is-enabled provider is unnecessarily complicated.
> We do not need dt_cg_tramp_copy_regs() since the copied values will not
> be used. Nor do we need the (second) copy of regs[arg0] to mst->arg[0].
> We can inline copyout_val().
>
> Actually, we can simply consolidate the USDT and is-enabled providers.
>
> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_prov_uprobe.c | 165 ++++-------------------
> test/unittest/usdt/tst.enable_pid.r | 45 +++++++
> test/unittest/usdt/tst.enable_pid.r.p | 7 +
> test/unittest/usdt/tst.enable_pid.sh | 183 ++++++++++++++++++++++++++
> 4 files changed, 261 insertions(+), 139 deletions(-)
> create mode 100644 test/unittest/usdt/tst.enable_pid.r
> create mode 100755 test/unittest/usdt/tst.enable_pid.r.p
> create mode 100755 test/unittest/usdt/tst.enable_pid.sh
>
> diff --git a/libdtrace/dt_prov_uprobe.c b/libdtrace/dt_prov_uprobe.c
> index 54e115f51..dcec76956 100644
> --- a/libdtrace/dt_prov_uprobe.c
> +++ b/libdtrace/dt_prov_uprobe.c
> @@ -43,7 +43,6 @@
>
> /* Provider name for the underlying probes. */
> static const char prvname[] = "uprobe";
> -static const char prvname_is_enabled[] = "uprobe__is_enabled";
>
> #define PP_IS_RETURN 1
> #define PP_IS_FUNCALL 2
> @@ -77,7 +76,6 @@ static const dtrace_pattr_t pattr = {
> { DTRACE_STABILITY_PRIVATE, DTRACE_STABILITY_PRIVATE, DTRACE_CLASS_UNKNOWN },
> };
>
> -dt_provimpl_t dt_uprobe_is_enabled;
> dt_provimpl_t dt_pid;
> dt_provimpl_t dt_usdt;
>
> @@ -85,8 +83,6 @@ static int populate(dtrace_hdl_t *dtp)
> {
> if (dt_provider_create(dtp, dt_uprobe.name, &dt_uprobe, &pattr,
> NULL) == NULL ||
> - dt_provider_create(dtp, dt_uprobe_is_enabled.name,
> - &dt_uprobe_is_enabled, &pattr, NULL) == NULL ||
> dt_provider_create(dtp, dt_pid.name, &dt_pid, &pattr,
> NULL) == NULL)
> return -1; /* errno already set */
> @@ -464,7 +460,6 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> dtrace_probedesc_t pd;
> dt_probe_t *uprp;
> dt_uprobe_t *upp;
> - int is_enabled = 0;
>
> /*
> * The underlying probes (uprobes) represent the tracepoints that pid
> @@ -487,8 +482,6 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> strcpy(prb, "return");
> break;
> case DTPPT_IS_ENABLED:
> - is_enabled = 1;
> - /* Fallthrough. */
> case DTPPT_ENTRY:
> case DTPPT_OFFSETS:
> snprintf(prb, sizeof(prb), "%lx", psp->pps_off);
> @@ -499,7 +492,7 @@ static dt_probe_t *create_underlying(dtrace_hdl_t *dtp,
> }
>
> pd.id = DTRACE_IDNONE;
> - pd.prv = is_enabled ? prvname_is_enabled : prvname;
> + pd.prv = prvname;
> pd.mod = mod;
> pd.fun = psp->pps_fun;
> pd.prb = prb;
> @@ -691,21 +684,6 @@ static void enable(dtrace_hdl_t *dtp, dt_probe_t *prp, int is_usdt)
> dt_probe_enable(dtp, uprp);
> }
>
> - /*
> - * If necessary, we need to enable is-enabled probes too (if they
> - * exist).
> - */
> - if (is_usdt) {
> - dtrace_probedesc_t pd;
> - dt_probe_t *iep;
> -
> - memcpy(&pd, &prp->desc, sizeof(pd));
> - pd.prv = prvname_is_enabled;
> - iep = dt_probe_lookup(dtp, &pd);
> - if (iep != NULL)
> - dt_probe_enable(dtp, iep);
> - }
> -
> /*
> * Finally, ensure we're in the list of enablings as well.
> * (This ensures that, among other things, the probes map
> @@ -850,6 +828,29 @@ static int trampoline(dt_pcb_t *pcb, uint_t exitlbl)
> emit(dlp, BPF_CALL_HELPER(BPF_FUNC_map_lookup_elem));
> emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_exit));
>
> + if (upp->flags & PP_IS_ENABLED) {
> + /*
> + * Generate a BPF trampoline for an is-enabled probe. The is-enabled probe
> + * prototype looks like:
> + *
> + * int is_enabled(int *arg)
> + *
> + * The trampoline writes 1 into the location pointed to by the passed-in arg.
> + */
> + emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_FP, DT_TRAMP_SP_SLOT(0), 1));
> + emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_8, PT_REGS_ARG0));
> + emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_FP));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DT_TRAMP_SP_SLOT(0)));
> + emit(dlp, BPF_MOV_IMM(BPF_REG_3, sizeof(uint32_t)));
> + emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_write_user));
> +
> + goto out;
> + }
> +
> + /*
> + * Continue with normal USDT probes.
> + */
> +
> /* Read the PRID from the table lookup and store to mst->prid. */
> emit(dlp, BPF_LOAD(BPF_W, BPF_REG_1, BPF_REG_0, 0));
> emit(dlp, BPF_STORE(BPF_W, BPF_REG_7, DMST_PRID, BPF_REG_1));
> @@ -906,110 +907,11 @@ out:
> return 0;
> }
>
> -/*
> - * Copy the given immediate value into the address given by the specified probe
> - * argument.
> - */
> -static void
> -copyout_val(dt_pcb_t *pcb, uint_t lbl, uint32_t val, int arg)
> -{
> - dt_regset_t *drp = pcb->pcb_regs;
> - dt_irlist_t *dlp = &pcb->pcb_ir;
> -
> - emitl(dlp, lbl, BPF_STORE_IMM(BPF_DW, BPF_REG_FP, DT_TRAMP_SP_SLOT(0),
> - val));
> -
> - if (dt_regset_xalloc_args(drp) == -1)
> - longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_1, BPF_REG_7, DMST_ARG(arg)));
> - emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_FP));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, DT_TRAMP_SP_SLOT(0)));
> - emit(dlp, BPF_MOV_IMM(BPF_REG_3, sizeof(uint32_t)));
> - dt_regset_xalloc(drp, BPF_REG_0);
> - emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_write_user));
> -
> - /* XXX any point error-checking here? What can we possibly do? */
> - dt_regset_free(drp, BPF_REG_0);
> - dt_regset_free_args(drp);
> -}
> -
> -/*
> - * Generate a BPF trampoline for an is-enabled probe. The is-enabled probe
> - * prototype looks like:
> - *
> - * int is_enabled(int *arg)
> - *
> - * The trampoline dereferences the passed-in arg and writes 1 into it if this is
> - * one of the processes for which the probe is enabled.
> - */
> -static int trampoline_is_enabled(dt_pcb_t *pcb, uint_t exitlbl)
> -{
> - dt_irlist_t *dlp = &pcb->pcb_ir;
> - const dt_probe_t *uprp = pcb->pcb_probe;
> - dt_ident_t *usdt_prids = dt_dlib_get_map(pcb->pcb_hdl, "usdt_prids");
> -
> - assert(usdt_prids != NULL);
> -
> - dt_cg_tramp_prologue(pcb);
> -
> - /*
> - * After the dt_cg_tramp_prologue() call, we have:
> - * // (%r7 = dctx->mst)
> - * // (%r8 = dctx->ctx)
> - */
> - dt_cg_tramp_copy_regs(pcb);
> -
> - /*
> - * Copy in the first function argument, a pointer value to which
> - * the is-enabled state of the probe will be written (necessarily
> - * 1 if this probe is running at all).
> - */
> - emit(dlp, BPF_LOAD(BPF_DW, BPF_REG_0, BPF_REG_8, PT_REGS_ARG0));
> - emit(dlp, BPF_STORE(BPF_DW, BPF_REG_7, DMST_ARG(0), BPF_REG_0));
> -
> - /*
> - * Retrieve the PID of the process that caused the probe to fire.
> - */
> - emit(dlp, BPF_CALL_HELPER(BPF_FUNC_get_current_pid_tgid));
> - emit(dlp, BPF_ALU64_IMM(BPF_RSH, BPF_REG_0, 32));
> -
> - /*
> - * Look up in the BPF 'usdt_prids' map. Space for the look-up key
> - * will be used on the BPF stack:
> - *
> - * offset value
> - *
> - * -sizeof(usdt_prids_map_key_t) pid (in %r0)
> - *
> - * -sizeof(usdt_prids_map_key_t) + sizeof(pid_t)
> - * ==
> - * -sizeof(dtrace_id_t) underlying-probe prid
> - */
> - emit(dlp, BPF_STORE(BPF_W, BPF_REG_9, (int)(-sizeof(usdt_prids_map_key_t)), BPF_REG_0));
> - emit(dlp, BPF_STORE_IMM(BPF_W, BPF_REG_9, (int)(-sizeof(dtrace_id_t)), uprp->desc->id));
> - dt_cg_xsetx(dlp, usdt_prids, DT_LBL_NONE, BPF_REG_1, usdt_prids->di_id);
> - emit(dlp, BPF_MOV_REG(BPF_REG_2, BPF_REG_9));
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, (int)(-sizeof(usdt_prids_map_key_t))));
> - emit(dlp, BPF_CALL_HELPER(BPF_FUNC_map_lookup_elem));
> - emit(dlp, BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, pcb->pcb_exitlbl));
> -
> - /*
> - * If we succeeded, then we use copyout_val() above to assign:
> - * *arg0 = 1;
> - */
> - copyout_val(pcb, DT_LBL_NONE, 1, 0);
> -
> - dt_cg_tramp_return(pcb);
> -
> - return 0;
> -}
> -
> static char *uprobe_name(dev_t dev, ino_t ino, uint64_t addr, int flags)
> {
> char *name;
>
> - if (asprintf(&name, "dt_pid%s/%c_%llx_%llx_%lx",
> - flags & PP_IS_ENABLED ? "_is_enabled" : "",
> + if (asprintf(&name, "dt_pid/%c_%llx_%llx_%lx",
> flags & PP_IS_RETURN ? 'r' : 'p', (unsigned long long)dev,
> (unsigned long long)ino, (unsigned long)addr) < 0)
> return NULL;
> @@ -1019,7 +921,7 @@ static char *uprobe_name(dev_t dev, ino_t ino, uint64_t addr, int flags)
>
> /*
> * Create a uprobe for a given dev/ino, mapping filename, and address: the
> - * uprobe may be a uretprobe or an is-enabled probe. Return the probe's name as
> + * uprobe may be a uretprobe. Return the probe's name as
> * a new dynamically-allocated string, or NULL on error.
> */
> static char *uprobe_create(dev_t dev, ino_t ino, const char *mapping_fn,
> @@ -1181,21 +1083,6 @@ dt_provimpl_t dt_uprobe = {
> .add_probe = &add_probe_uprobe,
> };
>
> -/*
> - * Used for underlying is-enabled uprobes.
> - */
> -dt_provimpl_t dt_uprobe_is_enabled = {
> - .name = prvname_is_enabled,
> - .prog_type = BPF_PROG_TYPE_KPROBE,
> - .populate = &populate,
> - .load_prog = &dt_bpf_prog_load,
> - .trampoline = &trampoline_is_enabled,
> - .attach = &attach,
> - .probe_info = &probe_info,
> - .detach = &detach,
> - .probe_destroy = &probe_destroy_underlying,
> -};
> -
> /*
> * Used for pid probes.
> */
> diff --git a/test/unittest/usdt/tst.enable_pid.r b/test/unittest/usdt/tst.enable_pid.r
> new file mode 100644
> index 000000000..675fcdd6f
> --- /dev/null
> +++ b/test/unittest/usdt/tst.enable_pid.r
> @@ -0,0 +1,45 @@
> + FUNCTION:NAME
> + :tick-1s
> +
> + FUNCTION:NAME
> + :tick-1s
> +
> + FUNCTION:NAME
> + :tick-1s
> +
> + FUNCTION:NAME
> + :tick-1s
> +
> +done
> +========== out 1
> +is not enabled
> +=== epoch ===
> +is not enabled
> +is enabled
> +is not enabled
> +=== epoch ===
> +is not enabled
> +=== epoch ===
> +is not enabled
> +is enabled
> +is not enabled
> +=== epoch ===
> +========== out 2
> +is not enabled
> +=== epoch ===
> +is not enabled
> +=== epoch ===
> +is not enabled
> +is enabled
> +is not enabled
> +=== epoch ===
> +is not enabled
> +is enabled
> +is not enabled
> +=== epoch ===
> +success
> +-- @@stderr --
> +dtrace: description 'test_provNNN:::go ' matched 1 probe
> +dtrace: description 'test_provNNN:::go ' matched 2 probes
> +dtrace: description 'test_provNNN:::go ' matched 2 probes
> +dtrace: description 'test_prov*:::go ' matched 3 probes
> diff --git a/test/unittest/usdt/tst.enable_pid.r.p b/test/unittest/usdt/tst.enable_pid.r.p
> new file mode 100755
> index 000000000..baf9d2a90
> --- /dev/null
> +++ b/test/unittest/usdt/tst.enable_pid.r.p
> @@ -0,0 +1,7 @@
> +#!/usr/bin/awk -f
> +{
> + # ignore the specific process ID
> + gsub(/test_prov[0-9]+/, "test_provNNN");
> +
> + print;
> +}
> diff --git a/test/unittest/usdt/tst.enable_pid.sh b/test/unittest/usdt/tst.enable_pid.sh
> new file mode 100755
> index 000000000..91923014d
> --- /dev/null
> +++ b/test/unittest/usdt/tst.enable_pid.sh
> @@ -0,0 +1,183 @@
> +#!/bin/bash
> +#
> +# Oracle Linux DTrace.
> +# Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.
> +# Licensed under the Universal Permissive License v 1.0 as shown at
> +# http://oss.oracle.com/licenses/upl.
> +#
> +# @@timeout: 80
> +
> +PATH=/usr/bin:/usr/sbin:$PATH
> +
> +#
> +# In this test, we check that is-enabled probes depend correctly on pid.
> +#
> +
> +dtrace=$1
> +CC=/usr/bin/gcc
> +CFLAGS=
> +
> +DIRNAME="$tmpdir/usdt-enable_pid.$$.$RANDOM"
> +mkdir -p $DIRNAME
> +cd $DIRNAME
> +
> +#
> +# Set up the source files.
> +#
> +
> +cat > prov.d <<EOF
> +provider test_prov {
> + probe go();
> +};
> +EOF
> +
> +cat > main.c <<EOF
> +#include <signal.h>
> +#include <stdio.h>
> +#include "prov.h"
> +
> +/* We check if the is-enabled probe is or is not enabled (or unknown). */
> +#define ENABLED_IS 1
> +#define ENABLED_NOT 2
> +#define ENABLED_UNK 3
> +
> +/* Start with the previous probe "unknown". */
> +int prv = ENABLED_UNK;
> +long long num = 0;
> +
> +/* Report how many times the previous case was encountered. */
> +static void report() {
> +
> + /* Skip if there is nothing to report. */
> + if (num == 0)
> + return;
> +
> + switch (prv) {
> + case ENABLED_IS:
> + printf("is enabled\n");
> + break;
> + case ENABLED_NOT:
> + printf("is not enabled\n");
> + break;
> + }
> + fflush(stdout);
> +
> + /* Reset. */
> + prv = ENABLED_UNK;
> + num = 0;
> +}
> +
> +/* When USR1 is received, mark an "epoch" in the output. */
> +static void mark_epoch(int sig) {
> + report();
> + printf("=== epoch ===\n");
> + fflush(stdout);
> +}
> +
> +int
> +main(int argc, char **argv)
> +{
> + struct sigaction act;
> +
> + /* Set USR1 to mark epochs. */
> + act.sa_flags = 0;
> + act.sa_handler = mark_epoch;
> + if (sigaction(SIGUSR1, &act, NULL)) {
> + printf("set handler failed\n");
> + return 1;
> + }
> +
> + /* Just keep looping, counting consecutive cases. */
> + while (1) {
> + int now;
> +
> + /* Check the is-enabled probe. */
> + if (TEST_PROV_GO_ENABLED()) {
> + now = ENABLED_IS;
> + } else {
> + now = ENABLED_NOT;
> + }
> +
> + /* Compare to the previous case. */
> + if (now == prv) {
> + num++;
> + } else {
> + report(); /* resets num */
> + prv = now;
> + }
> + }
> +
> + return 0;
> +}
> +EOF
> +
> +#
> +# Build the test program.
> +#
> +
> +$dtrace -h -s prov.d
> +if [ $? -ne 0 ]; then
> + echo "failed to generate header file" >&2
> + exit 1
> +fi
> +cc $test_cppflags -c main.c
> +if [ $? -ne 0 ]; then
> + echo "failed to compile test" >&2
> + exit 1
> +fi
> +$dtrace -G -64 -s prov.d main.o
> +if [ $? -ne 0 ]; then
> + echo "failed to create DOF" >&2
> + exit 1
> +fi
> +cc $test_cppflags -o main main.o prov.o
> +if [ $? -ne 0 ]; then
> + echo "failed to link final executable" >&2
> + exit 1
> +fi
> +
> +#
> +# Start two copies.
> +#
> +
> +./main >& out.1 &
> +pid1=$!
> +./main >& out.2 &
> +pid2=$!
> +
> +#
> +# Run DTrace with different pid probes, each case is its own "epoch":
> +# pid1? pid2?
> +# - 1 no no
> +# - $pid1 YES no
> +# - $pid2 no YES
> +# - * YES YES
> +#
> +
> +for pid in 1 $pid1 $pid2 '*'; do
> + sleep 1
> + $dtrace $dt_flags -Zn 'test_prov'$pid':::go { trace("hi"); }
> + tick-1s { exit(0) }'
> + if [ $? -ne 0 ]; then
> + echo ERROR: dtrace
> + kill -TERM $pid1
> + kill -TERM $pid2
> + exit 1
> + fi
> + sleep 1
> +
> + # Use USR1 to mark epochs in the output.
> + kill -USR1 $pid1
> + kill -USR1 $pid2
> +done
> +
> +echo done
> +echo "========== out 1"; cat out.1
> +echo "========== out 2"; cat out.2
> +
> +echo success
> +
> +kill -TERM $pid1
> +kill -TERM $pid2
> +
> +exit 0
> --
> 2.43.5
>
>
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel
More information about the DTrace-devel
mailing list