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 <riastr...@netbsd.org>
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 <riastr...@netbsd.org>
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>
