[DTrace-devel] [PATCH v2 6/6] usdt: provide all underlying probes in a pid once per pid
Kris Van Hees
kris.van.hees at oracle.com
Thu May 18 14:04:40 UTC 2023
On Wed, May 17, 2023 at 08:53:03PM +0100, Nick Alcock via DTrace-devel wrote:
> Another bug report revealed that USDT probes were sometimes failing to
> be provided when there were multiple probes in the D script. The
> underlying cause of this is that dt_pid_create_usdt_probes() gets called
> once per dt_setcontext() of a USDT probe, but every call checks each
> underlying probe to see if it matches the overlying probe we're trying
> to provide, provides it if so, and then calls dt_pid_fix_mod() on that
> underlying probe... which changes the module name, and now it no longer
> matches in subsequent calls.
>
> Since these are in any case *underlying* probes, and thus kept disabled
> unless an overlying probe enables them, there's no reason not to just
> provide all underlying probes in a pid the first time
> dt_pid_create_usdt_probes() is called for a given pid, and pay no
> attention to the probe description passed down from dt_setcontext() at
> all.
>
> The existing change whereby all DTrace uprobes in the system are
> remembered on the first call to dt_pid_create_usdt_probes() is still
> beneficial and is retained: this just simplifies things and speeds them
> up even more. As a side benefit this means that some wildcard USDT
> probes now work: just provider wildcards, not other wildcards, but
> that's better than nothing.
>
> We soup up test/unittest/usdt/tst.multiple.sh to identify this failure
> (which means not using -p or -c, since those trigger additional uprobe
> scans in the old implementation and catch more probes by accident, and
> also probing multiple different probes as well as one probe with
> multiple loci; we also add a probe that isn't probed just to make sure
> that non-enabled underlying probes really don't fire).
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_pid.c | 94 ++++++++---------------------
> libdtrace/dt_proc.h | 1 +
> test/unittest/pid/tst.provregex3.sh | 4 +-
> test/unittest/usdt/tst.multiple.r | 2 +
> test/unittest/usdt/tst.multiple.sh | 26 +++++++-
> 5 files changed, 52 insertions(+), 75 deletions(-)
>
> diff --git a/libdtrace/dt_pid.c b/libdtrace/dt_pid.c
> index 627587b6bb51b..b0689aa8a4955 100644
> --- a/libdtrace/dt_pid.c
> +++ b/libdtrace/dt_pid.c
> @@ -51,8 +51,6 @@ typedef struct dt_pid_probe {
> uint_t dpp_last_taken;
> } dt_pid_probe_t;
>
> -static char *dt_pid_de_pid(char *buf, const char *prv);
> -
> /*
> * Compose the lmid and object name into the canonical representation. We
> * omit the lmid for the default link map for convenience.
> @@ -761,15 +759,11 @@ out:
> * If dpr is set, just set up probes relating to mappings found in that one
> * process. (dpr must in this case be locked.)
> *
> - * If pdp is set, create overlying USDT probes for the specified probe
> - * description.
> - *
> * Return 0 on success or -1 on error. (Failure to create specific underlying
> * probes is not an error.)
> */
> static int
> -dt_pid_create_usdt_probes(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> - dtrace_probedesc_t *pdp, dt_pcb_t *pcb)
> +dt_pid_create_usdt_probes(dtrace_hdl_t *dtp, dt_proc_t *dpr, dt_pcb_t *pcb)
> {
> const dt_provider_t *pvp;
> size_t i;
> @@ -823,56 +817,24 @@ dt_pid_create_usdt_probes(dtrace_hdl_t *dtp, dt_proc_t *dpr,
> }
>
> /*
> - * Does this match the overlying probe we are meant to be
> - * creating? If so, create an overlying probe and let
> - * provide_probe() create the underlying probe for us. Among
> - * other things, this ensures that the PID part of the provider
> - * name gets stuck into the overlying probe properly.
> + * Create an underlying probe using psp, if not already present.
> *
> - * TODO: wildcard probes get handled here.
> + * Complain if any probe cannot be created: at this stage we
> + * cannot reliably tell whether a corresponding overlying probe
> + * will be created (since dt_setcontext only calls us for the
> + * first one in any given provider).
> */
> - if (pdp) {
> - char de_pid_prov[DTRACE_PROVNAMELEN + 1];
> -
> - if ((pdp->prv[0] != 0 &&
> - strcmp(dt_pid_de_pid(de_pid_prov, pdp->prv),
> - psp->pps_prv) != 0) ||
> - (pdp->mod[0] != 0 && strcmp(pdp->mod, psp->pps_mod) != 0) ||
> - (pdp->fun[0] != 0 && strcmp(pdp->fun, psp->pps_fun) != 0) ||
> - (pdp->prb[0] != 0 && strcmp(pdp->prb, psp->pps_prb) != 0)) {
> - dt_dprintf("%s:%s:%s:%s -> %s:%s:%s:%s: match failure\n",
> - pdp->prv, pdp->mod, pdp->fun, pdp->prb,
> - psp->pps_prv, psp->pps_mod, psp->pps_fun,
> - psp->pps_prb);
> - continue;
> - }
> - }
>
> - /*
> - * Create a probe using psp, if not already present.
> - *
> - * If we are creating an overlying probe, complain about it if
> - * probe creation fails. Otherwise, this is just an underlying
> - * probe: we'll complain later if we use it for anything.
> - */
> -
> - dt_dprintf("providing %s:%s:%s:%s\n", pdp->prv, pdp->mod,
> - pdp->fun, pdp->prb);
> - if (pvp->impl->provide_probe(dtp, psp) < 0 && pdp) {
> + dt_dprintf("providing %s:%s:%s:%s\n", psp->pps_prv, psp->pps_mod,
> + psp->pps_fun, psp->pps_prb);
> + if (pvp->impl->provide_probe(dtp, psp) < 0) {
> dt_pid_error(dtp, pcb, dpr, D_PROC_USDT,
> "failed to instantiate %sprobe %s for pid %d: %s",
> psp->pps_type == DTPPT_IS_ENABLED ?
> - "is-enabled ": "", pdp->prb, dpr->dpr_pid,
> + "is-enabled ": "", psp->pps_prb, dpr->dpr_pid,
> dtrace_errmsg(dtp, dtrace_errno(dtp)));
> ret = -1;
> }
> -
> - if (pdp && dpr) {
> - /*
> - * Put the module name in its canonical form.
> - */
> - dt_pid_fix_mod(NULL, pdp, dtp, dpr->dpr_pid);
> - }
> }
>
> return ret;
> @@ -970,23 +932,6 @@ dt_pid_get_pid(const dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_t *pcb,
> return pid;
> }
>
> -/*
> - * Return a USDT provider name with the pid removed.
> - */
> -static char *
> -dt_pid_de_pid(char *buf, const char *prv)
> -{
> - const char *c;
> - char *p = buf;
> -
> - for (c = prv; *c != '\0'; c++) {
> - if (!isdigit(*c))
> - *p++ = *c;
> - }
> - *p = 0;
> - return buf;
> -}
> -
> int
> dt_pid_create_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_t *pcb)
> {
> @@ -1029,7 +974,15 @@ dt_pid_create_probes(dtrace_probedesc_t *pdp, dtrace_hdl_t *dtp, dt_pcb_t *pcb)
> dpr = dt_proc_lookup(dtp, pid);
> assert(dpr != NULL);
>
> - err = dt_pid_create_usdt_probes(dtp, dpr, pdp, pcb);
> + if (!dpr->dpr_usdt) {
> + err = dt_pid_create_usdt_probes(dtp, dpr, pcb);
> + dpr->dpr_usdt = B_TRUE;
> + }
> +
> + /*
> + * Put the module name in its canonical form.
> + */
> + dt_pid_fix_mod(NULL, pdp, dtp, dpr->dpr_pid);
>
> dt_proc_release_unlock(dtp, pid);
> }
> @@ -1073,9 +1026,12 @@ dt_pid_create_probes_module(dtrace_hdl_t *dtp, dt_proc_t *dpr)
> * If it's not strictly a pid provider, we might match
> * a USDT provider.
> */
> - if (strcmp(provname, pdp->prv) != 0 &&
> - dt_pid_create_usdt_probes(dtp, dpr, pdp, NULL) < 0)
> - ret = 1;
> + if (strcmp(provname, pdp->prv) != 0) {
> + if (dt_pid_create_usdt_probes(dtp, dpr, NULL) < 0)
> + ret = 1;
> + else
> + dt_pid_fix_mod(NULL, pdp, dtp, dpr->dpr_pid);
> + }
>
> free((char *)pd.fun);
> }
> diff --git a/libdtrace/dt_proc.h b/libdtrace/dt_proc.h
> index 0ec1caf08fbfa..128003ede10bd 100644
> --- a/libdtrace/dt_proc.h
> +++ b/libdtrace/dt_proc.h
> @@ -39,6 +39,7 @@ typedef struct dt_proc {
> uint_t dpr_refs; /* reference count */
> uint8_t dpr_stop; /* stop mask: see flag bits below */
> uint8_t dpr_done; /* done flag: ctl thread has exited */
> + uint8_t dpr_usdt; /* usdt flag: usdt probes created */
> uint8_t dpr_created; /* proc flag: true if we created this
> process, false if we grabbed it */
> uint8_t dpr_monitoring; /* true if we should background-monitor
> diff --git a/test/unittest/pid/tst.provregex3.sh b/test/unittest/pid/tst.provregex3.sh
> index 89ca4aec46a5b..6e1afd3e493ee 100755
> --- a/test/unittest/pid/tst.provregex3.sh
> +++ b/test/unittest/pid/tst.provregex3.sh
> @@ -1,15 +1,13 @@
> #!/bin/bash
> #
> # Oracle Linux DTrace.
> -# Copyright (c) 2008, 2020, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2008, 2023, 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.
> #
> # This test verifies that a regex in the provider name will match
> # USDT probes as well as pid probes (e.g., p*d$target matches both
> # pid$target and pyramid$target.)
> -#
> -# @@xfail: dtv2 (wildcarded usdt probes not yet implemented)
>
> if [ $# != 1 ]; then
> echo expected one argument: '<'dtrace-path'>'
> diff --git a/test/unittest/usdt/tst.multiple.r b/test/unittest/usdt/tst.multiple.r
> index 5e826192fed19..ab9cf62d77196 100644
> --- a/test/unittest/usdt/tst.multiple.r
> +++ b/test/unittest/usdt/tst.multiple.r
> @@ -2,4 +2,6 @@ test:main:go
> test:main:go
> test:main:go
> test:main:go
> +test:main:stop
> +test:main:stop
>
> diff --git a/test/unittest/usdt/tst.multiple.sh b/test/unittest/usdt/tst.multiple.sh
> index eb0a7ccf5e4d3..b31e8eff705d3 100755
> --- a/test/unittest/usdt/tst.multiple.sh
> +++ b/test/unittest/usdt/tst.multiple.sh
> @@ -1,7 +1,7 @@
> #!/bin/bash
> #
> # Oracle Linux DTrace.
> -# Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.
> +# Copyright (c) 2006, 2023, 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.
> #
> @@ -21,6 +21,8 @@ cd $DIRNAME
> cat > prov.d <<EOF
> provider test_prov {
> probe go();
> + probe stop();
> + probe ignore();
> };
> EOF
>
> @@ -32,15 +34,20 @@ fi
>
> cat > test.c <<EOF
> #include <sys/types.h>
> +#include <unistd.h>
> #include "prov.h"
>
> int
> main(int argc, char **argv)
> {
> + sleep(5); /* Until proper synchronization is implemented. */
> TEST_PROV_GO();
> TEST_PROV_GO();
> TEST_PROV_GO();
> TEST_PROV_GO();
> + TEST_PROV_IGNORE();
> + TEST_PROV_STOP();
> + TEST_PROV_STOP();
>
> return 0;
> }
> @@ -63,11 +70,24 @@ if [ $? -ne 0 ]; then
> fi
>
> script() {
> - $dtrace -c ./test -qs /dev/stdin <<EOF
> - test_prov\$target:::
> + # Wait for full startup, passing into main(), to prevent
> + # rtld activity monitoring from triggering repeated
> + # probe scans (checking for problems doing one-off scans of
> + # already-running processes while probing multiple probes).
> + ./test & { WAIT=$!; sleep 1; $dtrace -qs /dev/stdin $WAIT <<EOF; }
> + test_prov\$1:::go
> {
> printf("%s:%s:%s\n", probemod, probefunc, probename);
> }
> + test_prov\$1:::stop
> + {
> + printf("%s:%s:%s\n", probemod, probefunc, probename);
> + }
> + test_prov\$1:::stop
> + / ++i > 1 /
> + {
> + exit(0);
> + }
> EOF
> }
>
> --
> 2.39.1.268.g9de2f9a303
>
>
> _______________________________________________
> 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