[DTrace-devel] [PATCH v2 1/4] dtprobed: handle multiple providers in a single piece of DOF
Kris Van Hees
kris.van.hees at oracle.com
Fri Nov 15 04:50:42 UTC 2024
On Thu, Nov 14, 2024 at 10:01:05PM +0000, Nick Alcock wrote:
> A given ELF object can of course contain as many providers as you like. The
> DOF stash format, DOF stash management code and DTrace itself are all set up
> fine to handle DOF with multiple providers in: but, alas, the code that
> reads the DOF from the parser child in response to an ioctl() assumes that
> one provider is all it will ever see.
>
> This is not ideal, but rejigging it introduces an interesting problem: when
> do you *stop* reading providers from the parser stream? Right now,
> dof_parser.c loops over all of its providers and emits them until all are
> done, then just stops: there is no indication of how many providers there
> are or that this is the last provider or anything (though if there are none,
> it does try to send an empty provider to avoid the caller blocking waiting
> for a reply that will never come).
>
> It's a bit annoying to try to figure out how many providers there are in
> advance, so instead of that, take a route that's easier both to emit and
> parse, drop the empty provider stuff, and simply add a DIT_EOF record which
> indicates "no more providers expected", which is always sent last. (We
> stuff this in before DIT_ARGS_* in the enum because it makes more conceptual
> sense right after DIT_ERR, and DIT_ARGS_* hasn't released anywhere yet.)
>
> With that in place it's a simple matter to loop adding parsed DOF to the
> stash's accumulator until we hit an EOF record. Memory management is
> complicated a little, because a single parsed buffer (reply from dof_parser)
> is now owned by multiple accumulator records in the stash: but every one of
> those is terminated by a DIT_EOF, which appears nowhere else, so we can just
> not free a parsed buffer unless it's of type DIT_EOF.
>
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
Reviewed-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> dtprobed/dof_stash.c | 12 +++-
> dtprobed/dtprobed.c | 63 ++++++++++++++-------
> libcommon/dof_parser.c | 23 +++-----
> libcommon/dof_parser.h | 8 ++-
> test/triggers/Build | 6 +-
> test/triggers/usdt-tst-multiprovider-prov.d | 12 ++++
> test/triggers/usdt-tst-multiprovider.c | 23 ++++++++
> test/unittest/usdt/tst.multiprovider.r | 4 ++
> test/unittest/usdt/tst.multiprovider.r.p | 2 +
> test/unittest/usdt/tst.multiprovider.sh | 15 +++++
> 10 files changed, 126 insertions(+), 42 deletions(-)
> create mode 100644 test/triggers/usdt-tst-multiprovider-prov.d
> create mode 100644 test/triggers/usdt-tst-multiprovider.c
> create mode 100644 test/unittest/usdt/tst.multiprovider.r
> create mode 100755 test/unittest/usdt/tst.multiprovider.r.p
> create mode 100755 test/unittest/usdt/tst.multiprovider.sh
>
> Changes since v1: test multiple probes.
>
> This had fallout -- see later commits in this series!
>
> diff --git a/dtprobed/dof_stash.c b/dtprobed/dof_stash.c
> index 82fdd3174759d..296987ad41e93 100644
> --- a/dtprobed/dof_stash.c
> +++ b/dtprobed/dof_stash.c
> @@ -360,7 +360,13 @@ dof_stash_free(dt_list_t *accum)
> for (accump = dt_list_next(accum); accump != NULL;
> accump = dt_list_next(accump)) {
> dt_list_delete(accum, accump);
> - free(accump->parsed);
> +
> + /*
> + * All parsed memory regions are terminated by an EOF, so once
> + * we encounter the EOF, this region can safely be freed.
> + */
> + if (accump->parsed->type == DIT_EOF)
> + free(accump->parsed);
> free(last_accump);
> last_accump = accump;
> }
> @@ -656,6 +662,10 @@ dof_stash_write_parsed(pid_t pid, dev_t dev, ino_t ino, dt_list_t *accum)
> fuse_log(FUSE_LOG_ERR, "dtprobed: parser error: %s\n",
> accump->parsed->err.err);
> goto err_provider_close;
> + /* EOF: nothing to do. No need to record this parser-stream
> + * implementation detail into the parsed representation. */
> + case DIT_EOF:
> + break;
>
> default:
> /*
> diff --git a/dtprobed/dtprobed.c b/dtprobed/dtprobed.c
> index 2ca39b26f88a9..86865eb467b67 100644
> --- a/dtprobed/dtprobed.c
> +++ b/dtprobed/dtprobed.c
> @@ -772,41 +772,60 @@ process_dof(pid_t pid, int out, int in, dev_t dev, ino_t inum, dev_t exec_dev,
> dof_parser_tidy(1);
> continue;
> }
> - if (provider->type != DIT_PROVIDER)
> + if (provider->type != DIT_PROVIDER && provider->type != DIT_EOF)
> goto err;
> break;
> } while (!provider);
>
> - if (dof_stash_push_parsed(&accum, provider) < 0)
> - goto oom;
> -
> - for (i = 0; i < provider->provider.nprobes; i++) {
> - dof_parsed_t *probe = dof_read(pid, in);
> - size_t j;
> -
> - errmsg = "no probes in this provider, or parse state corrupt";
> - if (!probe || probe->type != DIT_PROBE)
> - goto err;
> -
> - if (dof_stash_push_parsed(&accum, probe) < 0)
> + while (provider->type != DIT_EOF) {
> + if (dof_stash_push_parsed(&accum, provider) < 0)
> goto oom;
>
> - j = 0;
> - do {
> - dof_parsed_t *tp = dof_read(pid, in);
> + fuse_log(FUSE_LOG_DEBUG, "Parser read: provider %s, %i probes\n",
> + provider->provider.name, provider->provider.nprobes);
>
> - errmsg = "no tracepoints in a probe, or parse state corrupt";
> - if (!tp || tp->type == DIT_PROVIDER || tp->type == DIT_PROBE)
> + for (i = 0; i < provider->provider.nprobes; i++) {
> + dof_parsed_t *probe = dof_read(pid, in);
> + size_t j;
> +
> + errmsg = "no probes in this provider, or parse state corrupt";
> + if (!probe || probe->type != DIT_PROBE)
> goto err;
>
> - if (dof_stash_push_parsed(&accum, tp) < 0)
> + if (dof_stash_push_parsed(&accum, probe) < 0)
> goto oom;
>
> - if (tp->type == DIT_TRACEPOINT)
> - j++;
> - } while (j < probe->probe.ntp);
> + j = 0;
> + do {
> + dof_parsed_t *tp = dof_read(pid, in);
> +
> + errmsg = "no tracepoints in a probe, or parse state corrupt";
> + if (!tp || tp->type == DIT_PROVIDER ||
> + tp->type == DIT_PROBE || tp->type == DIT_EOF)
> + goto err;
> +
> + fuse_log(FUSE_LOG_DEBUG, "Parser read: adding %s:%s to stash\n",
> + provider->provider.name, probe->probe.name);
> +
> + if (dof_stash_push_parsed(&accum, tp) < 0)
> + goto oom;
> +
> + if (tp->type == DIT_TRACEPOINT)
> + j++;
> + } while (j < probe->probe.ntp);
> + }
> +
> + errmsg = "subsequent provider read failed, or stream not properly terminated";
> + provider = dof_read(pid, in);
> + if (!provider)
> + goto err;
> }
>
> + /* Push the EOF on. */
> +
> + if (dof_stash_push_parsed(&accum, provider) < 0)
> + goto oom;
> +
> if (!reparsing)
> if ((gen = dof_stash_add(pid, dev, inum, exec_dev, exec_inum, dh,
> in_buf, in_bufsz)) < 0)
> diff --git a/libcommon/dof_parser.c b/libcommon/dof_parser.c
> index 1792a8bfcc849..ae5abe5220929 100644
> --- a/libcommon/dof_parser.c
> +++ b/libcommon/dof_parser.c
> @@ -1145,7 +1145,7 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
> {
> int i, rv;
> uintptr_t daddr = (uintptr_t)dof;
> - int count = 0;
> + dof_parsed_t eof;
>
> dt_dbg_dof("DOF 0x%p from helper {'%s', %p, %p}...\n",
> dof, dhp ? dhp->dofhp_mod : "<none>", dhp, dof);
> @@ -1157,8 +1157,7 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
> }
>
> /*
> - * Look for helper providers, validate their descriptions, and
> - * parse them.
> + * Look for providers, validate their descriptions, and parse them.
> */
> if (dhp != NULL) {
> dt_dbg_dof(" DOF 0x%p Validating and parsing providers...\n", dof);
> @@ -1177,25 +1176,19 @@ dof_parse(int out, dof_helper_t *dhp, dof_hdr_t *dof)
> dof_destroy(dhp, dof);
> return;
> }
> - count++;
> emit_provider(out, dhp, dof, sec);
> }
> }
>
> /*
> - * If nothing was written, emit an empty result to wake up
> - * the caller.
> + * Always emit an EOF, to wake up the caller if nothing else, but also
> + * to notify the caller that there are no more providers to read.
> */
> - if (count == 0) {
> - dof_parsed_t empty;
> + memset(&eof, 0, sizeof(dof_parsed_t));
>
> - memset(&empty, 0, sizeof(dof_parsed_t));
> -
> - empty.size = offsetof(dof_parsed_t, provider.name);
> - empty.type = DIT_PROVIDER;
> - empty.provider.nprobes = 0;
> - dof_parser_write_one(out, &empty, empty.size);
> - }
> + eof.size = offsetof(dof_parsed_t, provider.nprobes);
> + eof.type = DIT_EOF;
> + dof_parser_write_one(out, &eof, eof.size);
>
> dof_destroy(dhp, dof);
> }
> diff --git a/libcommon/dof_parser.h b/libcommon/dof_parser.h
> index 8f42d00551e3f..75aa3082161b7 100644
> --- a/libcommon/dof_parser.h
> +++ b/libcommon/dof_parser.h
> @@ -24,6 +24,7 @@
> * DIT_ARGS_XLAT (1, optional)
> * DIT_ARGS_MAP (1, optional)
> * DIT_TRACEPOINT (any number >= 1)
> + * DIT_EOF (no more providers, last record)
> *
> * The dof_parsed.provider.flags word indicates the presence of the
> * various optional args records in the following stream (you can rely on
> @@ -37,9 +38,10 @@ typedef enum dof_parsed_info {
> DIT_PROBE = 1,
> DIT_TRACEPOINT = 2,
> DIT_ERR = 3,
> - DIT_ARGS_NATIVE = 4,
> - DIT_ARGS_XLAT = 5,
> - DIT_ARGS_MAP = 6,
> + DIT_EOF = 4,
> + DIT_ARGS_NATIVE = 5,
> + DIT_ARGS_XLAT = 6,
> + DIT_ARGS_MAP = 7,
> } dof_parsed_info_t;
>
> /*
> diff --git a/test/triggers/Build b/test/triggers/Build
> index 107b3b4d1f2e8..c50c265ccec7d 100644
> --- a/test/triggers/Build
> +++ b/test/triggers/Build
> @@ -13,7 +13,8 @@ EXTERNAL_64BIT_TRIGGERS = testprobe readwholedir mmap bogus-ioctl open delaydie
> ustack-tst-spin ustack-tst-mtspin \
> visible-constructor visible-constructor-static visible-constructor-static-unstripped
>
> -EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer usdt-tst-special
> +EXTERNAL_64BIT_SDT_TRIGGERS = usdt-tst-argmap usdt-tst-args usdt-tst-forker usdt-tst-defer \
> + usdt-tst-multiprovider usdt-tst-special
> EXTERNAL_64BIT_TRIGGERS += $(EXTERNAL_64BIT_SDT_TRIGGERS)
>
> EXTERNAL_32BIT_TRIGGERS := visible-constructor-32
> @@ -206,6 +207,9 @@ usdt-tst-forker_PROV := usdt-tst-forker-prov.d
> # usdt-tst-defer calls USDT probes based on dtrace -h
> usdt-tst-defer_PROV := usdt-tst-defer-prov.d
>
> +# usdt-tst-multiprovider calls USDT probes based on dtrace -h
> +usdt-tst-multiprovider_PROV := usdt-tst-multiprovider-prov.d
> +
> # usdt-tst-special calls USDT probes based on dtrace -h
> usdt-tst-special_CFLAGS := -fno-inline -O2
> usdt-tst-special_PROV := usdt-tst-special-prov.d
> diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
> new file mode 100644
> index 0000000000000..530693ea71fe7
> --- /dev/null
> +++ b/test/triggers/usdt-tst-multiprovider-prov.d
> @@ -0,0 +1,12 @@
> +/*
> + * 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.
> + */
> +
> +/* @@skip: provider declaration - not a test */
> +
> +provider prova { probe entrya(); };
> +provider provb { probe entryb(); probe entryc(int a, char *b) : (char * b, int a); };
> +provider provc { probe entryd(); };
> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
> new file mode 100644
> index 0000000000000..a954111379d9f
> --- /dev/null
> +++ b/test/triggers/usdt-tst-multiprovider.c
> @@ -0,0 +1,23 @@
> +/*
> + * 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.
> + */
> +
> +/*
> + * A trigger with multiple providers in it.
> + */
> +#include <stdio.h>
> +#include <unistd.h>
> +#include "usdt-tst-multiprovider-prov.h"
> +
> +int main(int argc, char **argv)
> +{
> + PROVA_ENTRYA();
> + PROVB_ENTRYB();
> + PROVB_ENTRYC(666, "foo");
> + PROVC_ENTRYD();
> + usleep(5 * 1000 * 1000);
> + return 0;
> +}
> diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
> new file mode 100644
> index 0000000000000..60a15c01eeee3
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r
> @@ -0,0 +1,4 @@
> +ID provaPID usdt-tst-multiprovider main entrya
> +ID provbPID usdt-tst-multiprovider main entryb
> +ID provbPID usdt-tst-multiprovider main entryc
> +ID provcPID usdt-tst-multiprovider main entryd
> diff --git a/test/unittest/usdt/tst.multiprovider.r.p b/test/unittest/usdt/tst.multiprovider.r.p
> new file mode 100755
> index 0000000000000..ae8493e3d5be4
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r.p
> @@ -0,0 +1,2 @@
> +#!/bin/sh
> +grep -v '^ *ID' | sed 's,^[0-9]*,ID,; s,prov\(.\)[0-9]*,prov\1PID,; s, *, ,g'
> diff --git a/test/unittest/usdt/tst.multiprovider.sh b/test/unittest/usdt/tst.multiprovider.sh
> new file mode 100755
> index 0000000000000..d5b72c2be77ec
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.sh
> @@ -0,0 +1,15 @@
> +#!/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.
> +#
> +if [ $# != 1 ]; then
> + echo expected one argument: '<'dtrace-path'>'
> + exit 2
> +fi
> +
> +dtrace=$1
> +
> +exec $dtrace $dt_flags -l -P 'prov*' -c `pwd`/test/triggers/usdt-tst-multiprovider
>
> base-commit: 68d14f30b5ffaaea405354a2edf44279127d209a
> --
> 2.46.0.278.g36e3a12567
>
More information about the DTrace-devel
mailing list