[DTrace-devel] [PATCH] dtprobed: handle multiple providers in a single piece of DOF

[Eugene Loh]

I'm not familiar with this code, but here is some amateur feedback...

On 11/14/24 11:03, 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

I'm not sure what this is saying, but how about adding a comma right 
after "loop"?

> 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>
>
> diff --git 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

The new line has indentation inconsistent with the pre-existing lines.

> diff --git 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

Okay.  Introducing a new test/triggers that is used by only one test 
is... well, I guess it's a judgment call.

> diff --git a/test/triggers/usdt-tst-multiprovider-prov.d b/test/triggers/usdt-tst-multiprovider-prov.d
> new file mode 100644
> @@ -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 */

I don't think the @@skip is needed for something inside test/triggers.

> +
> +provider prova { probe entrya(); };
> +provider provb { probe entryb(); };
> +provider provc { probe entryc(); };

We should stress test more:
*)   multiple probes per provider
*)   some probes with same and different names as in other providers
*)   probe args (including probes in different providers handling args 
differently)


> diff --git a/test/triggers/usdt-tst-multiprovider.c b/test/triggers/usdt-tst-multiprovider.c
> new file mode 100644
> index 0000000000000..430c0cdd2cf4d
> --- /dev/null
> +++ b/test/triggers/usdt-tst-multiprovider.c
> @@ -0,0 +1,22 @@
> +/*
> + * 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>

stdio.h not needed

> +#include <unistd.h>

only needed for usleep()

> +#include "usdt-tst-multiprovider-prov.h"
> +
> +int main(int argc, char **argv)
> +{
> +	PROVA_ENTRYA();
> +	PROVB_ENTRYB();
> +	PROVC_ENTRYC();
> +	usleep(5 * 1000 * 1000);

usleep not needed and just taking up test time.  It was originally in 
some tests that were checking /run/dtrace "manually".

> +	return 0;
> +}
> diff --git a/test/unittest/usdt/tst.multiprovider.r b/test/unittest/usdt/tst.multiprovider.r
> new file mode 100644
> index 0000000000000..b83f559a7e7ff
> --- /dev/null
> +++ b/test/unittest/usdt/tst.multiprovider.r
> @@ -0,0 +1,3 @@
> +ID provaPID usdt-tst-multiprovider main entrya
> +ID provbPID usdt-tst-multiprovider main entryb
> +ID provcPID usdt-tst-multiprovider main entryc
> 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'

Okay, though the "grep -v" would be clearer to me if it checked  '^ *ID 
*PROVIDER *MODULE *FUNCTION *NAME$'.

This whole thing would be clearer to me as a single awk, but maybe 
that's just me.

Comments might be nice.  I'm not sure how comfortably people read 
regex...  I need to concentrate hard to figure it out.

> 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

Okay, but there should also be a test that actually fires probes.