dtrace sdt probes in modules

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

Currently dtrace sdt probes in loaded modules (as opposed to builtin
modules) can't be used, because dtrace_sdt.kmod itself only processes
the statically linked link sets of providers/probes/argtypes.

For example, none of the zfs dtrace probes are available, and they
would be handy for diagnostics -- especially with recent performance
regressions and pagedaemon or xcall storms.

The attached pair of patches aims to fix this by teaching
dtrace_sdt.kmod to process link sets of loaded modules as they are
loaded, using module_callbacks_register(9).

One of the patches tweaks module_register_callbacks(9): currently it
invokes the load callback for all _previously_ loaded modules in
_reverse_ topological order (dependents before their dependencies),
and invokes the load callback for all _subsequently_ loaded modules in
topological order (dependencies then dependents).

This is a little weird, and not the reverse of the unload callbacks,
and it makes the life of dtrace_sdt harder -- dtrace_sdt wants to
process all providers before any probes that use them.  So I changed
it to always run in topological order (unload still runs in reverse
topological order).  I think the change is low risk because the only
other user is dtrace.kmod, which doesn't seem to care about order from
a cursory glance.

Anyone care to review or test?

From 27dc78b087f76a451b4350a81fc0e7b01d020fb8 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 7 Aug 2022 18:45:29 +0000
Subject: [PATCH 1/2] module(9): Call callbacks in topological order on load.

They are called in reverse topological order on unload.

dtrace_sdt will rely on this soon to ensure provider definitions are
processed before their uses.
---
 sys/kern/kern_module.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sys/kern/kern_module.c b/sys/kern/kern_module.c
