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

[Kris Van Hees]

On Thu, May 18, 2023 at 07:32:52PM -0400, Eugene Loh via DTrace-devel wrote:
> 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?

Copy error.  Will fix.

> > + * 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.

All probes will always be in the probe_args list.

And yes, conceptually this is a bit awkward, but on the other hand, this is
an implementation detail that people writing SDT providers do not need to
care about.  It should not matter how we accomplish looping through all the
probes in the provider.

But yes, a bit awkward.  But it makes for easier code.

> > +	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?

Ugh, forgot to change that one.  WIll fix.

> > + */
> > +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?

No, the output is generated based on the ordinal index of each argument.  Not
the order in which they are listed.

> > + *
> > + * A single probe entry that mentions only name of the probe indicates a probe
> > + * that provides no arguments.
> > + */
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel