Re: [DTrace-devel] [PATCH v2 10/19] Remove the is-enabled provider

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Oct 25, 2024 at 09:16:19PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh@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@oracle.com>

Reviewed-by: Kris Van Hees <kris.van.hees@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@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel