[DTrace-devel] [PATCH v2] sdt: consolidate the SDT infrastructure in its own provider framework

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with some notes...

On 5/18/23 15:30, Kris Van Hees via DTrace-devel wrote:
> diff --git a/libdtrace/dt_provider_sdt.c b/libdtrace/dt_provider_sdt.c
> new file mode 100644
> @@ -0,0 +1,165 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2021, 2023, Oracle and/or its affiliates. All rights reserved.

2021?

> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + *
> + * Provider support code for tracepoint-based probes.
> + */
> +int
> +dt_sdt_populate(dtrace_hdl_t *dtp, const char *prvname, const char *modname,
> +		const dt_provimpl_t *ops, const dtrace_pattr_t *pattr,
> +		const probe_arg_t *probe_args, const probe_dep_t *probes)
> +{
> +	dt_provider_t		*prv;
> +	const probe_arg_t	*arg;
> +	sdt_data_t		*sdp;
> +	int			n = 0;
> +
> +	sdp = dt_alloc(dtp, sizeof(sdt_data_t));
> +	if (sdp == NULL)
> +		return 0;
> +
> +	sdp->probes = probes;
> +	sdp->probe_args = probe_args;
> +	prv = dt_provider_create(dtp, prvname, ops, pattr, sdp);
> +	if (prv == NULL)
> +		return 0;
> +
> +	/*
> +	 * Create SDT probes based on the probe_args list.  Since each probe
> +	 * will have at least one entry (with argno == 0), we can use those
> +	 * entries to identify the probe names.
> +	 */

This seems a little weird to me.  Why are we using the probe_args list 
rather than the probes list?  If we want to loop over probes, we should 
loop over the probes list not the probe args list.  To be fair, the 
"probes" list is a little misnamed in that each probe is not listed just 
once... it's more of a "probes dependencies" list. But at least we know 
each probe will appear in the list.  Meanwhile, some probes will not 
have arguments, so we might expect such a probe not to appear in the 
probe args list.  Well, that objection is fixed because we require each 
probe to show up in the args list even if it doesn't have any args.  
And, magically, the probe arg "that doesn't exist" has argno==0, as the 
comment block advertises.  So my objection is not that this doesn't 
work, but only that it's conceptually awkward.

> +	for (arg = &probe_args[0]; arg->name != NULL; arg++) {
> +		if (arg->argno == 0 &&
> +		    dt_probe_insert(dtp, prv, prvname, modname, "", arg->name,
> +				    NULL))
> +			n++;
> +	}
> +
> +	return n;
> +}
> diff --git a/libdtrace/dt_provider_sdt.h b/libdtrace/dt_provider_sdt.h
> +/*
> + * Probe dependencies
> + *
> + * SDT probes are implemented using probes made available by other providers.
> + * THe probe dependency table associates each SDT probe with one or more probe

THe

> + * specifications (possibly containing wildcards).  Each matching probe will
> + * have SDT lockstat probe added as a dependent probe.

lockstat?

> + */
> +typedef struct probe_dep {
> +	const char		*name;			/* probe name */
> +	dtrace_probespec_t	spec;			/* spec type */
> +	const char		*str;			/* spec string */
> +} probe_dep_t;
> +
> +/*
> + * Probe signature specifications
> + *
> + * This table *must* group the arguments of probes.  I.e. the arguments of a
> + * given probe must be listed in consecutive records.

Should args also be in order?  E.g., does that impact user-visible 
"dtrace -lv" output, where a user would expect to see args in order?

> + *
> + * A single probe entry that mentions only name of the probe indicates a probe
> + * that provides no arguments.
> + */