[DTrace-devel] [PATCH v3 dtrace 3/4] fbt: avoid mix of kprobe, fprobe implementations for used probes

[Alan Maguire]

If we have a mix of kprobe and fprobe implementations, it causes problems
for DTrace since we cannot guarantee execution in program order.

When fprobes are active, the only place kprobes are used as fbt
implementation is for "."-suffixed functions that cannot be supported
by fprobe.  However, by using kprobes for this case, we run the risk of
mixing fprobe and kprobe.  So use probe_info callbacks (invoked when
setting context during compilation) to count instances of fbt probe use
for each provider/implementation.  If we have a mix of fprobes and kprobes,
revert to using the kprobe implementation for the existing fprobes.
This involves setting provider impl to kprobes, and resetting event
id for any existing fprobes (since it will be set to the BTF id of the
function).

There is no risk in having multiple probes for the same function since
a kprobe for a "."-suffixed function will not be added if the
unsuffixed probe already exists (in practice, such a case is not seen
but is not impossible).

Signed-off-by: Alan Maguire <alan.maguire at oracle.com>
---
 libdtrace/dt_prov_fbt.c | 66 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/libdtrace/dt_prov_fbt.c b/libdtrace/dt_prov_fbt.c
index 06ea78ca..259cf674 100644
--- a/libdtrace/dt_prov_fbt.c
+++ b/libdtrace/dt_prov_fbt.c
@@ -76,7 +76,7 @@ static int populate(dtrace_hdl_t *dtp)
 
 	default_impl = BPF_HAS(dtp, BPF_FEAT_FENTRY) ? &dt_fbt_fprobe : &dt_fbt_kprobe;
 
-	default_prv = dt_provider_create(dtp, prvname, default_impl, &pattr, NULL);
+	default_prv = dt_provider_create(dtp, prvname, default_impl, &pattr, 0);
 	if (default_prv == NULL)
 		return -1;			/* errno already set */
 	if (default_impl == &dt_fbt_kprobe)
@@ -148,7 +148,7 @@ static int populate(dtrace_hdl_t *dtp)
 			if (!kprobe_prv) {
 				kprobe_prv = dt_provider_create(dtp, prvname,
 								impl, &pattr,
-								NULL);
+								0);
 				if (kprobe_prv == NULL)
 					return -1;
 			}
@@ -187,6 +187,56 @@ static int populate(dtrace_hdl_t *dtp)
 	return n;
 }
 
+static int
+fbt_unset_event_id(dtrace_hdl_t *dtp, dt_probe_t *prp, dt_provider_t *prv)
+{
+	if (prp->prov == prv)
+		dt_tp_set_event_id(prp, 0);
+	return 0;
+}
+
+/* Ensure we do not have a mix of kprobe and fprobe implementations for
+ * _actually used_ probes; fall back to using kprobe implementation for
+ * fprobe provider if so.
+ */
+void fbt_check_impl(struct dtrace_hdl *dtp, dt_provider_t *fbt_prv)
+{
+	dt_htab_next_t          *it = NULL;
+	dt_provider_t		*pvp;
+	dtrace_probedesc_t	fbt_desc = {	.prv = "fbt",
+						.mod = "",
+						.fun = "",
+						.prb = ""
+					   };
+
+
+	/* count number of used probes for provider + impl */
+	fbt_prv->prv_data++;
+
+	/* check other provider + impl (if any) */
+	while ((pvp = dt_htab_next(dtp->dt_provs, &it)) != NULL) {
+		if (pvp == fbt_prv || strcmp(pvp->desc.dtvd_name, "fbt") != 0)
+			continue;
+		if (pvp->prv_data == 0)
+			return;
+		/* already done? */
+		if (pvp->impl == fbt_prv->impl)
+			return;
+		/* multiple fprobe, kprobe probes; revert to kprobes */
+		if (fbt_prv->impl != &dt_fbt_kprobe)
+			pvp = fbt_prv;
+		break;
+	}
+	if (!pvp)
+		return;
+	dt_dprintf("reverting to kprobe impl for %s due to fprobe/kprobe mix\n",
+		   pvp->desc.dtvd_name);
+	pvp->impl = &dt_fbt_kprobe;
+	dt_probe_iter(dtp, &fbt_desc, (dt_probe_f *)fbt_unset_event_id, NULL,
+		      pvp);
+
+}
+
 /*******************************\
  * FPROBE-based implementation *
 \*******************************/
@@ -296,6 +346,8 @@ static int fprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
 		goto done;
 	}
 
+	fbt_check_impl(dtp, prp->prov);
+
 	argc = dt_btf_func_argc(dtp, dmp->dm_btf, btf_id);
 	if (argc == 0)
 		goto done;
@@ -453,6 +505,15 @@ static int kprobe_attach(dtrace_hdl_t *dtp, const dt_probe_t *prp, int bpf_fd)
 	return dt_tp_probe_attach(dtp, prp, bpf_fd);
 }
 
+static int kprobe_probe_info(dtrace_hdl_t *dtp, const dt_probe_t *prp,
+			     int *argcp, dt_argdesc_t **argvp)
+{
+	fbt_check_impl(dtp, prp->prov);
+	*argcp = 0;
+	*argvp = NULL;
+	return 0;
+}
+
 /*
  * Try to clean up system resources that may have been allocated for this
  * probe.
@@ -502,6 +563,7 @@ dt_provimpl_t	dt_fbt_kprobe = {
 	.load_prog	= &dt_bpf_prog_load,
 	.trampoline	= &kprobe_trampoline,
 	.attach		= &kprobe_attach,
+	.probe_info	= &kprobe_probe_info,
 	.detach		= &kprobe_detach,
 	.probe_destroy	= &dt_tp_probe_destroy,
 };
-- 
2.43.5