From da90f3be7e93a841b474ee55fbd7ef96515fdf2f Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Mon, 29 Apr 2024 15:26:56 +1000 Subject: [PATCH 1/2] linux/kstat: allow multi-level module names Module names are mapped directly to directory names in procfs, but nothing is done to create the intermediate directories, or remove them. This makes it impossible to sensibly present kstats about sub-objects. This commit loops through '/'-separated names in the full module name, creates a separate module for each, and hooks them up with a parent pointer and child counter, and then unrolls this on the other side when deleting a module. Sponsored-by: Klara, Inc. Sponsored-by: Syneto Signed-off-by: Rob Norris --- include/os/linux/spl/sys/kstat.h | 6 ++ module/os/linux/spl/spl-kstat.c | 102 ++++++++++++++++++++++++------- 2 files changed, 85 insertions(+), 23 deletions(-) diff --git a/include/os/linux/spl/sys/kstat.h b/include/os/linux/spl/sys/kstat.h index a9e9f1880649..be58b455f9b5 100644 --- a/include/os/linux/spl/sys/kstat.h +++ b/include/os/linux/spl/sys/kstat.h @@ -21,6 +21,10 @@ * You should have received a copy of the GNU General Public License along * with the SPL. If not, see . */ +/* + * Copyright (c) 2024-2025, Klara, Inc. + * Copyright (c) 2024-2025, Syneto + */ #ifndef _SPL_KSTAT_H #define _SPL_KSTAT_H @@ -90,6 +94,8 @@ typedef struct kstat_module { struct list_head ksm_module_list; /* module linkage */ struct list_head ksm_kstat_list; /* list of kstat entries */ struct proc_dir_entry *ksm_proc; /* proc entry */ + struct kstat_module *ksm_parent; /* parent module in hierarchy */ + uint_t ksm_nchildren; /* number of child modules */ } kstat_module_t; typedef struct kstat_raw_ops { diff --git a/module/os/linux/spl/spl-kstat.c b/module/os/linux/spl/spl-kstat.c index 04578204a318..0a6125755118 100644 --- a/module/os/linux/spl/spl-kstat.c +++ b/module/os/linux/spl/spl-kstat.c @@ -27,6 +27,10 @@ * [1] https://illumos.org/man/1M/kstat * [2] https://illumos.org/man/9f/kstat_create */ +/* + * Copyright (c) 2024-2025, Klara, Inc. + * Copyright (c) 2024-2025, Syneto + */ #include #include @@ -370,6 +374,8 @@ static const struct seq_operations kstat_seq_ops = { static kstat_module_t * kstat_find_module(char *name) { + ASSERT(MUTEX_HELD(&kstat_module_lock)); + kstat_module_t *module = NULL; list_for_each_entry(module, &kstat_module_list, ksm_module_list) { @@ -380,33 +386,75 @@ kstat_find_module(char *name) return (NULL); } -static kstat_module_t * -kstat_create_module(char *name) +static void +kstat_delete_module(kstat_module_t *module) { - kstat_module_t *module; - struct proc_dir_entry *pde; + ASSERT(MUTEX_HELD(&kstat_module_lock)); + ASSERT(list_empty(&module->ksm_kstat_list)); + ASSERT0(module->ksm_nchildren); - pde = proc_mkdir(name, proc_spl_kstat); - if (pde == NULL) - return (NULL); + kstat_module_t *parent = module->ksm_parent; - module = kmem_alloc(sizeof (kstat_module_t), KM_SLEEP); - module->ksm_proc = pde; - strlcpy(module->ksm_name, name, KSTAT_STRLEN); - INIT_LIST_HEAD(&module->ksm_kstat_list); - list_add_tail(&module->ksm_module_list, &kstat_module_list); + char *p = module->ksm_name, *frag; + while (p != NULL && (frag = strsep(&p, "/"))) {} - return (module); + remove_proc_entry(frag, parent ? parent->ksm_proc : proc_spl_kstat); + list_del(&module->ksm_module_list); + kmem_free(module, sizeof (kstat_module_t)); + if (parent) { + parent->ksm_nchildren--; + if (parent->ksm_nchildren == 0 && + list_empty(&parent->ksm_kstat_list)) + kstat_delete_module(parent); + } } -static void -kstat_delete_module(kstat_module_t *module) +static kstat_module_t * +kstat_create_module(char *name) { - ASSERT(list_empty(&module->ksm_kstat_list)); - remove_proc_entry(module->ksm_name, proc_spl_kstat); - list_del(&module->ksm_module_list); - kmem_free(module, sizeof (kstat_module_t)); + ASSERT(MUTEX_HELD(&kstat_module_lock)); + + char buf[KSTAT_STRLEN]; + kstat_module_t *module, *parent; + + (void) strlcpy(buf, name, KSTAT_STRLEN); + + parent = NULL; + char *p = buf, *frag; + while ((frag = strsep(&p, "/")) != NULL) { + module = kstat_find_module(buf); + if (module == NULL) { + struct proc_dir_entry *pde = proc_mkdir(frag, + parent ? parent->ksm_proc : proc_spl_kstat); + if (pde == NULL) { + cmn_err(CE_WARN, "kstat_create('%s'): " + "module dir create failed", buf); + if (parent) + kstat_delete_module(parent); + return (NULL); + } + + module = kmem_zalloc(sizeof (kstat_module_t), KM_SLEEP); + module->ksm_proc = pde; + strlcpy(module->ksm_name, buf, KSTAT_STRLEN); + INIT_LIST_HEAD(&module->ksm_kstat_list); + list_add_tail(&module->ksm_module_list, + &kstat_module_list); + + if (parent != NULL) { + module->ksm_parent = parent; + parent->ksm_nchildren++; + } + } + + parent = module; + if (p != NULL) + p[-1] = '/'; + } + + return (module); + } static int @@ -625,12 +673,20 @@ kstat_proc_entry_install(kstat_proc_entry_t *kpep, mode_t mode, } /* - * Only one entry by this name per-module, on failure the module - * shouldn't be deleted because we know it has at least one entry. + * We can only have one entry of this name per module. If one already + * exists, replace it by first removing the proc entry, then removing + * it from the list. The kstat itself lives on; it just can't be + * inspected through the filesystem. */ list_for_each_entry(tmp, &module->ksm_kstat_list, kpe_list) { - if (strncmp(tmp->kpe_name, kpep->kpe_name, KSTAT_STRLEN) == 0) - goto out; + if (tmp->kpe_proc != NULL && + strncmp(tmp->kpe_name, kpep->kpe_name, KSTAT_STRLEN) == 0) { + ASSERT3P(tmp->kpe_owner, ==, module); + remove_proc_entry(tmp->kpe_name, module->ksm_proc); + tmp->kpe_proc = NULL; + list_del_init(&tmp->kpe_list); + break; + } } list_add_tail(&kpep->kpe_list, &module->ksm_kstat_list); From 911c8dcc0e0261592b85c9be39d2421a58121703 Mon Sep 17 00:00:00 2001 From: Rob Norris Date: Thu, 16 May 2024 14:58:15 +1000 Subject: [PATCH 2/2] freebsd/kstat: allow multi-level module names This extends the existing special-case for zfs/poolname to split and create any number of intermediate sysctl names, so that multi-level module names are possible. Sponsored-by: Klara, Inc. Sponsored-by: Syneto Signed-off-by: Rob Norris --- module/os/freebsd/spl/spl_kstat.c | 87 ++++++++++++++++--------------- 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/module/os/freebsd/spl/spl_kstat.c b/module/os/freebsd/spl/spl_kstat.c index 1fb294b37097..1af1e0a3846b 100644 --- a/module/os/freebsd/spl/spl_kstat.c +++ b/module/os/freebsd/spl/spl_kstat.c @@ -28,6 +28,10 @@ * [1] https://illumos.org/man/1M/kstat * [2] https://illumos.org/man/9f/kstat_create */ +/* + * Copyright (c) 2024-2025, Klara, Inc. + * Copyright (c) 2024-2025, Syneto + */ #include #include @@ -288,7 +292,7 @@ __kstat_create(const char *module, int instance, const char *name, char buf[KSTAT_STRLEN]; struct sysctl_oid *root; kstat_t *ksp; - char *pool; + char *p, *frag; KASSERT(instance == 0, ("instance=%d", instance)); if ((ks_type == KSTAT_TYPE_INTR) || (ks_type == KSTAT_TYPE_IO)) @@ -346,74 +350,54 @@ __kstat_create(const char *module, int instance, const char *name, else ksp->ks_data = kmem_zalloc(ksp->ks_data_size, KM_SLEEP); - /* - * Some kstats use a module name like "zfs/poolname" to distinguish a - * set of kstats belonging to a specific pool. Split on '/' to add an - * extra node for the pool name if needed. - */ + sysctl_ctx_init(&ksp->ks_sysctl_ctx); + (void) strlcpy(buf, module, KSTAT_STRLEN); - module = buf; - pool = strchr(module, '/'); - if (pool != NULL) - *pool++ = '\0'; /* - * Create sysctl tree for those statistics: - * - * kstat.[.].. + * Walk over the module name, splitting on '/', and create the + * intermediate nodes. */ - sysctl_ctx_init(&ksp->ks_sysctl_ctx); - root = SYSCTL_ADD_NODE(&ksp->ks_sysctl_ctx, - SYSCTL_STATIC_CHILDREN(_kstat), OID_AUTO, module, CTLFLAG_RW, 0, - ""); - if (root == NULL) { - printf("%s: Cannot create kstat.%s tree!\n", __func__, module); - sysctl_ctx_free(&ksp->ks_sysctl_ctx); - free(ksp, M_KSTAT); - return (NULL); - } - if (pool != NULL) { - root = SYSCTL_ADD_NODE(&ksp->ks_sysctl_ctx, - SYSCTL_CHILDREN(root), OID_AUTO, pool, CTLFLAG_RW, 0, ""); + root = NULL; + p = buf; + while ((frag = strsep(&p, "/")) != NULL) { + root = SYSCTL_ADD_NODE(&ksp->ks_sysctl_ctx, root ? + SYSCTL_CHILDREN(root) : SYSCTL_STATIC_CHILDREN(_kstat), + OID_AUTO, frag, CTLFLAG_RW, 0, ""); if (root == NULL) { - printf("%s: Cannot create kstat.%s.%s tree!\n", - __func__, module, pool); + printf("%s: Cannot create kstat.%s tree!\n", + __func__, buf); sysctl_ctx_free(&ksp->ks_sysctl_ctx); free(ksp, M_KSTAT); return (NULL); } + if (p != NULL) + p[-1] = '.'; } + root = SYSCTL_ADD_NODE(&ksp->ks_sysctl_ctx, SYSCTL_CHILDREN(root), OID_AUTO, class, CTLFLAG_RW, 0, ""); if (root == NULL) { - if (pool != NULL) - printf("%s: Cannot create kstat.%s.%s.%s tree!\n", - __func__, module, pool, class); - else - printf("%s: Cannot create kstat.%s.%s tree!\n", - __func__, module, class); + printf("%s: Cannot create kstat.%s.%s tree!\n", + __func__, buf, class); sysctl_ctx_free(&ksp->ks_sysctl_ctx); free(ksp, M_KSTAT); return (NULL); } + if (ksp->ks_type == KSTAT_TYPE_NAMED) { root = SYSCTL_ADD_NODE(&ksp->ks_sysctl_ctx, SYSCTL_CHILDREN(root), OID_AUTO, name, CTLFLAG_RW, 0, ""); if (root == NULL) { - if (pool != NULL) - printf("%s: Cannot create kstat.%s.%s.%s.%s " - "tree!\n", __func__, module, pool, class, - name); - else - printf("%s: Cannot create kstat.%s.%s.%s " - "tree!\n", __func__, module, class, name); + printf("%s: Cannot create kstat.%s.%s.%s tree!\n", + __func__, buf, class, name); sysctl_ctx_free(&ksp->ks_sysctl_ctx); free(ksp, M_KSTAT); return (NULL); } - } + ksp->ks_sysctl_root = root; return (ksp); @@ -437,7 +421,26 @@ kstat_install_named(kstat_t *ksp) if (ksent->data_type != 0) { typelast = ksent->data_type; namelast = ksent->name; + + /* + * If a sysctl with this name already exists on this on + * this root, first remove it by deleting it from its + * old context, and then destroying it. + */ + struct sysctl_oid *oid = NULL; + SYSCTL_FOREACH(oid, + SYSCTL_CHILDREN(ksp->ks_sysctl_root)) { + if (strcmp(oid->oid_name, namelast) == 0) { + kstat_t *oldksp = + (kstat_t *)oid->oid_arg1; + sysctl_ctx_entry_del( + &oldksp->ks_sysctl_ctx, oid); + sysctl_remove_oid(oid, 1, 0); + break; + } + } } + switch (typelast) { case KSTAT_DATA_CHAR: /* Not Implemented */