index d2457df91969..7ad2b5bffc83 100644
--- a/sys/kern/kern_module.c
+++ b/sys/kern/kern_module.c
@@ -1889,7 +1889,7 @@ module_register_callbacks(void (*load)(struct module *),
 
 	kernconfig_lock();
 	TAILQ_INSERT_TAIL(&modcblist, modcb, modcb_list);
-	TAILQ_FOREACH(mod, &module_list, mod_chain)
+	TAILQ_FOREACH_REVERSE(mod, &module_list, modlist, mod_chain)
 		load(mod);
 	kernconfig_unlock();
 

From 8cecbaf821160dc6ec18ae283a464f079024ac64 Mon Sep 17 00:00:00 2001
From: Taylor R Campbell <riastradh%NetBSD.org@localhost>
Date: Sun, 7 Aug 2022 16:08:49 +0000
Subject: [PATCH 2/2] dtrace_sdt: Register sdt providers and probes in loaded
 modules too.

---
 external/cddl/osnet/dev/sdt/sdt.c | 186 +++++++++++++++++++++++-------
 sys/modules/dtrace/sdt/Makefile   |   1 +
 2 files changed, 148 insertions(+), 39 deletions(-)

diff --git a/external/cddl/osnet/dev/sdt/sdt.c b/external/cddl/osnet/dev/sdt/sdt.c
index b18ba093f3cd..a23f71f3cf1a 100644
--- a/external/cddl/osnet/dev/sdt/sdt.c
+++ b/external/cddl/osnet/dev/sdt/sdt.c
@@ -52,6 +52,9 @@ __KERNEL_RCSID(0, "$NetBSD: sdt.c,v 1.21 2022/03/28 12:33:20 riastradh Exp $");
 #endif
 #include <sys/kernel.h>
 #include <sys/syslimits.h>
+#ifdef __NetBSD__
+#include <sys/kobj.h>
+#endif
 #ifdef __FreeBSD__
 #include <sys/linker.h>
 #include <sys/linker_set.h>
@@ -112,6 +115,10 @@ static dtrace_pops_t sdt_pops = {
 };
 
 #ifdef __NetBSD__
+struct linker_file {
+	struct module	lf_mod;
+};
+
 static int
 sdt_open(dev_t dev, int flags, int mode, struct lwp *l)
 {
@@ -203,14 +210,19 @@ sdt_create_probe(struct sdt_probe *probe)
 
 	SDT_KASSERT(prov != NULL, ("probe defined without a provider"));
 
-#ifdef __FreeBSD__
 	/* If no module name was specified, use the module filename. */
 	if (*probe->mod == 0) {
+#ifdef __NetBSD__
+		const char *modname = (probe->sdtp_lf == NULL ? "netbsd" :
+		    module_name(&probe->sdtp_lf->lf_mod));
+		strlcpy(mod, modname, sizeof(mod));
+#endif
+#ifdef __FreeBSD__
 		len = strlcpy(mod, probe->sdtp_lf->filename, sizeof(mod));
 		if (len > 3 && strcmp(mod + len - 3, ".ko") == 0)
 			mod[len - 3] = '\0';
-	} else
 #endif
+	} else
 		strlcpy(mod, probe->mod, sizeof(mod));
 
 	/*
@@ -257,6 +269,9 @@ sdt_enable(void *arg __unused, dtrace_id_t id, void *parg)
 	struct sdt_probe *probe = parg;
 
 	probe->id = id;
+#ifdef __NetBSD__
+	module_hold(&probe->sdtp_lf->lf_mod);
+#endif
 #ifdef __FreeBSD__
 	probe->sdtp_lf->nenabled++;
 	if (strcmp(probe->prov->name, "lockstat") == 0)
@@ -270,6 +285,9 @@ sdt_disable(void *arg __unused, dtrace_id_t id, void *parg)
 {
 	struct sdt_probe *probe = parg;
 
+#ifdef __NetBSD__
+	module_rele(&probe->sdtp_lf->lf_mod);
+#endif
 #ifdef __FreeBSD__
 	SDT_KASSERT(probe->sdtp_lf->nenabled > 0, ("no probes enabled"));
 	if (strcmp(probe->prov->name, "lockstat") == 0)
@@ -410,63 +428,154 @@ __link_set_decl(sdt_providers_set, struct sdt_provider);
 __link_set_decl(sdt_probes_set, struct sdt_probe);
 __link_set_decl(sdt_argtypes_set, struct sdt_argtype);
 
-/*
- * Unfortunately we don't have linker set functions and event handlers
- * to support loading and unloading probes in modules... Currently if
- * modules have probes, if the modules are loaded when sdt is loaded
- * they will work, but they will crash unloading.
- */
+struct module_callbacks *sdt_link_set_callbacks;
+
 static void
-sdt_link_set_load(void)
+sdt_link_set_load_provider(struct sdt_provider *const *provider)
+{
+
+	sdt_create_provider(*provider);
+}
+
+static void
+sdt_link_set_unload_provider(struct sdt_provider *const *curr)
+{
+	struct sdt_provider *prov, *tmp;
+
+	/*
+	 * Go through all the providers declared in this linker file and
+	 * unregister any that aren't declared in another loaded file.
+	 */
+	TAILQ_FOREACH_SAFE(prov, &sdt_prov_list, prov_entry, tmp) {
+		if (strcmp(prov->name, (*curr)->name) != 0)
+			continue;
+
+		if (prov->sdt_refs == 1) {
+			if (dtrace_unregister(prov->id) != 0) {
+				return;
+			}
+			TAILQ_REMOVE(&sdt_prov_list, prov, prov_entry);
+			free(__UNCONST(prov->name), M_SDT);
+			free(prov, M_SDT);
+		} else
+			prov->sdt_refs--;
+		break;
+	}
+}
+
+static void
+sdt_link_set_load_probe(struct sdt_probe *const *probe, struct module *mod)
+{
+	struct linker_file *lf = mod == NULL ? NULL :
+	    container_of(mod, struct linker_file, lf_mod);
+
+	(*probe)->sdtp_lf = lf;
+	sdt_create_probe(*probe);
+	TAILQ_INIT(&(*probe)->argtype_list);
+}
+
+static void
+sdt_link_set_load_argtype(struct sdt_argtype *const *argtype)
+{
+
+	(*argtype)->probe->n_args++;
+	TAILQ_INSERT_TAIL(&(*argtype)->probe->argtype_list,
+	    *argtype, argtype_entry);
+}
+
+static void
+sdt_link_set_load_module(struct module *mod)
 {
 	struct sdt_provider * const *provider;
 	struct sdt_probe * const *probe;
 	struct sdt_argtype * const *argtype;
+	void *p;
+	size_t size, n;
 
-	__link_set_foreach(provider, sdt_providers_set) {
-		sdt_create_provider(*provider);
+	/*
+	 * Skip builtin modules -- they are handled separately with
+	 * __link_set_foreach.
+	 */
+	if (module_source(mod) == MODULE_SOURCE_KERNEL)
+		return;
+
+	if (kobj_find_section(mod->mod_kobj, "link_set_sdt_providers_set",
+		&p, &size) == 0) {
+		n = size/sizeof(*provider);
+		for (provider = p; n --> 0; provider++)
+			sdt_link_set_load_provider(provider);
 	}
 
-	__link_set_foreach(probe, sdt_probes_set) {
-		(*probe)->sdtp_lf = NULL;	// XXX: we don't support it
-		sdt_create_probe(*probe);
-		TAILQ_INIT(&(*probe)->argtype_list);
+	if (kobj_find_section(mod->mod_kobj, "link_set_sdt_probes_set",
+		&p, &size) == 0) {
+		n = size/sizeof(*probe);
+		for (probe = p; n --> 0; probe++)
+			sdt_link_set_load_probe(probe, mod);
 	}
 
-	__link_set_foreach(argtype, sdt_argtypes_set) {
-		(*argtype)->probe->n_args++;
-		TAILQ_INSERT_TAIL(&(*argtype)->probe->argtype_list,
-		    *argtype, argtype_entry);
+	if (kobj_find_section(mod->mod_kobj, "link_set_sdt_argtypes_set",
+		&p, &size) == 0) {
+		n = size/sizeof(*argtype);
+		for (argtype = p; n --> 0; argtype++)
+			sdt_link_set_load_argtype(argtype);
 	}
 }
 
 static void
-sdt_link_set_unload(void)
+sdt_link_set_unload_module(struct module *mod)
 {
-	struct sdt_provider * const *curr, *prov, *tmp;
+	struct sdt_provider *const *curr;
+	void *p;
+	size_t size, n;
 
 	/*
-	 * Go through all the providers declared in this linker file and
-	 * unregister any that aren't declared in another loaded file.
+	 * Skip builtin modules -- they are handled separately with
+	 * __link_set_foreach.
 	 */
-	__link_set_foreach(curr, sdt_providers_set) {
-		TAILQ_FOREACH_SAFE(prov, &sdt_prov_list, prov_entry, tmp) {
-			if (strcmp(prov->name, (*curr)->name) != 0)
-				continue;
+	if (module_source(mod) == MODULE_SOURCE_KERNEL)
+		return;
 
-			if (prov->sdt_refs == 1) {
-				if (dtrace_unregister(prov->id) != 0) {
-					return;
-				}
-				TAILQ_REMOVE(&sdt_prov_list, prov, prov_entry);
-				free(__UNCONST(prov->name), M_SDT);
-				free(prov, M_SDT);
-			} else
-				prov->sdt_refs--;
-			break;
-		}
+	if (kobj_find_section(mod->mod_kobj, "link_set_sdt_providers_set",
+		&p, &size) == 0) {
+		n = size/sizeof(*curr);
+		for (curr = p; n --> 0; curr++)
+			sdt_link_set_unload_provider(curr);
 	}
 }
+
+static void
+sdt_link_set_load(void)
+{
+	struct sdt_provider * const *provider;
+	struct sdt_probe * const *probe;
+	struct sdt_argtype * const *argtype;
+
+	__link_set_foreach(provider, sdt_providers_set) {
+		sdt_link_set_load_provider(provider);
+	}
+	__link_set_foreach(probe, sdt_probes_set) {
+		sdt_link_set_load_probe(probe, NULL);
+	}
+	__link_set_foreach(argtype, sdt_argtypes_set) {
+		sdt_link_set_load_argtype(argtype);
+	}
+
+	sdt_link_set_callbacks = module_register_callbacks(
+	    sdt_link_set_load_module, sdt_link_set_unload_module);
+}
+
+static void
+sdt_link_set_unload(void)
+{
+	struct sdt_provider *const *curr;
+
+	module_unregister_callbacks(sdt_link_set_callbacks);
+	sdt_link_set_callbacks = NULL;
+
+       __link_set_foreach(curr, sdt_providers_set) {
+	       sdt_link_set_unload_provider(curr);
+       }
+}
 #endif
 
 
@@ -507,7 +616,6 @@ sdt_unload(void)
 
 #ifdef __NetBSD__
 	sdt_exit();
-
 	sdt_link_set_unload();
 #endif
 	TAILQ_FOREACH_SAFE(prov, &sdt_prov_list, prov_entry, tmp) {
diff --git a/sys/modules/dtrace/sdt/Makefile b/sys/modules/dtrace/sdt/Makefile
index 9f96f71a9064..5129401efa69 100644
--- a/sys/modules/dtrace/sdt/Makefile
+++ b/sys/modules/dtrace/sdt/Makefile
@@ -11,5 +11,6 @@ CPPFLAGS+=	-I${NETBSDSRCDIR}/external/cddl/osnet/sys \
 		-I${NETBSDSRCDIR}/external/cddl/osnet/dist/uts/common
 
 CPPFLAGS+=	-Wno-unknown-pragmas
+CPPFLAGS+=	-Wno-sign-compare
 
 .include <bsd.kmodule.mk>