* Re: [PATCH] tracing: separate module tracepoint strings from trace_printk formats
From: Petr Pavlu @ 2026-04-14 11:37 UTC (permalink / raw)
To: Cao Ruichuang; +Cc: linux-trace-kernel, linux-kernel, mhiramat, rostedt
In-Reply-To: <20260413123359.32517-1-create0818@163.com>
On 4/13/26 2:33 PM, Cao Ruichuang wrote:
> The previous module tracepoint_string() fix took the smallest
> implementation path and reused the existing module trace_printk format
> storage.
>
> That was enough to make module __tracepoint_str entries show up in
> printk_formats and be accepted by trace_is_tracepoint_string(), but it
> also made those copied mappings persist after module unload. That does
> not match the expected module lifetime semantics.
>
> Handle module tracepoint_string() mappings separately instead of mixing
> them into the module trace_printk format list. Keep copying the strings
> into tracing-managed storage while the module is loaded, but track them
> on their own list and drop them again on MODULE_STATE_GOING.
>
> Keep module trace_printk format handling unchanged.
>
> This split is intentional: module trace_printk formats and module
> tracepoint_string() mappings do not have the same lifetime requirements.
> Keeping them in one shared structure would either preserve
> tracepoint_string() mappings too long again, or require mixed
> ownership/refcount rules in a trace_printk-oriented structure.
>
> The separate module tracepoint_string() list intentionally keeps one
> copied mapping per module entry instead of trying to share copies across
> modules by string contents. printk_formats is address-based, and sharing
> those copies would add another layer of shared ownership/refcounting
> without changing the lifetime rule this fix is trying to restore.
I believe it would be more productive to first answer the question in my
previous email and give others time to comment on the noted issue and
discuss a possible solution. The previous patch hasn't been accepted
yet, so sending a new patch on top of it just hours after my comment
seems unusual.
The patch contains some unrelated changes, such as renaming last_index
to next_index in find_next(). I suspect it was generated with the help
of AI but it isn't attributed as such. Please check
Documentation/process/coding-assistants.rst.
The implementation splits the trace_bprintk_fmt_list into two lists.
While this looks sensible, it creates nearly duplicate code for handling
the two lists, for example, hold_module_trace_format() and
hold_module_trace_format(), or lookup_mod_format_ptr() and
lookup_mod_tracepoint_ptr(). An alternative would be to keep one list
and have a 'struct module *mod' member to differentiate between the
strings. 'mod == NULL' for printk fmts and 'mod != NULL' for tracepoint
strings.
I also considered whether kernel/trace/trace_printk.c could avoid
tracking tracepoint strings altogether and instead have
find_next_mod_entry() directly traverse the modules list and
module::tracepoint_strings_start. However, this would require taking
both btrace_mutex and the RCU lock, and I believe it would be more
complex.
Since the tracepoint strings are now tracked only for the duration of
a module's lifetime, kernel/trace/trace_printk.c doesn't need to copy
the strings and can reference directly the original module data.
Alternatively, the original strings could be placed in an .init section
so that once trace_printk.c copies them and the module is initialized,
they get dropped.
--
Thanks,
Petr
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217196
> Signed-off-by: Cao Ruichuang <create0818@163.com>
> ---
> include/linux/tracepoint.h | 9 +-
> kernel/trace/trace_printk.c | 250 ++++++++++++++++++++++--------------
> 2 files changed, 157 insertions(+), 102 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index f14da542402..aec598a4017 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -479,11 +479,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
> *
> * For built-in code, the tracing system uses the original string address.
> * For modules, the tracing code saves tracepoint strings into
> - * tracing-managed storage when the module loads, so their mappings remain
> - * available through printk_formats and trace string checks even after the
> - * module's own memory goes away. As long as the string does not change
> - * during the life of the module, it is fine to use tracepoint_string()
> - * within a module.
> + * tracing-managed storage while the module is loaded, and drops those
> + * mappings again when the module unloads. As long as the string does not
> + * change during the life of the module, it is fine to use
> + * tracepoint_string() within a module.
> */
> #define tracepoint_string(str) \
> ({ \
> diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
> index 9f67ce42ef6..0420ffcff93 100644
> --- a/kernel/trace/trace_printk.c
> +++ b/kernel/trace/trace_printk.c
> @@ -21,24 +21,24 @@
>
> #ifdef CONFIG_MODULES
>
> -/*
> - * modules trace_printk() formats and tracepoint_string() strings are
> - * autosaved in struct trace_bprintk_fmt, which are queued on
> - * trace_bprintk_fmt_list.
> - */
> +/* module trace_printk() formats are autosaved on trace_bprintk_fmt_list. */
> static LIST_HEAD(trace_bprintk_fmt_list);
> +/* module tracepoint_string() copies live on tracepoint_string_list. */
> +static LIST_HEAD(tracepoint_string_list);
>
> -/* serialize accesses to trace_bprintk_fmt_list */
> +/* serialize accesses to module format and tracepoint-string lists */
> static DEFINE_MUTEX(btrace_mutex);
>
> struct trace_bprintk_fmt {
> struct list_head list;
> const char *fmt;
> - unsigned int type;
> };
>
> -#define TRACE_BPRINTK_TYPE BIT(0)
> -#define TRACE_TRACEPOINT_TYPE BIT(1)
> +struct tracepoint_string_entry {
> + struct list_head list;
> + struct module *mod;
> + const char *str;
> +};
>
> static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
> {
> @@ -54,24 +54,21 @@ static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
> return NULL;
> }
>
> -static void hold_module_trace_format(const char **start, const char **end,
> - unsigned int type)
> +static void hold_module_trace_bprintk_format(const char **start, const char **end)
> {
> const char **iter;
> char *fmt;
>
> /* allocate the trace_printk per cpu buffers */
> - if ((type & TRACE_BPRINTK_TYPE) && start != end)
> + if (start != end)
> trace_printk_init_buffers();
>
> mutex_lock(&btrace_mutex);
> for (iter = start; iter < end; iter++) {
> struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
> if (tb_fmt) {
> - if (!IS_ERR(tb_fmt)) {
> - tb_fmt->type |= type;
> + if (!IS_ERR(tb_fmt))
> *iter = tb_fmt->fmt;
> - }
> continue;
> }
>
> @@ -83,7 +80,6 @@ static void hold_module_trace_format(const char **start, const char **end,
> list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
> strcpy(fmt, *iter);
> tb_fmt->fmt = fmt;
> - tb_fmt->type = type;
> } else
> kfree(tb_fmt);
> }
> @@ -93,89 +89,156 @@ static void hold_module_trace_format(const char **start, const char **end,
> mutex_unlock(&btrace_mutex);
> }
>
> +static void hold_module_tracepoint_strings(struct module *mod,
> + const char **start,
> + const char **end)
> +{
> + const char **iter;
> +
> + mutex_lock(&btrace_mutex);
> + for (iter = start; iter < end; iter++) {
> + struct tracepoint_string_entry *tp_entry;
> + char *str;
> +
> + tp_entry = kmalloc_obj(*tp_entry);
> + if (!tp_entry)
> + continue;
> +
> + str = kstrdup(*iter, GFP_KERNEL);
> + if (!str) {
> + kfree(tp_entry);
> + continue;
> + }
> +
> + tp_entry->mod = mod;
> + tp_entry->str = str;
> + list_add_tail(&tp_entry->list, &tracepoint_string_list);
> + *iter = tp_entry->str;
> + }
> + mutex_unlock(&btrace_mutex);
> +}
> +
> +static void release_module_tracepoint_strings(struct module *mod)
> +{
> + struct tracepoint_string_entry *tp_entry, *n;
> +
> + mutex_lock(&btrace_mutex);
> + list_for_each_entry_safe(tp_entry, n, &tracepoint_string_list, list) {
> + if (tp_entry->mod != mod)
> + continue;
> + list_del(&tp_entry->list);
> + kfree(tp_entry->str);
> + kfree(tp_entry);
> + }
> + mutex_unlock(&btrace_mutex);
> +}
> +
> static int module_trace_format_notify(struct notifier_block *self,
> unsigned long val, void *data)
> {
> struct module *mod = data;
>
> - if (val != MODULE_STATE_COMING)
> - return NOTIFY_OK;
> + switch (val) {
> + case MODULE_STATE_COMING:
> + if (mod->num_trace_bprintk_fmt) {
> + const char **start = mod->trace_bprintk_fmt_start;
> + const char **end = start + mod->num_trace_bprintk_fmt;
>
> - if (mod->num_trace_bprintk_fmt) {
> - const char **start = mod->trace_bprintk_fmt_start;
> - const char **end = start + mod->num_trace_bprintk_fmt;
> + hold_module_trace_bprintk_format(start, end);
> + }
> +
> + if (mod->num_tracepoint_strings) {
> + const char **start = mod->tracepoint_strings_start;
> + const char **end = start + mod->num_tracepoint_strings;
>
> - hold_module_trace_format(start, end, TRACE_BPRINTK_TYPE);
> + hold_module_tracepoint_strings(mod, start, end);
> + }
> + break;
> + case MODULE_STATE_GOING:
> + release_module_tracepoint_strings(mod);
> + break;
> }
>
> - if (mod->num_tracepoint_strings) {
> - const char **start = mod->tracepoint_strings_start;
> - const char **end = start + mod->num_tracepoint_strings;
> + return NOTIFY_OK;
> +}
>
> - hold_module_trace_format(start, end, TRACE_TRACEPOINT_TYPE);
> +static const char **find_first_mod_entry(void)
> +{
> + struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> +
> + if (!list_empty(&trace_bprintk_fmt_list)) {
> + tb_fmt = list_first_entry(&trace_bprintk_fmt_list,
> + typeof(*tb_fmt), list);
> + return &tb_fmt->fmt;
> }
>
> - return NOTIFY_OK;
> + if (!list_empty(&tracepoint_string_list)) {
> + tp_entry = list_first_entry(&tracepoint_string_list,
> + typeof(*tp_entry), list);
> + return &tp_entry->str;
> + }
> +
> + return NULL;
> }
>
> -/*
> - * The debugfs/tracing/printk_formats file maps the addresses with
> - * the ASCII formats that are used in the bprintk events in the
> - * buffer. For userspace tools to be able to decode the events from
> - * the buffer, they need to be able to map the address with the format.
> - *
> - * The addresses of the bprintk formats are in their own section
> - * __trace_printk_fmt. But for modules we copy them into a link list.
> - * The code to print the formats and their addresses passes around the
> - * address of the fmt string. If the fmt address passed into the seq
> - * functions is within the kernel core __trace_printk_fmt section, then
> - * it simply uses the next pointer in the list.
> - *
> - * When the fmt pointer is outside the kernel core __trace_printk_fmt
> - * section, then we need to read the link list pointers. The trick is
> - * we pass the address of the string to the seq function just like
> - * we do for the kernel core formats. To get back the structure that
> - * holds the format, we simply use container_of() and then go to the
> - * next format in the list.
> - */
> -static const char **
> -find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> +static struct trace_bprintk_fmt *lookup_mod_format_ptr(const char **fmt_ptr)
> {
> - struct trace_bprintk_fmt *mod_fmt;
> + struct trace_bprintk_fmt *tb_fmt;
>
> - if (list_empty(&trace_bprintk_fmt_list))
> - return NULL;
> + list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
> + if (fmt_ptr == &tb_fmt->fmt)
> + return tb_fmt;
> + }
>
> - /*
> - * v will point to the address of the fmt record from t_next
> - * v will be NULL from t_start.
> - * If this is the first pointer or called from start
> - * then we need to walk the list.
> - */
> - if (!v || start_index == *pos) {
> - struct trace_bprintk_fmt *p;
> -
> - /* search the module list */
> - list_for_each_entry(p, &trace_bprintk_fmt_list, list) {
> - if (start_index == *pos)
> - return &p->fmt;
> - start_index++;
> + return NULL;
> +}
> +
> +static struct tracepoint_string_entry *lookup_mod_tracepoint_ptr(const char **str_ptr)
> +{
> + struct tracepoint_string_entry *tp_entry;
> +
> + list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
> + if (str_ptr == &tp_entry->str)
> + return tp_entry;
> + }
> +
> + return NULL;
> +}
> +
> +static const char **find_next_mod_entry(int start_index, void *v, loff_t *pos)
> +{
> + struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> +
> + if (!v || start_index == *pos)
> + return find_first_mod_entry();
> +
> + tb_fmt = lookup_mod_format_ptr(v);
> + if (tb_fmt) {
> + if (tb_fmt->list.next != &trace_bprintk_fmt_list) {
> + tb_fmt = list_next_entry(tb_fmt, list);
> + return &tb_fmt->fmt;
> }
> - /* pos > index */
> +
> + if (!list_empty(&tracepoint_string_list)) {
> + tp_entry = list_first_entry(&tracepoint_string_list,
> + typeof(*tp_entry), list);
> + return &tp_entry->str;
> + }
> +
> return NULL;
> }
>
> - /*
> - * v points to the address of the fmt field in the mod list
> - * structure that holds the module print format.
> - */
> - mod_fmt = container_of(v, typeof(*mod_fmt), fmt);
> - if (mod_fmt->list.next == &trace_bprintk_fmt_list)
> + tp_entry = lookup_mod_tracepoint_ptr(v);
> + if (!tp_entry)
> return NULL;
>
> - mod_fmt = container_of(mod_fmt->list.next, typeof(*mod_fmt), list);
> + if (tp_entry->list.next == &tracepoint_string_list)
> + return NULL;
>
> - return &mod_fmt->fmt;
> + tp_entry = list_next_entry(tp_entry, list);
> + return &tp_entry->str;
> }
>
> static void format_mod_start(void)
> @@ -195,8 +258,8 @@ module_trace_format_notify(struct notifier_block *self,
> {
> return NOTIFY_OK;
> }
> -static inline const char **
> -find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
> +static inline const char **find_next_mod_entry(int start_index, void *v,
> + loff_t *pos)
> {
> return NULL;
> }
> @@ -274,7 +337,7 @@ bool trace_is_tracepoint_string(const char *str)
> {
> const char **ptr = __start___tracepoint_str;
> #ifdef CONFIG_MODULES
> - struct trace_bprintk_fmt *tb_fmt;
> + struct tracepoint_string_entry *tp_entry;
> #endif
>
> for (ptr = __start___tracepoint_str; ptr < __stop___tracepoint_str; ptr++) {
> @@ -284,8 +347,8 @@ bool trace_is_tracepoint_string(const char *str)
>
> #ifdef CONFIG_MODULES
> mutex_lock(&btrace_mutex);
> - list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
> - if ((tb_fmt->type & TRACE_TRACEPOINT_TYPE) && str == tb_fmt->fmt) {
> + list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
> + if (str == tp_entry->str) {
> mutex_unlock(&btrace_mutex);
> return true;
> }
> @@ -297,9 +360,8 @@ bool trace_is_tracepoint_string(const char *str)
>
> static const char **find_next(void *v, loff_t *pos)
> {
> - const char **fmt = v;
> int start_index;
> - int last_index;
> + int next_index;
>
> start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;
>
> @@ -307,25 +369,19 @@ static const char **find_next(void *v, loff_t *pos)
> return __start___trace_bprintk_fmt + *pos;
>
> /*
> - * The __tracepoint_str section is treated the same as the
> - * __trace_printk_fmt section. The difference is that the
> - * __trace_printk_fmt section should only be used by trace_printk()
> - * in a debugging environment, as if anything exists in that section
> - * the trace_prink() helper buffers are allocated, which would just
> - * waste space in a production environment.
> - *
> - * The __tracepoint_str sections on the other hand are used by
> - * tracepoints which need to map pointers to their strings to
> - * the ASCII text for userspace.
> + * Built-in __tracepoint_str entries are exported directly from the
> + * core section. Module tracepoint_string() mappings are kept on a
> + * separate tracing-managed list below, because their lifetime is tied
> + * to module load/unload and differs from module trace_printk() formats.
> */
> - last_index = start_index;
> + next_index = start_index;
> start_index = __stop___tracepoint_str - __start___tracepoint_str;
>
> - if (*pos < last_index + start_index)
> - return __start___tracepoint_str + (*pos - last_index);
> + if (*pos < next_index + start_index)
> + return __start___tracepoint_str + (*pos - next_index);
>
> - start_index += last_index;
> - return find_next_mod_format(start_index, v, fmt, pos);
> + start_index += next_index;
> + return find_next_mod_entry(start_index, v, pos);
> }
>
> static void *
^ permalink raw reply
* [PATCH v6 5/5] tracing/fprobe: Fix to unregister ftrace_ops if it is empty on module unloading
From: Masami Hiramatsu (Google) @ 2026-04-14 9:15 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
In-Reply-To: <177615807787.1165997.921227352050738693.stgit@mhiramat.tok.corp.google.com>
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Fix fprobe to unregister ftrace_ops if corresponding type of fprobe
does not exist on the fprobe_ip_table and it is expected to be empty
when unloading modules.
Since ftrace thinks that the empty hash means everything to be traced,
if we set fprobes only on the unloaded module, all functions are traced
unexpectedly after unloading module.
e.g.
# modprobe xt_LOG.ko
# echo 'f:test log_tg*' > dynamic_events
# echo 1 > events/fprobes/test/enable
# cat enabled_functions
log_tg [xt_LOG] (1) tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
log_tg_check [xt_LOG] (1) tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
log_tg_destroy [xt_LOG] (1) tramp: 0xffffffffa0004000 (fprobe_ftrace_entry+0x0/0x490) ->fprobe_ftrace_entry+0x0/0x490
# rmmod xt_LOG
# wc -l enabled_functions
34085 enabled_functions
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v6:
- Newly added.
---
kernel/trace/fprobe.c | 191 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 143 insertions(+), 48 deletions(-)
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 1767c2b0884c..5cbe7deb855a 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -79,7 +79,7 @@ static const struct rhashtable_params fprobe_rht_params = {
};
/* Node insertion and deletion requires the fprobe_mutex */
-static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
+static int __insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
{
int ret;
@@ -92,7 +92,7 @@ static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
return ret;
}
-static void delete_fprobe_node(struct fprobe_hlist_node *node)
+static void __delete_fprobe_node(struct fprobe_hlist_node *node)
{
lockdep_assert_held(&fprobe_mutex);
@@ -250,7 +250,63 @@ static inline int __fprobe_kprobe_handler(unsigned long ip, unsigned long parent
return ret;
}
+static int fprobe_fgraph_entry(struct ftrace_graph_ent *trace, struct fgraph_ops *gops,
+ struct ftrace_regs *fregs);
+static void fprobe_return(struct ftrace_graph_ret *trace,
+ struct fgraph_ops *gops,
+ struct ftrace_regs *fregs);
+
+static struct fgraph_ops fprobe_graph_ops = {
+ .entryfunc = fprobe_fgraph_entry,
+ .retfunc = fprobe_return,
+};
+static int fprobe_graph_active;
+/* Number of fgraph fprobes */
+static int nr_fgraph_fprobes;
+
+/* Add @addrs to the ftrace filter and register fgraph if needed. */
+static int fprobe_graph_add_ips(unsigned long *addrs, int num)
+{
+ int ret;
+
+ lockdep_assert_held(&fprobe_mutex);
+
+ ret = ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 0, 0);
+ if (ret)
+ return ret;
+
+ if (!fprobe_graph_active) {
+ ret = register_ftrace_graph(&fprobe_graph_ops);
+ if (WARN_ON_ONCE(ret)) {
+ ftrace_free_filter(&fprobe_graph_ops.ops);
+ return ret;
+ }
+ }
+ fprobe_graph_active++;
+ return 0;
+}
+
+/* Remove @addrs from the ftrace filter and unregister fgraph if possible. */
+static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
+{
+ lockdep_assert_held(&fprobe_mutex);
+
+ if (!fprobe_graph_active)
+ return;
+ fprobe_graph_active--;
+ if (!fprobe_graph_active) {
+ unregister_ftrace_graph(&fprobe_graph_ops);
+ ftrace_free_filter(&fprobe_graph_ops.ops);
+ }
+
+ if (num)
+ ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 1, 0);
+}
+
#if defined(CONFIG_DYNAMIC_FTRACE_WITH_ARGS) || defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS)
+/* Number of ftrace fprobes */
+static int nr_ftrace_fprobes;
+
/* ftrace_ops callback, this processes fprobes which have only entry_handler. */
static void fprobe_ftrace_entry(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *ops, struct ftrace_regs *fregs)
@@ -320,9 +376,14 @@ static void fprobe_ftrace_remove_ips(unsigned long *addrs, int num)
{
lockdep_assert_held(&fprobe_mutex);
- fprobe_ftrace_active--;
if (!fprobe_ftrace_active)
+ return;
+
+ fprobe_ftrace_active--;
+ if (!fprobe_ftrace_active) {
unregister_ftrace_function(&fprobe_ftrace_ops);
+ ftrace_free_filter(&fprobe_ftrace_ops);
+ }
if (num)
ftrace_set_filter_ips(&fprobe_ftrace_ops, addrs, num, 1, 0);
}
@@ -332,6 +393,40 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
return !fp->exit_handler;
}
+/* Node insertion and deletion requires the fprobe_mutex */
+static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
+{
+ int ret;
+
+ lockdep_assert_held(&fprobe_mutex);
+
+ ret = __insert_fprobe_node(node, fp);
+ if (!ret) {
+ if (fprobe_is_ftrace(fp))
+ nr_ftrace_fprobes++;
+ else
+ nr_fgraph_fprobes++;
+ }
+
+ return ret;
+}
+
+static void delete_fprobe_node(struct fprobe_hlist_node *node)
+{
+ struct fprobe *fp;
+
+ lockdep_assert_held(&fprobe_mutex);
+
+ fp = READ_ONCE(node->fp);
+ if (fp) {
+ if (fprobe_is_ftrace(fp))
+ nr_ftrace_fprobes--;
+ else
+ nr_fgraph_fprobes--;
+ }
+ __delete_fprobe_node(node);
+}
+
static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace)
{
struct rhlist_head *head, *pos;
@@ -361,8 +456,19 @@ static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace)
#ifdef CONFIG_MODULES
static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
- ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
- ftrace_set_filter_ips(&fprobe_ftrace_ops, ips, cnt, 1, 0);
+ if (!nr_fgraph_fprobes && fprobe_graph_active) {
+ unregister_ftrace_graph(&fprobe_graph_ops);
+ ftrace_free_filter(&fprobe_graph_ops.ops);
+ fprobe_graph_active = 0;
+ } else
+ ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
+
+ if (!nr_ftrace_fprobes && fprobe_ftrace_active) {
+ unregister_ftrace_function(&fprobe_ftrace_ops);
+ ftrace_free_filter(&fprobe_ftrace_ops);
+ fprobe_ftrace_active = 0;
+ } else
+ ftrace_set_filter_ips(&fprobe_ftrace_ops, ips, cnt, 1, 0);
}
#endif
#else
@@ -380,6 +486,32 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
return false;
}
+/* Node insertion and deletion requires the fprobe_mutex */
+static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
+{
+ int ret;
+
+ lockdep_assert_held(&fprobe_mutex);
+
+ ret = __insert_fprobe_node(node, fp);
+ if (!ret)
+ nr_fgraph_fprobes++;
+
+ return ret;
+}
+
+static void delete_fprobe_node(struct fprobe_hlist_node *node)
+{
+ struct fprobe *fp;
+
+ lockdep_assert_held(&fprobe_mutex);
+
+ fp = READ_ONCE(node->fp);
+ if (fp)
+ nr_fgraph_fprobes--;
+ __delete_fprobe_node(node);
+}
+
static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused)
{
struct rhlist_head *head, *pos;
@@ -406,7 +538,12 @@ static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused)
#ifdef CONFIG_MODULES
static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
- ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
+ if (!nr_fgraph_fprobes && fprobe_graph_active) {
+ unregister_ftrace_graph(&fprobe_graph_ops);
+ ftrace_free_filter(&fprobe_graph_ops.ops);
+ fprobe_graph_active = 0;
+ } else
+ ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
}
#endif
#endif /* !CONFIG_DYNAMIC_FTRACE_WITH_ARGS && !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
@@ -536,48 +673,6 @@ static void fprobe_return(struct ftrace_graph_ret *trace,
}
NOKPROBE_SYMBOL(fprobe_return);
-static struct fgraph_ops fprobe_graph_ops = {
- .entryfunc = fprobe_fgraph_entry,
- .retfunc = fprobe_return,
-};
-static int fprobe_graph_active;
-
-/* Add @addrs to the ftrace filter and register fgraph if needed. */
-static int fprobe_graph_add_ips(unsigned long *addrs, int num)
-{
- int ret;
-
- lockdep_assert_held(&fprobe_mutex);
-
- ret = ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 0, 0);
- if (ret)
- return ret;
-
- if (!fprobe_graph_active) {
- ret = register_ftrace_graph(&fprobe_graph_ops);
- if (WARN_ON_ONCE(ret)) {
- ftrace_free_filter(&fprobe_graph_ops.ops);
- return ret;
- }
- }
- fprobe_graph_active++;
- return 0;
-}
-
-/* Remove @addrs from the ftrace filter and unregister fgraph if possible. */
-static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
-{
- lockdep_assert_held(&fprobe_mutex);
-
- fprobe_graph_active--;
- /* Q: should we unregister it ? */
- if (!fprobe_graph_active)
- unregister_ftrace_graph(&fprobe_graph_ops);
-
- if (num)
- ftrace_set_filter_ips(&fprobe_graph_ops.ops, addrs, num, 1, 0);
-}
-
#ifdef CONFIG_MODULES
#define FPROBE_IPS_BATCH_INIT 128
^ permalink raw reply related
* [PATCH v6 4/5] tracing/fprobe: Check the same type fprobe on table as the unregistered one
From: Masami Hiramatsu (Google) @ 2026-04-14 9:15 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
In-Reply-To: <177615807787.1165997.921227352050738693.stgit@mhiramat.tok.corp.google.com>
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Commit 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
introduced a different ftrace_ops for entry-only fprobes.
However, when unregistering an fprobe, the kernel only checks if another
fprobe exists at the same address, without checking which type of fprobe
it is.
If different fprobes are registered at the same address, the same address
will be registered in both fgraph_ops and ftrace_ops, but only one of
them will be deleted when unregistering. (the one removed first will not
be deleted from the ops).
This results in junk entries remaining in either fgraph_ops or ftrace_ops.
For example:
=======
cd /sys/kernel/tracing
# 'Add entry and exit events on the same place'
echo 'f:event1 vfs_read' >> dynamic_events
echo 'f:event2 vfs_read%return' >> dynamic_events
# 'Enable both of them'
echo 1 > events/fprobes/enable
cat enabled_functions
vfs_read (2) ->arch_ftrace_ops_list_func+0x0/0x210
# 'Disable and remove exit event'
echo 0 > events/fprobes/event2/enable
echo -:event2 >> dynamic_events
# 'Disable and remove all events'
echo 0 > events/fprobes/enable
echo > dynamic_events
# 'Add another event'
echo 'f:event3 vfs_open%return' > dynamic_events
cat dynamic_events
f:fprobes/event3 vfs_open%return
echo 1 > events/fprobes/enable
cat enabled_functions
vfs_open (1) tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
vfs_read (1) tramp: 0xffffffffa0001000 (ftrace_graph_func+0x0/0x60) ->ftrace_graph_func+0x0/0x60 subops: {ent:fprobe_fgraph_entry+0x0/0x620 ret:fprobe_return+0x0/0x150}
=======
As you can see, an entry for the vfs_read remains.
To fix this issue, when unregistering, the kernel should also check if
there is the same type of fprobes still exist at the same address, and
if not, delete its entry from either fgraph_ops or ftrace_ops.
Fixes: 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
kernel/trace/fprobe.c | 85 +++++++++++++++++++++++++++++++++++++------------
1 file changed, 65 insertions(+), 20 deletions(-)
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 2059d8d83b4c..1767c2b0884c 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -92,11 +92,8 @@ static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
return ret;
}
-/* Return true if there are synonims */
-static bool delete_fprobe_node(struct fprobe_hlist_node *node)
+static void delete_fprobe_node(struct fprobe_hlist_node *node)
{
- bool ret;
-
lockdep_assert_held(&fprobe_mutex);
/* Avoid double deleting and non-inserted nodes */
@@ -105,13 +102,6 @@ static bool delete_fprobe_node(struct fprobe_hlist_node *node)
rhltable_remove(&fprobe_ip_table, &node->hlist,
fprobe_rht_params);
}
-
- rcu_read_lock();
- ret = !!rhltable_lookup(&fprobe_ip_table, &node->addr,
- fprobe_rht_params);
- rcu_read_unlock();
-
- return ret;
}
/* Check existence of the fprobe */
@@ -342,6 +332,32 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
return !fp->exit_handler;
}
+static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace)
+{
+ struct rhlist_head *head, *pos;
+ struct fprobe_hlist_node *node;
+ struct fprobe *fp;
+
+ guard(rcu)();
+ head = rhltable_lookup(&fprobe_ip_table, &ip,
+ fprobe_rht_params);
+ if (!head)
+ return false;
+ /* We have to check the same type on the list. */
+ rhl_for_each_entry_rcu(node, pos, head, hlist) {
+ if (node->addr != ip)
+ break;
+ fp = READ_ONCE(node->fp);
+ if (likely(fp)) {
+ if ((!ftrace && fp->exit_handler) ||
+ (ftrace && !fp->exit_handler))
+ return true;
+ }
+ }
+
+ return false;
+}
+
#ifdef CONFIG_MODULES
static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
@@ -364,6 +380,29 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
return false;
}
+static bool fprobe_exists_on_hash(unsigned long ip, bool ftrace __maybe_unused)
+{
+ struct rhlist_head *head, *pos;
+ struct fprobe_hlist_node *node;
+ struct fprobe *fp;
+
+ guard(rcu)();
+ head = rhltable_lookup(&fprobe_ip_table, &ip,
+ fprobe_rht_params);
+ if (!head)
+ return false;
+ /* We only need to check fp is there. */
+ rhl_for_each_entry_rcu(node, pos, head, hlist) {
+ if (node->addr != ip)
+ break;
+ fp = READ_ONCE(node->fp);
+ if (likely(fp))
+ return true;
+ }
+
+ return false;
+}
+
#ifdef CONFIG_MODULES
static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
@@ -552,18 +591,25 @@ struct fprobe_addr_list {
static int fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node,
struct fprobe_addr_list *alist)
{
+ lockdep_assert_in_rcu_read_lock();
+
if (!within_module(node->addr, mod))
return 0;
- if (delete_fprobe_node(node))
- return 0;
+ delete_fprobe_node(node);
/* If no address list is available, we can't track this address. */
if (!alist->addrs)
return 0;
+ /*
+ * Don't care the type here, because all fprobes on the same
+ * address must be removed eventually.
+ */
+ if (!rhltable_lookup(&fprobe_ip_table, &node->addr, fprobe_rht_params)) {
+ alist->addrs[alist->index++] = node->addr;
+ if (alist->index == alist->size)
+ return -ENOSPC;
+ }
- alist->addrs[alist->index++] = node->addr;
- if (alist->index == alist->size)
- return -ENOSPC;
return 0;
}
@@ -934,10 +980,9 @@ static int unregister_fprobe_nolock(struct fprobe *fp, bool force)
/* Remove non-synonim ips from table and hash */
count = 0;
for (i = 0; i < hlist_array->size; i++) {
- if (delete_fprobe_node(&hlist_array->array[i]))
- continue;
-
- if (addrs)
+ delete_fprobe_node(&hlist_array->array[i]);
+ if (addrs && !fprobe_exists_on_hash(hlist_array->array[i].addr,
+ fprobe_is_ftrace(fp)))
addrs[count++] = hlist_array->array[i].addr;
}
del_fprobe_hash(fp);
^ permalink raw reply related
* [PATCH v6 3/5] tracing/fprobe: Avoid kcalloc() in rcu_read_lock section
From: Masami Hiramatsu (Google) @ 2026-04-14 9:15 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
In-Reply-To: <177615807787.1165997.921227352050738693.stgit@mhiramat.tok.corp.google.com>
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
fprobe_remove_node_in_module() is called under RCU read locked, but
this invokes kcalloc() if there are more than 8 fprobes installed
on the module. Sashiko warns it because kcalloc() can sleep [1].
[1] https://sashiko.dev/#/patchset/177552432201.853249.5125045538812833325.stgit%40mhiramat.tok.corp.google.com
To fix this issue, expand the batch size to 128 and do not expand
the fprobe_addr_list, but just cancel walking on fprobe_ip_table,
update fgraph/ftrace_ops and retry the loop again.
Fixes: 0de4c70d04a4 ("tracing: fprobe: use rhltable for fprobe_ip_table")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v6:
- Retry outside rhltable_walk_enter/exit() again.
Changes in v5:
- Skip updating ftrace_ops when fails to allocate memory in module
unloading.
Changes in v4:
- fix a build error typo in case of CONFIG_DYNAMIC_FTRACE=n.
Changes in v3:
- Retry inside rhltable_walk_enter/exit().
- Rename fprobe_set_ips() to fprobe_remove_ips().
- Rename 'retry' label to 'again'.
---
kernel/trace/fprobe.c | 92 ++++++++++++++++++++++++-------------------------
1 file changed, 45 insertions(+), 47 deletions(-)
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index 6a23bb787295..2059d8d83b4c 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -343,11 +343,10 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
}
#ifdef CONFIG_MODULES
-static void fprobe_set_ips(unsigned long *ips, unsigned int cnt, int remove,
- int reset)
+static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
- ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, remove, reset);
- ftrace_set_filter_ips(&fprobe_ftrace_ops, ips, cnt, remove, reset);
+ ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
+ ftrace_set_filter_ips(&fprobe_ftrace_ops, ips, cnt, 1, 0);
}
#endif
#else
@@ -366,10 +365,9 @@ static bool fprobe_is_ftrace(struct fprobe *fp)
}
#ifdef CONFIG_MODULES
-static void fprobe_set_ips(unsigned long *ips, unsigned int cnt, int remove,
- int reset)
+static void fprobe_remove_ips(unsigned long *ips, unsigned int cnt)
{
- ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, remove, reset);
+ ftrace_set_filter_ips(&fprobe_graph_ops.ops, ips, cnt, 1, 0);
}
#endif
#endif /* !CONFIG_DYNAMIC_FTRACE_WITH_ARGS && !CONFIG_DYNAMIC_FTRACE_WITH_REGS */
@@ -543,7 +541,7 @@ static void fprobe_graph_remove_ips(unsigned long *addrs, int num)
#ifdef CONFIG_MODULES
-#define FPROBE_IPS_BATCH_INIT 8
+#define FPROBE_IPS_BATCH_INIT 128
/* instruction pointer address list */
struct fprobe_addr_list {
int index;
@@ -551,45 +549,24 @@ struct fprobe_addr_list {
unsigned long *addrs;
};
-static int fprobe_addr_list_add(struct fprobe_addr_list *alist, unsigned long addr)
+static int fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node,
+ struct fprobe_addr_list *alist)
{
- unsigned long *addrs;
-
- /* Previously we failed to expand the list. */
- if (alist->index == alist->size)
- return -ENOSPC;
-
- alist->addrs[alist->index++] = addr;
- if (alist->index < alist->size)
+ if (!within_module(node->addr, mod))
return 0;
- /* Expand the address list */
- addrs = kcalloc(alist->size * 2, sizeof(*addrs), GFP_KERNEL);
- if (!addrs)
- return -ENOMEM;
-
- memcpy(addrs, alist->addrs, alist->size * sizeof(*addrs));
- alist->size *= 2;
- kfree(alist->addrs);
- alist->addrs = addrs;
+ if (delete_fprobe_node(node))
+ return 0;
+ /* If no address list is available, we can't track this address. */
+ if (!alist->addrs)
+ return 0;
+ alist->addrs[alist->index++] = node->addr;
+ if (alist->index == alist->size)
+ return -ENOSPC;
return 0;
}
-static void fprobe_remove_node_in_module(struct module *mod, struct fprobe_hlist_node *node,
- struct fprobe_addr_list *alist)
-{
- if (!within_module(node->addr, mod))
- return;
- if (delete_fprobe_node(node))
- return;
- /*
- * If failed to update alist, just continue to update hlist.
- * Therefore, at list user handler will not hit anymore.
- */
- fprobe_addr_list_add(alist, node->addr);
-}
-
/* Handle module unloading to manage fprobe_ip_table. */
static int fprobe_module_callback(struct notifier_block *nb,
unsigned long val, void *data)
@@ -598,29 +575,50 @@ static int fprobe_module_callback(struct notifier_block *nb,
struct fprobe_hlist_node *node;
struct rhashtable_iter iter;
struct module *mod = data;
+ bool retry;
if (val != MODULE_STATE_GOING)
return NOTIFY_DONE;
alist.addrs = kcalloc(alist.size, sizeof(*alist.addrs), GFP_KERNEL);
- /* If failed to alloc memory, we can not remove ips from hash. */
- if (!alist.addrs)
- return NOTIFY_DONE;
+ /*
+ * If failed to alloc memory, ftrace_ops will not be able to remove ips from
+ * hash, but we can still remove nodes from fprobe_ip_table, so we can avoid
+ * the potential wrong callback. So just print a warning here and try to
+ * continue without address list.
+ */
+ WARN_ONCE(!alist.addrs,
+ "Failed to allocate memory for fprobe_addr_list, ftrace_ops will not be updated");
mutex_lock(&fprobe_mutex);
+again:
+ retry = false;
+ alist.index = 0;
rhltable_walk_enter(&fprobe_ip_table, &iter);
do {
rhashtable_walk_start(&iter);
while ((node = rhashtable_walk_next(&iter)) && !IS_ERR(node))
- fprobe_remove_node_in_module(mod, node, &alist);
+ if (fprobe_remove_node_in_module(mod, node, &alist) < 0) {
+ retry = true;
+ break;
+ }
rhashtable_walk_stop(&iter);
- } while (node == ERR_PTR(-EAGAIN));
+ } while (node == ERR_PTR(-EAGAIN) && !retry);
rhashtable_walk_exit(&iter);
+ /* Remove any ips from hash table(s) */
+ if (alist.index > 0) {
+ fprobe_remove_ips(alist.addrs, alist.index);
+ /*
+ * If we break rhashtable walk loop except for -EAGAIN, we need
+ * to restart looping from start for safety. Anyway, this is
+ * not a hotpath.
+ */
+ if (retry)
+ goto again;
+ }
- if (alist.index > 0)
- fprobe_set_ips(alist.addrs, alist.index, 1, 0);
mutex_unlock(&fprobe_mutex);
kfree(alist.addrs);
^ permalink raw reply related
* [PATCH v6 2/5] tracing/fprobe: Remove fprobe from hash in failure path
From: Masami Hiramatsu (Google) @ 2026-04-14 9:14 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
In-Reply-To: <177615807787.1165997.921227352050738693.stgit@mhiramat.tok.corp.google.com>
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
When register_fprobe_ips() fails, it tries to remove a list of
fprobe_hash_node from fprobe_ip_table, but it missed to remove
fprobe itself from fprobe_table. Moreover, when removing
the fprobe_hash_node which is added to rhltable once, it must
use kfree_rcu() after removing from rhltable.
To fix these issues, this reuses unregister_fprobe() internal
code to rollback the half-way registered fprobe.
Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v6:
- Wait for an RCU grace period before returning error in
unregister_fprobe_nolock().
Changes in v5:
- When rolling back an fprobe that failed to register, the
fprobe_hash_node are forcibly removed and warn if failure.
Changes in v4:
- Remove short-cut case because we always need to upadte ftrace_ops.
- Use guard(mutex) in register_fprobe_ips() to unlock it correctly.
- Remove redundant !ret check in register_fprobe_ips().
- Do not set hlist_array->size in failure case, instead,
hlist_array->array[i].fp is set only when insertion is succeeded.
Changes in v3:
- Newly added.
---
kernel/trace/fprobe.c | 109 ++++++++++++++++++++++++++++---------------------
1 file changed, 63 insertions(+), 46 deletions(-)
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index fc7018b28fdd..6a23bb787295 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -79,20 +79,27 @@ static const struct rhashtable_params fprobe_rht_params = {
};
/* Node insertion and deletion requires the fprobe_mutex */
-static int insert_fprobe_node(struct fprobe_hlist_node *node)
+static int insert_fprobe_node(struct fprobe_hlist_node *node, struct fprobe *fp)
{
+ int ret;
+
lockdep_assert_held(&fprobe_mutex);
- return rhltable_insert(&fprobe_ip_table, &node->hlist, fprobe_rht_params);
+ ret = rhltable_insert(&fprobe_ip_table, &node->hlist, fprobe_rht_params);
+ /* Set the fprobe pointer if insertion was successful. */
+ if (!ret)
+ WRITE_ONCE(node->fp, fp);
+ return ret;
}
/* Return true if there are synonims */
static bool delete_fprobe_node(struct fprobe_hlist_node *node)
{
- lockdep_assert_held(&fprobe_mutex);
bool ret;
- /* Avoid double deleting */
+ lockdep_assert_held(&fprobe_mutex);
+
+ /* Avoid double deleting and non-inserted nodes */
if (READ_ONCE(node->fp) != NULL) {
WRITE_ONCE(node->fp, NULL);
rhltable_remove(&fprobe_ip_table, &node->hlist,
@@ -757,7 +764,6 @@ static int fprobe_init(struct fprobe *fp, unsigned long *addrs, int num)
fp->hlist_array = hlist_array;
hlist_array->fp = fp;
for (i = 0; i < num; i++) {
- hlist_array->array[i].fp = fp;
addr = ftrace_location(addrs[i]);
if (!addr) {
fprobe_fail_cleanup(fp);
@@ -821,6 +827,8 @@ int register_fprobe(struct fprobe *fp, const char *filter, const char *notfilter
}
EXPORT_SYMBOL_GPL(register_fprobe);
+static int unregister_fprobe_nolock(struct fprobe *fp, bool force);
+
/**
* register_fprobe_ips() - Register fprobe to ftrace by address.
* @fp: A fprobe data structure to be registered.
@@ -847,29 +855,26 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
if (ret)
return ret;
- hlist_array = fp->hlist_array;
if (fprobe_is_ftrace(fp))
ret = fprobe_ftrace_add_ips(addrs, num);
else
ret = fprobe_graph_add_ips(addrs, num);
+ if (ret) {
+ fprobe_fail_cleanup(fp);
+ return ret;
+ }
- if (!ret) {
- add_fprobe_hash(fp);
- for (i = 0; i < hlist_array->size; i++) {
- ret = insert_fprobe_node(&hlist_array->array[i]);
- if (ret)
- break;
- }
- /* fallback on insert error */
+ hlist_array = fp->hlist_array;
+ add_fprobe_hash(fp);
+ for (i = 0; i < hlist_array->size; i++) {
+ ret = insert_fprobe_node(&hlist_array->array[i], fp);
if (ret) {
- for (i--; i >= 0; i--)
- delete_fprobe_node(&hlist_array->array[i]);
+ if (unregister_fprobe_nolock(fp, true))
+ pr_warn("Failed to cleanup fprobe after insertion failure.\n");
+ break;
}
}
- if (ret)
- fprobe_fail_cleanup(fp);
-
return ret;
}
EXPORT_SYMBOL_GPL(register_fprobe_ips);
@@ -912,37 +917,29 @@ bool fprobe_is_registered(struct fprobe *fp)
return true;
}
-/**
- * unregister_fprobe() - Unregister fprobe.
- * @fp: A fprobe data structure to be unregistered.
- *
- * Unregister fprobe (and remove ftrace hooks from the function entries).
- *
- * Return 0 if @fp is unregistered successfully, -errno if not.
- */
-int unregister_fprobe(struct fprobe *fp)
+static int unregister_fprobe_nolock(struct fprobe *fp, bool force)
{
- struct fprobe_hlist *hlist_array;
+ struct fprobe_hlist *hlist_array = fp->hlist_array;
unsigned long *addrs = NULL;
- int ret = 0, i, count;
+ int i, count;
- mutex_lock(&fprobe_mutex);
- if (!fp || !fprobe_registered(fp)) {
- ret = -EINVAL;
- goto out;
- }
-
- hlist_array = fp->hlist_array;
addrs = kcalloc(hlist_array->size, sizeof(unsigned long), GFP_KERNEL);
- if (!addrs) {
- ret = -ENOMEM; /* TODO: Fallback to one-by-one loop */
- goto out;
- }
+ if (!addrs && !force)
+ return -ENOMEM;
+ /*
+ * If @force is set, this function will remove fprobe_hash_node
+ * from the hash table even if memory allocation fails. However,
+ * ftrace_ops will not be updated. Anyway, when the last fprobe
+ * is unregistered, ftrace_ops is also unregistered.
+ */
/* Remove non-synonim ips from table and hash */
count = 0;
for (i = 0; i < hlist_array->size; i++) {
- if (!delete_fprobe_node(&hlist_array->array[i]))
+ if (delete_fprobe_node(&hlist_array->array[i]))
+ continue;
+
+ if (addrs)
addrs[count++] = hlist_array->array[i].addr;
}
del_fprobe_hash(fp);
@@ -951,15 +948,35 @@ int unregister_fprobe(struct fprobe *fp)
fprobe_ftrace_remove_ips(addrs, count);
else
fprobe_graph_remove_ips(addrs, count);
+ /*
+ * If count == 0, instead of calling ftrace_set_filter_ips(),
+ * we must wait for RCU grace period to finish del_fprobe_hash().
+ */
+ if (!count)
+ synchronize_rcu();
kfree_rcu(hlist_array, rcu);
fp->hlist_array = NULL;
+ kfree(addrs);
-out:
- mutex_unlock(&fprobe_mutex);
+ return !addrs ? -ENOMEM : 0;
+}
- kfree(addrs);
- return ret;
+/**
+ * unregister_fprobe() - Unregister fprobe.
+ * @fp: A fprobe data structure to be unregistered.
+ *
+ * Unregister fprobe (and remove ftrace hooks from the function entries).
+ *
+ * Return 0 if @fp is unregistered successfully, -errno if not.
+ */
+int unregister_fprobe(struct fprobe *fp)
+{
+ guard(mutex)(&fprobe_mutex);
+ if (!fp || !fprobe_registered(fp))
+ return -EINVAL;
+
+ return unregister_fprobe_nolock(fp, false);
}
EXPORT_SYMBOL_GPL(unregister_fprobe);
^ permalink raw reply related
* [PATCH v6 1/5] tracing/fprobe: Reject registration of a registered fprobe before init
From: Masami Hiramatsu (Google) @ 2026-04-14 9:14 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
In-Reply-To: <177615807787.1165997.921227352050738693.stgit@mhiramat.tok.corp.google.com>
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Reject registration of a registered fprobe which is on the fprobe
hash table before initializing fprobe.
The add_fprobe_hash() checks this re-register fprobe, but since
fprobe_init() clears hlist_array field, it is too late to check it.
It has to check the re-registration before touncing fprobe.
Fixes: 4346ba160409 ("fprobe: Rewrite fprobe on function-graph tracer")
Cc: stable@vger.kernel.org
Signed-off-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
Changes in v6:
- Newly added.
---
kernel/trace/fprobe.c | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
index dcadf1d23b8a..fc7018b28fdd 100644
--- a/kernel/trace/fprobe.c
+++ b/kernel/trace/fprobe.c
@@ -4,6 +4,7 @@
*/
#define pr_fmt(fmt) "fprobe: " fmt
+#include <linux/cleanup.h>
#include <linux/err.h>
#include <linux/fprobe.h>
#include <linux/kallsyms.h>
@@ -107,7 +108,7 @@ static bool delete_fprobe_node(struct fprobe_hlist_node *node)
}
/* Check existence of the fprobe */
-static bool is_fprobe_still_exist(struct fprobe *fp)
+static bool fprobe_registered(struct fprobe *fp)
{
struct hlist_head *head;
struct fprobe_hlist *fph;
@@ -120,7 +121,7 @@ static bool is_fprobe_still_exist(struct fprobe *fp)
}
return false;
}
-NOKPROBE_SYMBOL(is_fprobe_still_exist);
+NOKPROBE_SYMBOL(fprobe_registered);
static int add_fprobe_hash(struct fprobe *fp)
{
@@ -132,9 +133,6 @@ static int add_fprobe_hash(struct fprobe *fp)
if (WARN_ON_ONCE(!fph))
return -EINVAL;
- if (is_fprobe_still_exist(fp))
- return -EEXIST;
-
head = &fprobe_table[hash_ptr(fp, FPROBE_HASH_BITS)];
hlist_add_head_rcu(&fp->hlist_array->hlist, head);
return 0;
@@ -149,7 +147,7 @@ static int del_fprobe_hash(struct fprobe *fp)
if (WARN_ON_ONCE(!fph))
return -EINVAL;
- if (!is_fprobe_still_exist(fp))
+ if (!fprobe_registered(fp))
return -ENOENT;
fph->fp = NULL;
@@ -482,7 +480,7 @@ static void fprobe_return(struct ftrace_graph_ret *trace,
if (!fp)
break;
curr += FPROBE_HEADER_SIZE_IN_LONG;
- if (is_fprobe_still_exist(fp) && !fprobe_disabled(fp)) {
+ if (fprobe_registered(fp) && !fprobe_disabled(fp)) {
if (WARN_ON_ONCE(curr + size > size_words))
break;
fp->exit_handler(fp, trace->func, ret_ip, fregs,
@@ -841,12 +839,14 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
struct fprobe_hlist *hlist_array;
int ret, i;
+ guard(mutex)(&fprobe_mutex);
+ if (fprobe_registered(fp))
+ return -EEXIST;
+
ret = fprobe_init(fp, addrs, num);
if (ret)
return ret;
- mutex_lock(&fprobe_mutex);
-
hlist_array = fp->hlist_array;
if (fprobe_is_ftrace(fp))
ret = fprobe_ftrace_add_ips(addrs, num);
@@ -866,7 +866,6 @@ int register_fprobe_ips(struct fprobe *fp, unsigned long *addrs, int num)
delete_fprobe_node(&hlist_array->array[i]);
}
}
- mutex_unlock(&fprobe_mutex);
if (ret)
fprobe_fail_cleanup(fp);
@@ -928,7 +927,7 @@ int unregister_fprobe(struct fprobe *fp)
int ret = 0, i, count;
mutex_lock(&fprobe_mutex);
- if (!fp || !is_fprobe_still_exist(fp)) {
+ if (!fp || !fprobe_registered(fp)) {
ret = -EINVAL;
goto out;
}
^ permalink raw reply related
* [PATCH v6 0/5] tracing/fprobe: Fix fprobe_ip_table related bugs
From: Masami Hiramatsu (Google) @ 2026-04-14 9:14 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu
Cc: Menglong Dong, Mathieu Desnoyers, jiang.biao, linux-kernel,
linux-trace-kernel
Hi,
Here is the 6th version of fprobe bugfix series.
The previous version is here.
https://lore.kernel.org/all/177606956628.929411.17392736689322577701.stgit@devnote2/
In this version, I added yet another bugfixes for rejecting registered
fprobe correctly [1/5], and unregistering ftrace_ops if it is empty on
module unloading [5/5]. Also, add RCU synchronization for failure path
[2/5] and retry rhashtable walking outside of rhltable_walk_enter/exit
[3/5].
The [5/5] is to handle the module unloading problem I reported
previously [1]. This was handled solely within fprobe to avoid
impacting the live patch.
[1] https://lore.kernel.org/all/20260408110237.08d982fab08356bc8d48af45@kernel.org/
Thank you!
Masami Hiramatsu (Google) (5):
tracing/fprobe: Reject registration of a registered fprobe before init
tracing/fprobe: Remove fprobe from hash in failure path
tracing/fprobe: Avoid kcalloc() in rcu_read_lock section
tracing/fprobe: Check the same type fprobe on table as the unregistered one
tracing/fprobe: Fix to unregister ftrace_ops if it is empty on module unloading
kernel/trace/fprobe.c | 462 +++++++++++++++++++++++++++++++++----------------
1 file changed, 308 insertions(+), 154 deletions(-)
base-commit: 4346be6577aaa04586167402ae87bbdbe32484a4
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH 0/3] mm: split the file's i_mmap tree for NUMA
From: Huang Shijie @ 2026-04-14 9:11 UTC (permalink / raw)
To: Mateusz Guzik
Cc: akpm, viro, brauner, linux-mm, linux-kernel, linux-arm-kernel,
linux-fsdevel, muchun.song, osalvador, linux-trace-kernel,
linux-perf-users, linux-parisc, nvdimm, zhongyuan, fangbaoshun,
yingzhiwei
In-Reply-To: <76pfiwabdgsej6q2yxfh3efuqvsyg7mt7rvl5itzzjyhdrto5r@53viaxsackzv>
On Mon, Apr 13, 2026 at 05:33:21PM +0200, Mateusz Guzik wrote:
> On Mon, Apr 13, 2026 at 02:20:39PM +0800, Huang Shijie wrote:
> > In NUMA, there are maybe many NUMA nodes and many CPUs.
> > For example, a Hygon's server has 12 NUMA nodes, and 384 CPUs.
> > In the UnixBench tests, there is a test "execl" which tests
> > the execve system call.
> >
> > When we test our server with "./Run -c 384 execl",
> > the test result is not good enough. The i_mmap locks contended heavily on
> > "libc.so" and "ld.so". For example, the i_mmap tree for "libc.so" can have
> > over 6000 VMAs, all the VMAs can be in different NUMA mode.
> > The insert/remove operations do not run quickly enough.
> >
> > patch 1 & patch 2 are try to hide the direct access of i_mmap.
> > patch 3 splits the i_mmap into sibling trees, and we can get better
> > performance with this patch set:
> > we can get 77% performance improvement(10 times average)
> >
>
> To my reading you kept the lock as-is and only distributed the protected
> state.
>
> While I don't doubt the improvement, I'm confident should you take a
> look at the profile you are going to find this still does not scale with
> rwsem being one of the problems (there are other global locks, some of
> which have experimental patches for).
IMHO, when the number of VMAs in the i_mmap is very large, only optimise the rwsem
lock does not help too much for our NUMA case.
In our NUMA server, the remote access could be the major issue.
>
> Apart from that this does nothing to help high core systems which are
> all one node, which imo puts another question mark on this specific
> proposal.
Yes, this patch set only focus on the NUMA case.
The one-node case should use the original i_mmap.
Maybe I can add a new config, CONFIG_SPILT_I_MMAP. The config is disabled
by default, and enabled when the NUMA node is not one.
>
> Of course one may question whether a RB tree is the right choice here,
> it may be the lock-protected cost can go way down with merely a better
> data structure.
>
> Regardless of that, for actual scalability, there will be no way around
> decentralazing locking around this and partitioning per some core count
> (not just by numa awareness).
>
> Decentralizing locking is definitely possible, but I have not looked
> into specifics of how problematic it is. Best case scenario it will
> merely with separate locks. Worst case scenario something needs a fully
> stabilized state for traversal, in that case another rw lock can be
Yes.
The traversal may need to hold many locks.
> slapped around this, creating locking order read lock -> per-subset
> write lock -- this will suffer scalability due to the read locking, but
> it will still scale drastically better as apart from that there will be
> no serialization. In this setting the problematic consumer will write
> lock the new thing to stabilize the state.
>
> So my non-maintainer opinion is that the patchset is not worth it as it
> fails to address anything for significantly more common and already
> affected setups.
This patch set is to reduce the remote access latency for insert/remove VMA
in NUMA.
>
> Have you looked into splitting the lock?
>
I ever tried.
But there are two disadvantages:
1.) The traversal may need to hold many locks which makes the
code very horrible.
2.) Even we split the locks. Each lock protects a tree, when the tree becomes
big enough, the VMA insert/remove will also become slow in NUMA.
The reason is that the tree has VMAs in different NUMA nodes.
Thanks
Huang Shijie
^ permalink raw reply
* Re: [PATCH v2] tracing/hist: bound expression strings with seq_buf
From: Steven Rostedt @ 2026-04-14 9:10 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Masami Hiramatsu, Mathieu Desnoyers, linux-trace-kernel,
linux-kernel
In-Reply-To: <20260409123001.1-tracing-hist-expr-v2-pengpeng@iscas.ac.cn>
On Thu, 9 Apr 2026 10:56:28 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:
Hi Pengpeng,
Again, subject should be:
tracing: Bound histogram expression strings with seq_buf
> expr_str() allocates a fixed MAX_FILTER_STR_VAL buffer and then builds
> expression names with a series of raw strcat() appends. Nested operands,
> constants and field flags can push the rendered string past that fixed
> limit before the name is attached to the hist field.
>
> Build the expression strings with seq_buf and propagate failures back to
> the expression parser when the rendered name would exceed
> MAX_FILTER_STR_VAL.
>
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1:
> - replace the previous bounded append helper and manual length tracking
> with seq_buf as suggested by Steven Rostedt
> - keep the -E2BIG propagation back into parse_unary() and parse_expr()
>
> kernel/trace/trace_events_hist.c | 93 ++++++++++++++++++++++----------
> 1 file changed, 64 insertions(+), 29 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..09aaedb92993 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -8,6 +8,7 @@
> #include <linux/module.h>
> #include <linux/kallsyms.h>
> #include <linux/security.h>
> +#include <linux/seq_buf.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> #include <linux/stacktrace.h>
> @@ -1738,85 +1739,104 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
> return flags_str;
> }
>
> -static void expr_field_str(struct hist_field *field, char *expr)
> +static bool expr_field_str(struct hist_field *field, struct seq_buf *s)
> {
> + const char *field_name;
> +
> if (field->flags & HIST_FIELD_FL_VAR_REF)
> - strcat(expr, "$");
> - else if (field->flags & HIST_FIELD_FL_CONST) {
> - char str[HIST_CONST_DIGITS_MAX];
> + seq_buf_putc(s, '$');
> + else if (field->flags & HIST_FIELD_FL_CONST)
> + seq_buf_printf(s, "%llu", field->constant);
>
> - snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
> - strcat(expr, str);
> - }
> + field_name = hist_field_name(field, 0);
> + if (!field_name)
> + return false;
>
> - strcat(expr, hist_field_name(field, 0));
> + seq_buf_puts(s, field_name);
>
> if (field->flags && !(field->flags & HIST_FIELD_FL_VAR_REF)) {
> const char *flags_str = get_hist_field_flags(field);
>
> - if (flags_str) {
> - strcat(expr, ".");
> - strcat(expr, flags_str);
> - }
> + if (flags_str)
> + seq_buf_printf(s, ".%s", flags_str);
> }
> +
> + return !seq_buf_has_overflowed(s);
> }
>
> static char *expr_str(struct hist_field *field, unsigned int level)
> {
> char *expr;
> + struct seq_buf s;
> + int ret = -E2BIG;
>
> if (level > 1)
> - return NULL;
> + return ERR_PTR(-EINVAL);
>
> expr = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
> if (!expr)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
A patch should do one thing at a time. This patch appears to be doing
two things: fixing the bound strings, returning errors instead of NULL.
Please split this up into two patches. One that changes the return
values from NULL to ERR_PTR() and the other to use seq_buf.
Thanks,
-- Steve
^ permalink raw reply
* Re: [PATCH v3] tracing/hist: bound synthetic-field strings with seq_buf
From: Steven Rostedt @ 2026-04-14 8:58 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Masami Hiramatsu, Tom Zanussi, Mathieu Desnoyers,
linux-trace-kernel, linux-kernel
In-Reply-To: <20260409103001.1-tracing-hist-synth-v3-pengpeng@iscas.ac.cn>
On Thu, 9 Apr 2026 10:19:43 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:
Hi Pengpeng,
Note, the tracing subsystem uses capital letters in the subject:
Subject: tracing: Bound synthetic-field strings with seq_buf
> The synthetic field helpers build a prefixed synthetic variable name and
> a generated hist command in fixed MAX_FILTER_STR_VAL buffers. The
> current code appends those strings with raw strcat(), so long key lists,
> field names, or saved filters can run past the end of the staging
> buffers.
>
> Build both strings with seq_buf and propagate -E2BIG if either the
> synthetic variable name or the generated command exceeds
> MAX_FILTER_STR_VAL. This keeps the existing tracing-side limit while
> using the helper intended for bounded command construction.
>
> Fixes: 02205a6752f2 ("tracing: Add support for 'field variables'")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v2: https://lore.kernel.org/all/20260401112224.85582-2-pengpeng@iscas.ac.cn/
>
> - switch the synthetic name and generated command construction to seq_buf
> as suggested by Steven Rostedt
> - keep MAX_FILTER_STR_VAL as the tracing-side limit and return -E2BIG on
> overflow
>
> kernel/trace/trace_events_hist.c | 44 ++++++++++++++++++++++----------
> 1 file changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..7c3873719beb 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -8,6 +8,7 @@
> #include <linux/module.h>
> #include <linux/kallsyms.h>
> #include <linux/security.h>
> +#include <linux/seq_buf.h>
> #include <linux/mutex.h>
> #include <linux/slab.h>
> #include <linux/stacktrace.h>
> @@ -2962,14 +2963,21 @@ find_synthetic_field_var(struct hist_trigger_data *target_hist_data,
> char *system, char *event_name, char *field_name)
> {
> struct hist_field *event_var;
> + struct seq_buf s;
> char *synthetic_name;
>
> synthetic_name = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
> if (!synthetic_name)
> return ERR_PTR(-ENOMEM);
>
> - strcpy(synthetic_name, "synthetic_");
> - strcat(synthetic_name, field_name);
> + seq_buf_init(&s, synthetic_name, MAX_FILTER_STR_VAL);
> + seq_buf_puts(&s, "synthetic_");
> + seq_buf_puts(&s, field_name);
Should have a comment here specifying what the seq_buf_str() is doing:
/* Terminate synthetic_name with a nul */
> + seq_buf_str(&s);
> + if (seq_buf_has_overflowed(&s)) {
> + kfree(synthetic_name);
> + return ERR_PTR(-E2BIG);
> + }
>
> event_var = find_event_var(target_hist_data, system, event_name, synthetic_name);
>
> @@ -3014,6 +3022,7 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
> struct trace_event_file *file;
> struct hist_field *key_field;
> struct hist_field *event_var;
> + struct seq_buf s;
> char *saved_filter;
> char *cmd;
> int ret;
> @@ -3046,41 +3055,48 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
> /* See if a synthetic field variable has already been created */
> event_var = find_synthetic_field_var(target_hist_data, subsys_name,
> event_name, field_name);
> - if (!IS_ERR_OR_NULL(event_var))
> + if (IS_ERR(event_var))
> + return event_var;
> + if (event_var)
> return event_var;
Note, the above is equivalent to:
if (event_var)
return event_var;
And since it is a separate issue than the bounding of the string, it
should be a separate patch.
>
> var_hist = kzalloc_obj(*var_hist);
> if (!var_hist)
> return ERR_PTR(-ENOMEM);
>
> + saved_filter = find_trigger_filter(hist_data, file);
Why did you move this up here?
> +
> cmd = kzalloc(MAX_FILTER_STR_VAL, GFP_KERNEL);
> if (!cmd) {
> kfree(var_hist);
> return ERR_PTR(-ENOMEM);
> }
>
> + seq_buf_init(&s, cmd, MAX_FILTER_STR_VAL);
> +
> /* Use the same keys as the compatible histogram */
> - strcat(cmd, "keys=");
> + seq_buf_puts(&s, "keys=");
>
> for_each_hist_key_field(i, hist_data) {
> key_field = hist_data->fields[i];
> if (!first)
> - strcat(cmd, ",");
> - strcat(cmd, key_field->field->name);
> + seq_buf_putc(&s, ',');
> + seq_buf_puts(&s, key_field->field->name);
> first = false;
> }
>
> /* Create the synthetic field variable specification */
> - strcat(cmd, ":synthetic_");
> - strcat(cmd, field_name);
> - strcat(cmd, "=");
> - strcat(cmd, field_name);
> + seq_buf_printf(&s, ":synthetic_%s=%s", field_name, field_name);
>
> /* Use the same filter as the compatible histogram */
> - saved_filter = find_trigger_filter(hist_data, file);
It makes more sense to define saved_filter next to where it is used.
> - if (saved_filter) {
> - strcat(cmd, " if ");
> - strcat(cmd, saved_filter);
> + if (saved_filter)
> + seq_buf_printf(&s, " if %s", saved_filter);
> +
> + seq_buf_str(&s);
> + if (seq_buf_has_overflowed(&s)) {
> + kfree(cmd);
> + kfree(var_hist);
> + return ERR_PTR(-E2BIG);
> }
>
> var_hist->cmd = kstrdup(cmd, GFP_KERNEL);
-- Steve
^ permalink raw reply
* Re: [PATCH v5 0/3] tracing/fprobe: Fix fprobe_ip_table related bugs
From: Masami Hiramatsu @ 2026-04-14 1:19 UTC (permalink / raw)
To: Masami Hiramatsu (Google)
Cc: Steven Rostedt, Menglong Dong, Mathieu Desnoyers, jiang.biao,
linux-kernel, linux-trace-kernel
In-Reply-To: <177606956628.929411.17392736689322577701.stgit@devnote2>
On Mon, 13 Apr 2026 17:39:26 +0900
"Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> Here is the 5th series of patches to fix bugs in fprobe.
> The previous version is here.
>
> https://lore.kernel.org/all/177584108931.388483.11311214679686745474.stgit@devnote2/
>
> This version fixes to remove fprobe_hash_node forcibly when fprobe
> registration failed [1/3] and skips updating ftrace_ops when fails
> to allocate memory in module unloading [2/3].
Hmm, Sashiko pointed out some issues in fprobe, which seems not introduced
this series but existing UAF cases.
https://sashiko.dev/#/patchset/177606956628.929411.17392736689322577701.stgit%40devnote2
Especially,
> In fprobe_return(), the code traverses the fprobe_table which contains
> RCU-protected struct fprobe_hlist nodes. These nodes are freed using
> kfree_rcu(hlist_array, rcu) in unregister_fprobe_nolock().
>
> To safely traverse this RCU-protected list, readers must hold the RCU read
> lock. However, fprobe_return() only calls preempt_disable_notrace(). While
> disabling preemption acts as an RCU-sched read-side critical section on
> non-RT kernels, it does not prevent regular RCU grace periods from
> completing on PREEMPT_RT. Thus, kfree_rcu() can free the hlist_array while
> fprobe_return() is actively iterating over it.
I would like to ask Steve a comment about this. Is fgraph return handler
context RCU safe?
Thanks,
>
> Thanks,
> ---
>
> Masami Hiramatsu (Google) (3):
> tracing/fprobe: Remove fprobe from hash in failure path
> tracing/fprobe: Avoid kcalloc() in rcu_read_lock section
> tracing/fprobe: Check the same type fprobe on table as the unregistered one
>
>
> kernel/trace/fprobe.c | 251 +++++++++++++++++++++++++++++--------------------
> 1 file changed, 147 insertions(+), 104 deletions(-)
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>
--
Masami Hiramatsu (Google) <mhiramat@kernel.org>
^ permalink raw reply
* Re: [PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call
From: Tom Zanussi @ 2026-04-13 22:38 UTC (permalink / raw)
To: Pengpeng Hou, rostedt
Cc: mhiramat, mathieu.desnoyers, tom.zanussi, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260401112224.85582-1-pengpeng@iscas.ac.cn>
On Wed, 2026-04-01 at 19:22 +0800, Pengpeng Hou wrote:
> hist_field_name() uses a static MAX_FILTER_STR_VAL buffer for fully
> qualified variable-reference names, but it currently appends into that
> buffer with strcat() without rebuilding it first. As a result, repeated
> calls append a new "system.event.field" name onto the previous one,
> which can eventually run past the end of full_name.
>
> Build the name with snprintf() on each call and return NULL if the fully
> qualified name does not fit in MAX_FILTER_STR_VAL.
>
> Fixes: 067fe038e70f ("tracing: Add variable reference handling to hist triggers")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
Looks good to me, thanks.
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
Tested-by: Tom Zanussi <zanussi@kernel.org>
> ---
> Changes since v1: https://lore.kernel.org/all/20260329030950.32503-1-pengpeng@iscas.ac.cn/
>
> - rebuild full_name on each call instead of falling back to field->name
> - return NULL on overflow as suggested
> - split out the snprintf() length check instead of using an inline if
>
> kernel/trace/trace_events_hist.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 73ea180cad55..f9c8a4f078ea 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -1361,12 +1361,14 @@ static const char *hist_field_name(struct hist_field *field,
> field->flags & HIST_FIELD_FL_VAR_REF) {
> if (field->system) {
> static char full_name[MAX_FILTER_STR_VAL];
> + int len;
> +
> + len = snprintf(full_name, sizeof(full_name), "%s.%s.%s",
> + field->system, field->event_name,
> + field->name);
> + if (len >= sizeof(full_name))
> + return NULL;
>
> - strcat(full_name, field->system);
> - strcat(full_name, ".");
> - strcat(full_name, field->event_name);
> - strcat(full_name, ".");
> - strcat(full_name, field->name);
> field_name = full_name;
> } else
> field_name = field->name;
^ permalink raw reply
* [PATCH 2/2] selftests/ftrace: Add test case for fully-qualified variable references
From: Tom Zanussi @ 2026-04-13 22:35 UTC (permalink / raw)
To: rostedt
Cc: pengpeng, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel
In-Reply-To: <cover.1776112478.git.zanussi@kernel.org>
This test adds a variable (ts0) to two events (sched_waking and
sched_wakeup) and uses a fully-qualified variable reference to
expicitly choose a particular one (sched_wakeup.$ts0) when calculating
the wakeup latency.
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
.../trigger-fully-qualified-var-ref.tc | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-fully-qualified-var-ref.tc
diff --git a/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-fully-qualified-var-ref.tc b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-fully-qualified-var-ref.tc
new file mode 100644
index 000000000000..8d12cdd06f1d
--- /dev/null
+++ b/tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-fully-qualified-var-ref.tc
@@ -0,0 +1,34 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# description: event trigger - test fully-qualified variable reference support
+# requires: set_event synthetic_events events/sched/sched_process_fork/hist ping:program
+
+fail() { #msg
+ echo $1
+ exit_fail
+}
+
+echo "Test fully-qualified variable reference support"
+
+echo 'wakeup_latency u64 lat; pid_t pid; int prio; char comm[16]' > synthetic_events
+echo 'hist:keys=comm:ts0=common_timestamp.usecs if comm=="ping"' > events/sched/sched_waking/trigger
+echo 'hist:keys=comm:ts0=common_timestamp.usecs if comm=="ping"' > events/sched/sched_wakeup/trigger
+echo 'hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-sched.sched_wakeup.$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' > events/sched/sched_switch/trigger
+echo 'hist:keys=pid,prio,comm:vals=lat:sort=pid,prio' > events/synthetic/wakeup_latency/trigger
+
+ping $LOCALHOST -c 3
+if ! grep -q "ping" events/synthetic/wakeup_latency/hist; then
+ fail "Failed to create inter-event histogram"
+fi
+
+if ! grep -q "synthetic_prio=prio" events/sched/sched_waking/hist; then
+ fail "Failed to create histogram with fully-qualified variable reference"
+fi
+
+echo '!hist:keys=next_comm:wakeup_lat=common_timestamp.usecs-sched.sched_wakeup.$ts0:onmatch(sched.sched_waking).wakeup_latency($wakeup_lat,next_pid,sched.sched_waking.prio,next_comm) if next_comm=="ping"' >> events/sched/sched_switch/trigger
+
+if grep -q "synthetic_prio=prio" events/sched/sched_waking/hist; then
+ fail "Failed to remove histogram with fully-qualified variable reference"
+fi
+
+exit 0
--
2.43.0
^ permalink raw reply related
* [PATCH 1/2] tracing: Fix fully-qualified variable reference printing in histograms
From: Tom Zanussi @ 2026-04-13 22:35 UTC (permalink / raw)
To: rostedt
Cc: pengpeng, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel
In-Reply-To: <cover.1776112478.git.zanussi@kernel.org>
The syntax for fully-qualified variable references in histograms is
subsys.event.$var, which is parsed correctly, but not displayed
correctly when printing a histogram spec. The current code puts the $
reference at the beginning of the fully-qualified variable name
i.e. $subsys.event.var, which is incorrect.
Before:
trigger info: hist:keys=next_comm:vals=hitcount:wakeup_lat=common_timestamp.usecs-$sched.sched_wakeup.ts0: ...
After:
trigger info: hist:keys=next_comm:vals=hitcount:wakeup_lat=common_timestamp.usecs-sched.sched_wakeup.$ts0: ...
Signed-off-by: Tom Zanussi <zanussi@kernel.org>
---
kernel/trace/trace_events_hist.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
index b2b675c7d663..0dbbf6cca9bc 100644
--- a/kernel/trace/trace_events_hist.c
+++ b/kernel/trace/trace_events_hist.c
@@ -1361,9 +1361,12 @@ static const char *hist_field_name(struct hist_field *field,
field->flags & HIST_FIELD_FL_VAR_REF) {
if (field->system) {
static char full_name[MAX_FILTER_STR_VAL];
+ static char *fmt;
int len;
- len = snprintf(full_name, sizeof(full_name), "%s.%s.%s",
+ fmt = field->flags & HIST_FIELD_FL_VAR_REF ? "%s.%s.$%s" : "%s.%s.%s";
+
+ len = snprintf(full_name, sizeof(full_name), fmt,
field->system, field->event_name,
field->name);
if (len >= sizeof(full_name))
@@ -1742,9 +1745,10 @@ static const char *get_hist_field_flags(struct hist_field *hist_field)
static void expr_field_str(struct hist_field *field, char *expr)
{
- if (field->flags & HIST_FIELD_FL_VAR_REF)
- strcat(expr, "$");
- else if (field->flags & HIST_FIELD_FL_CONST) {
+ if (field->flags & HIST_FIELD_FL_VAR_REF) {
+ if (!field->system)
+ strcat(expr, "$");
+ } else if (field->flags & HIST_FIELD_FL_CONST) {
char str[HIST_CONST_DIGITS_MAX];
snprintf(str, HIST_CONST_DIGITS_MAX, "%llu", field->constant);
@@ -6156,7 +6160,8 @@ static void hist_field_print(struct seq_file *m, struct hist_field *hist_field)
else if (field_name) {
if (hist_field->flags & HIST_FIELD_FL_VAR_REF ||
hist_field->flags & HIST_FIELD_FL_ALIAS)
- seq_putc(m, '$');
+ if (!hist_field->system)
+ seq_putc(m, '$');
seq_printf(m, "%s", field_name);
} else if (hist_field->flags & HIST_FIELD_FL_TIMESTAMP)
seq_puts(m, "common_timestamp");
--
2.43.0
^ permalink raw reply related
* [PATCH 0/2] tracing: fully-qualified var-ref testcase
From: Tom Zanussi @ 2026-04-13 22:35 UTC (permalink / raw)
To: rostedt
Cc: pengpeng, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel
Hi Steve,
Here's the testcase for fully-qualified var references mentioned here
[1].
While working on it, I realized that the printing of the
fully-qualified references was wrong (because the testcases use that
output to remove the trigger), so added the first patch.
It depends on Pengpeng Hou's patch:
[PATCH v2 1/2] tracing/hist: rebuild full_name on each hist_field_name() call
Thanks,
Tom
[1] https://lore.kernel.org/lkml/36cf0fc5ee3a4476b0a70536d212278a9ee4d380.camel@kernel.org/
Tom Zanussi (2):
tracing: Fix fully-qualified variable reference printing in histograms
selftests/ftrace: Add test case for fully-qualified variable
references
kernel/trace/trace_events_hist.c | 15 +++++---
.../trigger-fully-qualified-var-ref.tc | 34 +++++++++++++++++++
2 files changed, 44 insertions(+), 5 deletions(-)
create mode 100644 tools/testing/selftests/ftrace/test.d/trigger/inter-event/trigger-fully-qualified-var-ref.tc
--
2.43.0
^ permalink raw reply
* Re: [PATCH v3 09/11] dt-bindings: input: Document hid-over-spi DT schema
From: Rob Herring @ 2026-04-13 22:34 UTC (permalink / raw)
To: Conor Dooley, Dmitry Torokhov, Jingyuan Liang
Cc: Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Mark Brown,
Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Krzysztof Kozlowski, Conor Dooley, linux-input, linux-doc,
linux-kernel, linux-spi, linux-trace-kernel, devicetree, hbarnor,
tfiga, Dmitry Antipov, Jarrett Schultz
In-Reply-To: <20260410-sake-dollop-9f253ddb0749@spud>
On Fri, Apr 10, 2026 at 06:35:00PM +0100, Conor Dooley wrote:
> On Thu, Apr 09, 2026 at 10:16:46AM -0700, Dmitry Torokhov wrote:
> > On Thu, Apr 09, 2026 at 05:02:11PM +0100, Conor Dooley wrote:
> > > On Thu, Apr 02, 2026 at 01:59:46AM +0000, Jingyuan Liang wrote:
> > > > Documentation describes the required and optional properties for
> > > > implementing Device Tree for a Microsoft G6 Touch Digitizer that
> > > > supports HID over SPI Protocol 1.0 specification.
> > > >
> > > > The properties are common to HID over SPI.
> > > >
> > > > Signed-off-by: Dmitry Antipov <dmanti@microsoft.com>
> > > > Signed-off-by: Jarrett Schultz <jaschultz@microsoft.com>
> > > > Signed-off-by: Jingyuan Liang <jingyliang@chromium.org>
> > > > ---
> > > > .../devicetree/bindings/input/hid-over-spi.yaml | 126 +++++++++++++++++++++
> > > > 1 file changed, 126 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> > > > new file mode 100644
> > > > index 000000000000..d1b0a2e26c32
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> > > > @@ -0,0 +1,126 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/input/hid-over-spi.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: HID over SPI Devices
> > > > +
> > > > +maintainers:
> > > > + - Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > + - Jiri Kosina <jkosina@suse.cz>
> > >
> > > Why them and not you, the developers of the series?
> > >
> > > > +
> > > > +description: |+
> > > > + HID over SPI provides support for various Human Interface Devices over the
> > > > + SPI bus. These devices can be for example touchpads, keyboards, touch screens
> > > > + or sensors.
> > > > +
> > > > + The specification has been written by Microsoft and is currently available
> > > > + here: https://www.microsoft.com/en-us/download/details.aspx?id=103325
> > > > +
> > > > + If this binding is used, the kernel module spi-hid will handle the
> > > > + communication with the device and the generic hid core layer will handle the
> > > > + protocol.
> > >
> > > This is not relevant to the binding, please remove it.
> > >
> > > > +
> > > > +allOf:
> > > > + - $ref: /schemas/input/touchscreen/touchscreen.yaml#
> > > > +
> > > > +properties:
> > > > + compatible:
> > > > + oneOf:
> > > > + - items:
> > > > + - enum:
> > > > + - microsoft,g6-touch-digitizer
> > > > + - const: hid-over-spi
> > > > + - description: Just "hid-over-spi" alone is allowed, but not recommended.
> > > > + const: hid-over-spi
> > >
> > > Why is it allowed but not recommended? Seems to me like we should
> > > require device-specific compatibles.
> >
> > Why would we want to change the driver code to add a new compatible each
> > time a vendor decides to create a chip that is fully hid-spi-protocol
> > compliant? Or is the plan to still allow "hid-over-spi" fallback but
> > require device-specific compatible that will be ignored unless there is
> > device-specific quirk needed?
The plan is the latter case (the 1st entry up above). The comment is
remove the 2nd entry (with 'Just "hid-over-spi" alone is allowed, but
not recommended.').
> This has nothing to do with the driver, just the oddity of having a
> comment saying that not having a device specific compatible was
> permitted by not recommended in a binding. Requiring device-specific
> compatibles is the norm after all and a comment like this makes draws
> more attention to the fact that this is abnormal. Regardless of what the
> driver does, device-specific compatibles should be required.
>
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > +
> > > > + interrupts:
> > > > + maxItems: 1
> > > > +
> > > > + reset-gpios:
> > > > + maxItems: 1
> > > > + description:
> > > > + GPIO specifier for the digitizer's reset pin (active low). The line must
> > > > + be flagged with GPIO_ACTIVE_LOW.
> > > > +
> > > > + vdd-supply:
> > > > + description:
> > > > + Regulator for the VDD supply voltage.
> > > > +
> > > > + input-report-header-address:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + minimum: 0
> > > > + maximum: 0xffffff
> > > > + description:
> > > > + A value to be included in the Read Approval packet, listing an address of
> > > > + the input report header to be put on the SPI bus. This address has 24
> > > > + bits.
> > > > +
> > > > + input-report-body-address:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + minimum: 0
> > > > + maximum: 0xffffff
> > > > + description:
> > > > + A value to be included in the Read Approval packet, listing an address of
> > > > + the input report body to be put on the SPI bus. This address has 24 bits.
> > > > +
> > > > + output-report-address:
> > > > + $ref: /schemas/types.yaml#/definitions/uint32
> > > > + minimum: 0
> > > > + maximum: 0xffffff
> > > > + description:
> > > > + A value to be included in the Output Report sent by the host, listing an
> > > > + address where the output report on the SPI bus is to be written to. This
> > > > + address has 24 bits.
> > > > +
> > > > + read-opcode:
> > > > + $ref: /schemas/types.yaml#/definitions/uint8
> > > > + description:
> > > > + Value to be used in Read Approval packets. 1 byte.
> > > > +
> > > > + write-opcode:
> > > > + $ref: /schemas/types.yaml#/definitions/uint8
> > > > + description:
> > > > + Value to be used in Write Approval packets. 1 byte.
> > >
> > > Why can none of these things be determined from the device's compatible?
> > > On the surface, they like the kinds of things that could/should be.
> >
> > Why would we want to keep tables of these values in the kernel and again
> > have to update the driver for each new chip?
>
> That's pretty normal though innit? It's what match data does.
> If someone wants to have properties that communicate data that
> can be determined from the compatible, they need to provide
> justification why it is being done.
IIRC, it was explained in prior versions the spec itself says these
values vary by device. If we expect variation, then I think these
properties are fine. But please capture the reasoning for them in this
patch or we will just keep asking the same questions over and over.
Rob
^ permalink raw reply
* [PATCH] tracepoint: balance regfunc() on func_add() failure in tracepoint_add_func()
From: David Carlier @ 2026-04-13 19:06 UTC (permalink / raw)
To: Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers
Cc: linux-trace-kernel, linux-kernel, David Carlier, stable
When a tracepoint goes through the 0 -> 1 transition, tracepoint_add_func()
invokes the subsystem's ext->regfunc() before attempting to install the
new probe via func_add(). If func_add() then fails (for example, when
allocate_probes() cannot allocate a new probe array under memory pressure
and returns -ENOMEM), the function returns the error without calling the
matching ext->unregfunc(), leaving the side effects of regfunc() behind
with no installed probe to justify them.
For syscall tracepoints this is particularly unpleasant: syscall_regfunc()
bumps sys_tracepoint_refcount and sets SYSCALL_TRACEPOINT on every task.
After a leaked failure, the refcount is stuck at a non-zero value with no
consumer, and every task continues paying the syscall trace entry/exit
overhead until reboot. Other subsystems providing regfunc()/unregfunc()
pairs exhibit similarly scoped persistent state.
Mirror the existing 1 -> 0 cleanup and call ext->unregfunc() in the
func_add() error path, gated on the same condition used there so the
unwind is symmetric with the registration.
Fixes: 8cf868affdc4 ("tracing: Have the reg function allow to fail")
Cc: stable@vger.kernel.org
Signed-off-by: David Carlier <devnexen@gmail.com>
---
kernel/tracepoint.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 91905aa19294..dffef52a807b 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -300,6 +300,8 @@ static int tracepoint_add_func(struct tracepoint *tp,
lockdep_is_held(&tracepoints_mutex));
old = func_add(&tp_funcs, func, prio);
if (IS_ERR(old)) {
+ if (tp->ext && tp->ext->unregfunc && !static_key_enabled(&tp->key))
+ tp->ext->unregfunc();
WARN_ON_ONCE(warn && PTR_ERR(old) != -ENOMEM);
return PTR_ERR(old);
}
--
2.53.0
^ permalink raw reply related
* Re: [LSF/MM/BPF TOPIC][RFC PATCH v4 00/27] Private Memory Nodes (w/ Compressed RAM)
From: Gregory Price @ 2026-04-13 17:05 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: lsf-pc, linux-kernel, linux-cxl, cgroups, linux-mm,
linux-trace-kernel, damon, kernel-team, gregkh, rafael, dakr,
dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams, longman, akpm,
lorenzo.stoakes, Liam.Howlett, vbabka, rppt, surenb, mhocko,
osalvador, ziy, matthew.brost, joshua.hahnjy, rakie.kim,
byungchul, ying.huang, apopple, axelrasmussen, yuanchu, weixugc,
yury.norov, linux, mhiramat, mathieu.desnoyers, tj, hannes,
mkoutny, jackmanb, sj, baolin.wang, npache, ryan.roberts,
dev.jain, baohua, lance.yang, muchun.song, xu.xin16,
chengming.zhou, jannh, linmiaohe, nao.horiguchi, pfalcato,
rientjes, shakeel.butt, riel, harry.yoo, cl, roman.gushchin,
chrisl, kasong, shikemeng, nphamcs, bhe, zhengqi.arch,
terry.bowman
In-Reply-To: <2608a03b-72bb-4033-8e6f-a439502b5573@kernel.org>
On Mon, Apr 13, 2026 at 03:11:12PM +0200, David Hildenbrand (Arm) wrote:
> > Normally cloud-hypervisor VMs with virtio-net can't be subject to KSM
> > because the entire boot region gets marked shared.
>
> What exactly do you mean with "mark shared". Do you mean, that "shared
> memory" is used in the hypervisor for all boot memory?
>
Sorry, meant MAP_SHARED. But yes, in some setups the hypervisor simply
makes a memfd with the entire main memory region MAP_SHARED.
This is because the virtio-net device / network stack does GFP_KERNEL
allocations and then pins them on the host to allow zero-copy - so all
of ZONE_NORMAL is a valid target.
(At least that's my best understanding of the entire setup).
>
> You mean, in the VM, memory usable by virtio-net can only be consumed
> from a dedicated physical memory region, and that region would be a
> separate node?
>
Correct - it does requires teaching the network stack numa awareness.
I was surprised by how little code this required, though I can't be
100% sure of its correctness since networking isn't my normal space.
Alternatively you could imagine this as a real device bringing its own
dedicated networking memory for network buffers, and then telling the
network start "Hey, prefer this node over normal kernel allocations".
What I'd been hacking on was cobbled together with memfd + SRAT bits to
bring up a private node statically and then have the device claim it -
but this is just a proof of concept. A proper implementation would be
extending virtio-net to report a dedicated EFI_RESERVED region.
> >
> > I see you saw below that one of the extensions is removing the nodes
> > from the fallback list. That is part one, but it's insufficient to
> > prevent complete leakage (someone might iterate over the nodes-possible
> > list and try migrating memory).
>
> Which code would do that?
>
There are many callers of for_each_node() throughout the system.
but one discrete example:
int alloc_shrinker_info(struct mem_cgroup *memcg)
{
... snip ...
for_each_node(nid) {
struct shrinker_info *info = kvzalloc_node(sizeof(*info) + array_size,
GFP_KERNEL, nid);
... snip ..
}
If you disallow fallbacks in this scenario, this allocation always fails.
This partially answers your question about slub fallback allocations,
there are slab allocations like this that depend on fallbacks (more
below on this explicitly).
> > Basically the only isolation mechanism we have today is ZONE_DEVICE.
> >
> > Either via mbind and friends, or even just the driver itself managing it
> > directly via alloc_pages_node() and exposing some userland interface.
>
> Would mbind() work here? I thought mbind() would not suddenly give
> access to some ZONE_DEVICE memory.
>
Sorry these were orthogonal thoughts.
1) We don't have such a mechanism. ZONE_DEVICE's preferred mechanism is
setting up explicit migrations via migrate_device.c
2) mbind / alloc_pages_node would only work for private nodes.
Extending ZONE_DEVICE to enable mbind() would be an extreme lift,
as the kernel makes a lot of assumptions about folio->lru.
This is why i went the node route in the first place.
> >
> > in the NP_OPS_MIGRATION patch, this gets covered.
>
> Right, but I am not sure if NP_OPS_MIGRATION is really the right
> approach for that. Have to think about that.
>
So, OPS is a bit misleading, but it's the closest i came to some
existing pattern. OPS does not necessarily need to imply callbacks.
I've been trying to minimize the patch set and I'm starting to think
the MVP may actually be able to do away with the private_ops structure
for a basic migration+mempolicy example by simply teaching some services
(migrate.c, mempolicy.c) how/when to inject __GFP_PRIVATE.
the mempolicy.c patch already does this, but not migrate.c - i haven't
figured out the right pattern for that yet.
> > 1) as you note, removing it from the default bitmaps, which is actually
> > hard. You can't remove it from the possible-node bitmap, so that
> > just seemed non-tractable.
>
> What about making people use a different set of bitmaps here? Quite some
> work, but maybe that's the right direction given that we'll now treat
> some nodes differently.
>
It's an option, although it is fragile. That means having to police all
future users of possible-nodes and for_each_node and etc.
I've been err'ing on the side of "not fragile", but i'm open to rework.
> >
> > 2) __GFP_THISNODE actually means (among other things) "don't fallback".
> > And, in fact, there are some hotplug-time allocations that occur in
> > SLAB (pglist_data) that target the private node that *must* fallback
> > to successfully allocate for successful kernel operation.
>
>
> Can you point me at the code?
>
There is actually a comment in slub.c that addresses this directly:
static int slab_mem_going_online_callback(int nid)
{
... snip ...
/*
* XXX: kmem_cache_alloc_node will fallback to other nodes
* since memory is not yet available from the node that
* is brought up.
*/
n = kmem_cache_alloc(kmem_cache_node, GFP_KERNEL);
... snip ...
}
Slab basically acknowledges the behavior is required on existing nodes
and just falls back immediately for the "going online" path.
Other specific calls in the hotplug path:
mm/sparse.c: kzalloc_node(size, GFP_KERNEL, nid)
mm/sparse-vmemmap.c: alloc_pages_node(nid, GFP_KERNEL|...)
mm/slub.c: kmalloc_node(sizeof(*barn), GFP_KERNEL, nid)
There are quite a number of callers to kmem_cache_alloc_node() that
would have to be individually audited.
And some non-slab interfaces examples as well:
alloc_shrinker_info
alloc_node_nr_active
I've been looking at this for a while, but I'm starting to think trying
to touch all this surface area is simply too fragile compared to just
letting normal memory be a fallback for private nodes and adding:
__GFP_PRIVATE - unlock's private node, but allow fallback
#define GFP_PRIVATE (__GFP_PRIVATE | __GFP_THISNODE) - only this node
__GFP_PRIVATE vs GFP_PRIVATE then is just a matter of use case.
For mbind() it probably makes sense we'd use GFP_PRIVATE - either it
succeeds or it OOMs.
> > The flexibility is kind of the point :]
>
> Yeah, but it would be interesting which minimal support we would need to
> just let some special memory be managed by the kernel, allowing mbind()
> users to use it, but not have any other fallback allocations end up on it.
>
> Something very basic, on which we could build additional functionality.
>
I actually have a simplistic CXL driver that does exactly this:
https://github.com/gourryinverse/linux/blob/072ecf7cbebd9871e76c0b52fd99aa1321405a59/drivers/cxl/type3_drivers/cxl_mempolicy/mempolicy.c#L65
We have to support migration because mbind can migrate on bind if the
VMA already has memory - but all this means is the migrate interfaces
are live - not that the kernel actually uses them.
so mbind requires (OPS_MIGRATE | OPS_MEMPOLICY)
All these flags say is:
- move_pages() syscalls can accept these nodes
- migrate_pages() function calls can accept these nodes
- mempolicy.c nodemasks allow the nodes (should restrict to mbind)
- vma's with these nodes now inject __GFP_PRIVATE on fault
All other services (reclaim, compaction, khugepaged, etc) do not scan
these nodes and do not know about __GFP_PRIVATE, so they never see
private node folios and can't allocate from the node.
In this example, all migrate_to() really does is inject __GFP_THISNODE,
but I've been thinking about whether we can just do this in migrate.c
and leave implementing the .ops to a user that requires is.
But otherwise "it just works".
One note here though - OOM conditions and allocation failures are not
intuitive, especially when THP/non-order-0 allocations are involved.
But that might just mean this minimal setup should only allow order-0
allocations - which is fiiiiiiiiiiiiiine :P.
-----------------
For basic examples
I've implemented 4 examples to consider building on:
1) CXL mempolicy driver:
https://github.com/gourryinverse/linux/blob/072ecf7cbebd9871e76c0b52fd99aa1321405a59/drivers/cxl/type3_drivers/cxl_mempolicy/mempolicy.c#L65
As described above
2) Virtio-net / CXL.mem Network Card
(Not published yet)
This doesn't require any ops at all - the plumbing happens entirely
inside the kernel. I onlined the node with an SRAT hack and no ops
structure at all associated with the device (just set node affinity
to the pcie_dev and plumbed it through the network stack).
A proper implementation would have virtio-net register is own
reserved memory region and online it during probe.
3) Accelerator
(Not published yet)
I have converted an open source but out of tree GPU driver which
uses NUMA nodes to use private nodes. This required:
NP_OPS_MIGRATION
NP_OPS_MEMPOLICY
The pattern is very similar to the CXL mempolicy driver, except
that the driver had alloc_pages_node() calls that needed to have
__GFP_PRIVATE added to ensure allocations landed on the device.
4) CXL Compressed RAM driver:
https://github.com/gourryinverse/linux/blob/55c06eb6bced58132d9001e318f2958e8ac80614/mm/cram.c#L340
needs pretty much everything - it's "normal memory" with access
rules, so the driver isn't really in the management lifecycle.
In this example - the only way to allocate memory on the node is
via demotion. This allows us to close off the device to new
allocations if the hardware reports low memory but the OS percieves
the device to still have free memory.
Which is a cool example: The driver just sets up the node with
certain attributes and then lets the kernel deal with it.
I have started compacting the _OPS_* flags related to reclaim into a
single NP_OPS_RECLAIM flag while testing with this. Really i've come
around to thinking many mm/ services need to be taken as a package,
not fully piecemeal.
The tl;dr: Once you cede some control over to the kernel, you're
very close to ceding ALL control, but you still get some control
over how/when allocations on the node can be made.
It is important to note that even if we don't expose callbacks, we do
still need a modicum of node filtering in some places that still use
for_each_node() (vmscan.c, compaction.c, oom_kill.c, etc).
These are basically all the places ZONE_DEVICE *implicitly* opts itself
out of by having managed_pages=0. We have to make those situations
explicit - but that doesn't mean we need callbacks.
> >
> > I would simply state: "That depends on the memory device"
>
> Let's keep it very simple: just some memory that you mbind(), and you
> only want the mbind() user to make use of that memory.
>
> What would be the minimal set of hooks to guarantee that.
>
If you want the mbind contract to stay intact:
NP_OPS_MIGRATION (mbind can generate migrations)
NP_OPS_MEMPOLICY (this just tells mempolicy.c to allow the node)
The set of callbacks required should be exactly 0 (assuming we teach
migrate.c to inject __GFP_PRIVATE like we have mempolicy.c).
If your device requires some special notification on allocation, free
or migration to/from you need:
ops.free_folio(folio)
ops.migrate_to(folios, nid, mode, reason, nr_success)
ops.migrate_folio(src_folio, dst_folio)
The free path is the tricky one to get right. You can imagine:
buf = malloc(...);
mbind(buf, private_node);
memset(buf, 0x42, ...);
ioctl(driver, CHECK_OUT_THIS_DATA, buf);
exit(0);
The task dies and frees the pages back to the buddy - the question is
whether the 4-5 free_folio paths (put_folio, put_unref_folios, etc) can
all eat an ops.free_folio() callback to inform the driver the memory has
been freed.
In practice - this worked on my accelerator and compressed examples, but
I can't say it's 100% safe in all contexts. The free path needs more
scrutiny.
> For example, I assume compaction could just be supported for such
> memory? Similarly, longterm-pinning.
>
> For some of the other hooks it's rather unclear how they would affect
> the very simple mbind() rule. What is the effect of demotion or NUMA
> balancing?
>
> I'm afraid we're making things too complicated here or it might be the
> wrong abstraction, if i cannot even figure out how to make the simplest
> use case work.
>
> Maybe I'm wrong :)
>
Actually, quite the opposite: None of that should be engaged by
default. In our above example:
OPS_MIGRATION | OPS_MEMPOLICY
All this should say is that migration and mempolicy are supported - not
that anything in the kernel that uses migration will suddenly operate on
that memory.
So: Compaction, Longterm Pin, NUMA balancing, Demotion - etc - all of
these do not ever operate on this memory by default. Your device driver
or service would have to specifically opt-in to those services and must
be capable of dealing with the implications of that.
---
kind of neat aside:
You can hotplug private ZONE_NORMAL without NP_OPS_LONGTERMPIN and as
long as the driver/service controls the type/lifetime of allocations,
the node can remain hot-unpluggable in the future.
e.g. if the service only ever allocates movable allocations, the lack
of NP_OPS_LONGTERMPIN prevents those pages from being pinned. If you
add NP_OPS_MIGRATION - the attempt to pin will cause migration :]
~Gregory
^ permalink raw reply
* Re: [PATCH 0/3] mm: split the file's i_mmap tree for NUMA
From: Mateusz Guzik @ 2026-04-13 15:33 UTC (permalink / raw)
To: Huang Shijie
Cc: akpm, viro, brauner, linux-mm, linux-kernel, linux-arm-kernel,
linux-fsdevel, muchun.song, osalvador, linux-trace-kernel,
linux-perf-users, linux-parisc, nvdimm, zhongyuan, fangbaoshun,
yingzhiwei
In-Reply-To: <20260413062042.804-1-huangsj@hygon.cn>
On Mon, Apr 13, 2026 at 02:20:39PM +0800, Huang Shijie wrote:
> In NUMA, there are maybe many NUMA nodes and many CPUs.
> For example, a Hygon's server has 12 NUMA nodes, and 384 CPUs.
> In the UnixBench tests, there is a test "execl" which tests
> the execve system call.
>
> When we test our server with "./Run -c 384 execl",
> the test result is not good enough. The i_mmap locks contended heavily on
> "libc.so" and "ld.so". For example, the i_mmap tree for "libc.so" can have
> over 6000 VMAs, all the VMAs can be in different NUMA mode.
> The insert/remove operations do not run quickly enough.
>
> patch 1 & patch 2 are try to hide the direct access of i_mmap.
> patch 3 splits the i_mmap into sibling trees, and we can get better
> performance with this patch set:
> we can get 77% performance improvement(10 times average)
>
To my reading you kept the lock as-is and only distributed the protected
state.
While I don't doubt the improvement, I'm confident should you take a
look at the profile you are going to find this still does not scale with
rwsem being one of the problems (there are other global locks, some of
which have experimental patches for).
Apart from that this does nothing to help high core systems which are
all one node, which imo puts another question mark on this specific
proposal.
Of course one may question whether a RB tree is the right choice here,
it may be the lock-protected cost can go way down with merely a better
data structure.
Regardless of that, for actual scalability, there will be no way around
decentralazing locking around this and partitioning per some core count
(not just by numa awareness).
Decentralizing locking is definitely possible, but I have not looked
into specifics of how problematic it is. Best case scenario it will
merely with separate locks. Worst case scenario something needs a fully
stabilized state for traversal, in that case another rw lock can be
slapped around this, creating locking order read lock -> per-subset
write lock -- this will suffer scalability due to the read locking, but
it will still scale drastically better as apart from that there will be
no serialization. In this setting the problematic consumer will write
lock the new thing to stabilize the state.
So my non-maintainer opinion is that the patchset is not worth it as it
fails to address anything for significantly more common and already
affected setups.
Have you looked into splitting the lock?
^ permalink raw reply
* Re: [RFC v4 0/7] ext4: fast commit: snapshot inode state for FC log
From: Theodore Tso @ 2026-04-13 13:12 UTC (permalink / raw)
To: Li Chen
Cc: Zhang Yi, Andreas Dilger, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, linux-ext4, linux-trace-kernel, linux-kernel
In-Reply-To: <19d86eec635.f7072461135455.4960134919814592320@linux.beauty>
On Mon, Apr 13, 2026 at 09:01:28PM +0800, Li Chen wrote:
> Absolutely! It's great to learn about the Sashiko development site.
> I will address the real issues in the next version.
Note that Sashiko will sometimes report a pre-existing issue as if it
were a problem with the commit. If that happens, feel free to ignore
its complaint; what I consider best practice is to either (a) fix it
in the a subsequent patch or patch series, or (b) leave a TODO in the
code.
I've asked the Sashiko folks to add way for URI's for each issue that
are identified by Sashiko, so we can put a URL in the TODO comment for
someone who wants to fix it later, and to make it easier for Sashiko
to identified pre-existing issues so it doesn't comment on the same
issue across multiple commit reviews (and perhaps save on the some LLM
token budget :-).
In the next few days, for patches sent to linux-ext4, Sashiko will
start e-mailing its reviews to the patch submitter and to me as the
maintainer. Once we can reduce the false positive rate, I'll ask that
the reviews be cc'ed to the linux-ext4 mailing list. But it seems
good enough that to send e-mails to the patch submitter and the
maintainer --- but that's a decision that each subsystem maintainer
will be making on their own.
Cheers,
- Ted
^ permalink raw reply
* Re: [LSF/MM/BPF TOPIC][RFC PATCH v4 00/27] Private Memory Nodes (w/ Compressed RAM)
From: David Hildenbrand (Arm) @ 2026-04-13 13:11 UTC (permalink / raw)
To: Gregory Price
Cc: lsf-pc, linux-kernel, linux-cxl, cgroups, linux-mm,
linux-trace-kernel, damon, kernel-team, gregkh, rafael, dakr,
dave, jonathan.cameron, dave.jiang, alison.schofield,
vishal.l.verma, ira.weiny, dan.j.williams, longman, akpm,
lorenzo.stoakes, Liam.Howlett, vbabka, rppt, surenb, mhocko,
osalvador, ziy, matthew.brost, joshua.hahnjy, rakie.kim,
byungchul, ying.huang, apopple, axelrasmussen, yuanchu, weixugc,
yury.norov, linux, mhiramat, mathieu.desnoyers, tj, hannes,
mkoutny, jackmanb, sj, baolin.wang, npache, ryan.roberts,
dev.jain, baohua, lance.yang, muchun.song, xu.xin16,
chengming.zhou, jannh, linmiaohe, nao.horiguchi, pfalcato,
rientjes, shakeel.butt, riel, harry.yoo, cl, roman.gushchin,
chrisl, kasong, shikemeng, nphamcs, bhe, zhengqi.arch,
terry.bowman
In-Reply-To: <abwRu1FNqI3dVyqL@gourry-fedora-PF4VCD3F>
On 3/19/26 16:09, Gregory Price wrote:
> On Tue, Mar 17, 2026 at 02:25:29PM +0100, David Hildenbrand (Arm) wrote:
>> On 2/22/26 09:48, Gregory Price wrote:
>>> Topic type: MM
>>
>> Hi Gregory,
>>
>> stumbling over this again, some questions whereby I'll just ignore the
>> compressed RAM bits for now and focus on use cases where promotion etc
>> are not relevant :)
>
> A more concrete example up your alley:
>
> I've since been playing with a virtio-net private node.
>
> Normally cloud-hypervisor VMs with virtio-net can't be subject to KSM
> because the entire boot region gets marked shared.
What exactly do you mean with "mark shared". Do you mean, that "shared
memory" is used in the hypervisor for all boot memory?
> If virtio-net has
> its own private node / region separate from the boot region, the boot
> region is now free to be subject to KSM.
You mean, in the VM, memory usable by virtio-net can only be consumed
from a dedicated physical memory region, and that region would be a
separate node?
>
> I may have that up as an example sometime before LSF, but i need to
> clean up some networking stack hacks i've made to make it work.
>
>>>
>>> N_MEMORY_PRIVATE is all about isolating NUMA nodes and then punching
>>> explicit holes in that isolation to do useful things we couldn't do
>>> before without re-implementing entire portions of mm/ in a driver.
>>
>> Just to clarify: we don't currently have any mechanism to expose, say,
>> SPM/PMEM/whatsoever to the buddy allocator through the dax/kmem driver
>> and *not* have random allocations end up on it, correct?
>>
>> Assume we online the memory to ZONE_MOVABLE, still other (fallback)
>> allocations might end up on that memory.
>>
>
> Correct, when you hotplug memory into a node, it's a free for all.
> Fallbacks are going to happen.
Right, and I agree that having a mechanism to prevent that is reasonable.
>
> I see you saw below that one of the extensions is removing the nodes
> from the fallback list. That is part one, but it's insufficient to
> prevent complete leakage (someone might iterate over the nodes-possible
> list and try migrating memory).
Which code would do that?
>
>> How would we currently handle something like that? (do we have drivers
>> for that? I'd assume that drivers would only migrate some user memory to
>> ZONE_DEVICE memory.)
>>
>> Assuming we don't have such a mechanism, I assume that part of your
>> proposal would be very interesting: online the memory to a
>> "special"/"restricted" (you call it private) NUMA node, whereby all
>> memory of that NUMA node will only be consumable through
>> mbind() and friends.
>>
>
> Basically the only isolation mechanism we have today is ZONE_DEVICE.
>
> Either via mbind and friends, or even just the driver itself managing it
> directly via alloc_pages_node() and exposing some userland interface.
Would mbind() work here? I thought mbind() would not suddenly give
access to some ZONE_DEVICE memory.
>
> You can imagine a network driver providing an ioctl for a shared buffer
> or a driver exposing a mmap'able file descriptor as the trivial case.
Right.
>
>> Any other allocations (including automatic page migration etc) would not
>> end up on that memory.
>
> One of the complications of exposing this memory via mbind is that
> mempolicy.c has a lot of migration mechanics, just to name two:
>
> - migrate on mbind
> - cpuset rebinds
>
> So for a completely solution you need to support migration if you
> support mempolicy. But with the callbacks, you can control how/when
> migration occurs.
>
> tl;dr: many of mm/'s services are actually predicated on migration
> support, so you have to manage that somehow.
Agreed.
>
>>
>> Thinking of some "terribly slow" or "terribly fast" memory that we don't
>> want to involve in automatic memory tiering, being able to just let
>> selected workloads consume that memory sounds very helpful.
>>
>>
>> (wondering if there could be some way allocations might get migrated out
>> of the node, for example, during memory offlining etc, which might also
>> not be desirable)
>>
>
> in the NP_OPS_MIGRATION patch, this gets covered.
Right, but I am not sure if NP_OPS_MIGRATION is really the right
approach for that. Have to think about that.
>
> I'm not sure the NP_OPS_* pattern is what we actually want, it's just
> what i came up with to make it clear what's being enabled.
>
> Basically without NP_OPS_MIGRATION, this memory is completely
> non-migratable. The driver managing it therefore needs to control the
> lifetime, and if hotplug is requested - kill anyone using it (which by
> definition should not the kernel) and either release the pages or take
> them so they can be released while hotplug is spinning.
>
>> I am not sure if __GFP_PRIVATE etc is really required for that. But some
>> mechanism to make that work seems extremely helpful.
>>
>> Because ...
>>
>>> /* And now I can use mempolicy with my memory */
>>> buf = mmap(...);
>>> mbind(buf, len, mode, private_node, ...);
>>> buf[0] = 0xdeadbeef; /* Faults onto private node */
>>
>> ... just being able to consume that memory through mbind() and having
>> guarantees sounds extremely helpful.
>>
>
> Yes! :]
>
>>>
>>> - Filter allocation requests on __GFP_PRIVATE
>>> numa_zone_allowed() excludes them otherwise.
>>
>> I think we discussed that in the past, but why can't we find a way that
>> only people requesting __GFP_THISNODE could allocate that memory, for
>> example? I guess we'd have to remove it from all "default NUMA bitmaps"
>> somehow.
>>
>
> I experimented with this. There were two concerns:
>
> 1) as you note, removing it from the default bitmaps, which is actually
> hard. You can't remove it from the possible-node bitmap, so that
> just seemed non-tractable.
What about making people use a different set of bitmaps here? Quite some
work, but maybe that's the right direction given that we'll now treat
some nodes differently.
>
> 2) __GFP_THISNODE actually means (among other things) "don't fallback".
> And, in fact, there are some hotplug-time allocations that occur in
> SLAB (pglist_data) that target the private node that *must* fallback
> to successfully allocate for successful kernel operation.
Can you point me at the code?
>
> So separating PRIVATE from THISNODE and allowing some use of fallback
> mechanics resolves some problems here.
>
> I think #2 is a solvable problem, but #1 i don't think can be addressed.
> I need to investigate the slab interactions a little more.
I'll also have to think about this some more.
>
>>> - Use standard struct page / folio. No ZONE_DEVICE, no pgmap,
>>> no struct page metadata limitations.
>>
>> Good.
>
> Note: I've actually since explored merging this with pgmap, and
> rebranding it as node-scope pgmap.
>
> In that sense, you could think of this as NODE_DEVICE instead of
> NODE_PRIVATE - but maybe I'm inviting too much baggage :]
:)
NODE_DEVICE sounds interesting though.
>
>>>
>>> Re-use of ZONE_DEVICE Hooks
>>> ===
>>
>> I think all of that might not be required for the simplistic use case I
>> mentioned above (fast/slow memory only to be consumed by selected user
>> space that opts in through mbind() and friends).
>>
>> Or are there other use cases for these callbacks
>>
>
> Many `folio_is_zone_device()` hooks result in the operations being
> a no-op / failing. We need all those same hooks.
>
> Some hooks I added - such as migration hooks, are combined with the
> zone_device hooks via i helper to demonstrate the pattern is the same
> when the memory is opted into migration.
>
> I do not think all of these hooks are required, I would think of this
> more as an exploration of the whole space, and then we can throw what
> does not have an active use case.
>
> For the compressed ram component I've been designing, the needs are:
>
> - Migration
> - Reclaim
> - Demotion
> - Write Protect (maybe, possibly optional)
>
> But you could argue another user might want the same device to have:
> - Migration
> - Mempolicy
>
> Where they manage things from userland, rather than via reclaim.
>
> The flexibility is kind of the point :]
Yeah, but it would be interesting which minimal support we would need to
just let some special memory be managed by the kernel, allowing mbind()
users to use it, but not have any other fallback allocations end up on it.
Something very basic, on which we could build additional functionality.
>
>> [...]
>>>
>>>
>>> Flag-gated behavior (NP_OPS_*) controls:
>>> ===
>>>
>>> We use OPS flags to denote what mm/ services we want to allow on our
>>> private node. I've plumbed these through so far:
>>>
>>> NP_OPS_MIGRATION - Node supports migration
>>> NP_OPS_MEMPOLICY - Node supports mempolicy actions
>>> NP_OPS_DEMOTION - Node appears in demotion target lists
>>> NP_OPS_PROTECT_WRITE - Node memory is read-only (wrprotect)
>>> NP_OPS_RECLAIM - Node supports reclaim
>>> NP_OPS_NUMA_BALANCING - Node supports numa balancing
>>> NP_OPS_COMPACTION - Node supports compaction
>>> NP_OPS_LONGTERM_PIN - Node supports longterm pinning
>>> NP_OPS_OOM_ELIGIBLE - (MIGRATION | DEMOTION), node is reachable
>>> as normal system ram storage, so it should
>>> be considered in OOM pressure calculations.
>>
>> I have to think about all that, and whether that would be required as a
>> first step. I'd assume in a simplistic use case mentioned above we might
>> only forbid the memory to be used as a fallback for any oom etc.
>>
>> Whether reclaim (e.g., swapout) makes sense is a good question.
>>
>
> I would simply state: "That depends on the memory device"
Let's keep it very simple: just some memory that you mbind(), and you
only want the mbind() user to make use of that memory.
What would be the minimal set of hooks to guarantee that.
For example, I assume compaction could just be supported for such
memory? Similarly, longterm-pinning.
For some of the other hooks it's rather unclear how they would affect
the very simple mbind() rule. What is the effect of demotion or NUMA
balancing?
I'm afraid we're making things too complicated here or it might be the
wrong abstraction, if i cannot even figure out how to make the simplest
use case work.
Maybe I'm wrong :)
>
> Which is kind of the point. The ability to isolate and poke holes in
> that isolation explictly, while using the same mm/ code, creates a new
> design space we haven't had before.
>
> ---
>
> I think it would be fair to say all of these would not be required for
> an MVP interface, and should require a use case to merge. But the code
> is here because I wanted to explore just how far it can go.
That's absolutely fair. :)
--
Cheers,
David
^ permalink raw reply
* Re: [RFC v4 0/7] ext4: fast commit: snapshot inode state for FC log
From: Li Chen @ 2026-04-13 13:01 UTC (permalink / raw)
To: Theodore Tso
Cc: Zhang Yi, Andreas Dilger, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, linux-ext4, linux-trace-kernel, linux-kernel
In-Reply-To: <20260410011843.GD99725@macsyma-wired.lan>
Hi Ted,
---- On Fri, 10 Apr 2026 09:18:43 +0800 Theodore Tso <tytso@mit.edu> wrote ---
> On Tue, Jan 20, 2026 at 07:25:29PM +0800, Li Chen wrote:
> > Hi,
> >
> > (This RFC v4 series is based on linux-next tag next-20260106, plus the
> > prerequisite patch "ext4: fast commit: make s_fc_lock reclaim-safe" posted at:
> > https://lore.kernel.org/all/20260106120621.440126-1-me@linux.beauty/)
>
> Can you take a look at the Sashiko reviews here:
>
> https://sashiko.dev/#/patchset/20260408112020.716706-1-me%40linux.beauty
>
> There seems to be at least one legitimate concern, which is the
> potential cur_lblk overflow. There are a couple of others which I
> think is real; could you please look at their review comments?
Absolutely! It's great to learn about the Sashiko development site.
I will address the real issues in the next version.
Regards,
Li
^ permalink raw reply
* Re: [RFC v6 6/7] ext4: fast commit: add lock_updates tracepoint
From: Li Chen @ 2026-04-13 12:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: Zhang Yi, Theodore Ts'o, Andreas Dilger, Baokun Li, Jan Kara,
Ojaswin Mujoo, Ritesh Harjani, Zhang Yi, Masami Hiramatsu,
Mathieu Desnoyers, linux-ext4, linux-kernel, linux-trace-kernel
In-Reply-To: <20260408160405.45a5ee09@gandalf.local.home>
Hi Steven,
---- On Thu, 09 Apr 2026 04:02:56 +0800 Steven Rostedt <rostedt@goodmis.org> wrote ---
> On Wed, 8 Apr 2026 19:20:17 +0800
> Li Chen <me@linux.beauty> wrote:
>
> > Commit-time fast commit snapshots run under jbd2_journal_lock_updates(),
> > so it is useful to quantify the time spent with updates locked and to
> > understand why snapshotting can fail.
> >
> > Add a new tracepoint, ext4_fc_lock_updates, reporting the time spent in
> > the updates-locked window along with the number of snapshotted inodes
> > and ranges. Record the first snapshot failure reason in a stable snap_err
> > field for tooling.
> >
>
> [..]
>
> > @@ -1338,13 +1375,13 @@ static int ext4_fc_perform_commit(journal_t *journal)
> > if (ret)
> > return ret;
> >
> > -
> > ret = ext4_fc_alloc_snapshot_inodes(sb, &inodes, &inodes_size);
> > if (ret)
> > return ret;
> >
> > /* Step 4: Mark all inodes as being committed. */
> > jbd2_journal_lock_updates(journal);
> > + lock_start = ktime_get();
>
> ktime_get() is rather quick but if you care about micro-optimizations, you
> could have:
>
> if (trace_ext4_fc_lock_updates_enabled())
> lock_start = ktime_get();
> else
> lock_start = 0;
>
> > /*
> > * The journal is now locked. No more handles can start and all the
> > * previous handles are now drained. Snapshotting happens in this
> > @@ -1358,8 +1395,15 @@ static int ext4_fc_perform_commit(journal_t *journal)
> > }
> > ext4_fc_unlock(sb, alloc_ctx);
> >
> > - ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size);
> > + ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size,
> > + &snap_inodes, &snap_ranges, &snap_err);
> > jbd2_journal_unlock_updates(journal);
> > + if (trace_ext4_fc_lock_updates_enabled()) {
>
> if (trace_ext4_fc_lock_updates_enabled() && lock_start) {
>
> But feel free to ignore this if the overhead of always calling ktime_get()
> is not an issue.
>
>
> > + locked_ns = ktime_to_ns(ktime_sub(ktime_get(), lock_start));
> > + trace_ext4_fc_lock_updates(sb, commit_tid, locked_ns,
> > + snap_inodes, snap_ranges, ret,
> > + snap_err);
> > + }
> > kvfree(inodes);
> > if (ret)
> > return ret;
> > @@ -1564,7 +1608,7 @@ int ext4_fc_commit(journal_t *journal, tid_t commit_tid)
> > journal_ioprio = EXT4_DEF_JOURNAL_IOPRIO;
> > set_task_ioprio(current, journal_ioprio);
> > fc_bufs_before = (sbi->s_fc_bytes + bsize - 1) / bsize;
> > - ret = ext4_fc_perform_commit(journal);
> > + ret = ext4_fc_perform_commit(journal, commit_tid);
> > if (ret < 0) {
> > if (ret == -EAGAIN || ret == -E2BIG || ret == -ECANCELED)
> > status = EXT4_FC_STATUS_INELIGIBLE;
> > diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> > index f493642cf121..7028a28316fa 100644
> > --- a/include/trace/events/ext4.h
> > +++ b/include/trace/events/ext4.h
> > @@ -107,6 +107,26 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_VERITY);
> > TRACE_DEFINE_ENUM(EXT4_FC_REASON_MOVE_EXT);
> > TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX);
> >
> > +#undef EM
> > +#undef EMe
> > +#define EM(a) TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
> > +#define EMe(a) TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
> > +
> > +#define TRACE_SNAP_ERR \
> > + EM(NONE) \
> > + EM(ES_MISS) \
> > + EM(ES_DELAYED) \
> > + EM(ES_OTHER) \
> > + EM(INODES_CAP) \
> > + EM(RANGES_CAP) \
> > + EM(NOMEM) \
> > + EMe(INODE_LOC)
> > +
> > +TRACE_SNAP_ERR
> > +
> > +#undef EM
> > +#undef EMe
> > +
> > #define show_fc_reason(reason) \
> > __print_symbolic(reason, \
> > { EXT4_FC_REASON_XATTR, "XATTR"}, \
> > @@ -2818,6 +2838,47 @@ TRACE_EVENT(ext4_fc_commit_stop,
> > __entry->num_fc_ineligible, __entry->nblks_agg, __entry->tid)
> > );
> >
> > +#define EM(a) { EXT4_FC_SNAP_ERR_##a, #a },
> > +#define EMe(a) { EXT4_FC_SNAP_ERR_##a, #a }
> > +
> > +TRACE_EVENT(ext4_fc_lock_updates,
> > + TP_PROTO(struct super_block *sb, tid_t commit_tid, u64 locked_ns,
> > + unsigned int nr_inodes, unsigned int nr_ranges, int err,
> > + int snap_err),
> > +
> > + TP_ARGS(sb, commit_tid, locked_ns, nr_inodes, nr_ranges, err, snap_err),
> > +
> > + TP_STRUCT__entry(/* entry */
> > + __field(dev_t, dev)
> > + __field(tid_t, tid)
> > + __field(u64, locked_ns)
> > + __field(unsigned int, nr_inodes)
> > + __field(unsigned int, nr_ranges)
> > + __field(int, err)
> > + __field(int, snap_err)
> > + ),
> > +
> > + TP_fast_assign(/* assign */
> > + __entry->dev = sb->s_dev;
> > + __entry->tid = commit_tid;
> > + __entry->locked_ns = locked_ns;
> > + __entry->nr_inodes = nr_inodes;
> > + __entry->nr_ranges = nr_ranges;
> > + __entry->err = err;
> > + __entry->snap_err = snap_err;
> > + ),
> > +
> > + TP_printk("dev %d,%d tid %u locked_ns %llu nr_inodes %u nr_ranges %u err %d snap_err %s",
> > + MAJOR(__entry->dev), MINOR(__entry->dev), __entry->tid,
> > + __entry->locked_ns, __entry->nr_inodes, __entry->nr_ranges,
> > + __entry->err, __print_symbolic(__entry->snap_err,
> > + TRACE_SNAP_ERR))
> > +);
> > +
> > +#undef EM
> > +#undef EMe
> > +#undef TRACE_SNAP_ERR
> > +
> > #define FC_REASON_NAME_STAT(reason) \
> > show_fc_reason(reason), \
> > __entry->fc_ineligible_rc[reason]
>
> As for the rest:
>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
>
> [ Please add this reviewed-by to any new versions so I remember I already
> looked at it. ]
Sure, thanks a lot for your thoughtful review!
Regards,
Li
^ permalink raw reply
* [PATCH] tracing: separate module tracepoint strings from trace_printk formats
From: Cao Ruichuang @ 2026-04-13 12:33 UTC (permalink / raw)
To: petr.pavlu; +Cc: linux-trace-kernel, linux-kernel, mhiramat, Cao Ruichuang
In-Reply-To: <41e81533-0fd6-49f5-b7c1-b4e172affd2a@suse.com>
The previous module tracepoint_string() fix took the smallest
implementation path and reused the existing module trace_printk format
storage.
That was enough to make module __tracepoint_str entries show up in
printk_formats and be accepted by trace_is_tracepoint_string(), but it
also made those copied mappings persist after module unload. That does
not match the expected module lifetime semantics.
Handle module tracepoint_string() mappings separately instead of mixing
them into the module trace_printk format list. Keep copying the strings
into tracing-managed storage while the module is loaded, but track them
on their own list and drop them again on MODULE_STATE_GOING.
Keep module trace_printk format handling unchanged.
This split is intentional: module trace_printk formats and module
tracepoint_string() mappings do not have the same lifetime requirements.
Keeping them in one shared structure would either preserve
tracepoint_string() mappings too long again, or require mixed
ownership/refcount rules in a trace_printk-oriented structure.
The separate module tracepoint_string() list intentionally keeps one
copied mapping per module entry instead of trying to share copies across
modules by string contents. printk_formats is address-based, and sharing
those copies would add another layer of shared ownership/refcounting
without changing the lifetime rule this fix is trying to restore.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=217196
Signed-off-by: Cao Ruichuang <create0818@163.com>
---
include/linux/tracepoint.h | 9 +-
kernel/trace/trace_printk.c | 250 ++++++++++++++++++++++--------------
2 files changed, 157 insertions(+), 102 deletions(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index f14da542402..aec598a4017 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -479,11 +479,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
*
* For built-in code, the tracing system uses the original string address.
* For modules, the tracing code saves tracepoint strings into
- * tracing-managed storage when the module loads, so their mappings remain
- * available through printk_formats and trace string checks even after the
- * module's own memory goes away. As long as the string does not change
- * during the life of the module, it is fine to use tracepoint_string()
- * within a module.
+ * tracing-managed storage while the module is loaded, and drops those
+ * mappings again when the module unloads. As long as the string does not
+ * change during the life of the module, it is fine to use
+ * tracepoint_string() within a module.
*/
#define tracepoint_string(str) \
({ \
diff --git a/kernel/trace/trace_printk.c b/kernel/trace/trace_printk.c
index 9f67ce42ef6..0420ffcff93 100644
--- a/kernel/trace/trace_printk.c
+++ b/kernel/trace/trace_printk.c
@@ -21,24 +21,24 @@
#ifdef CONFIG_MODULES
-/*
- * modules trace_printk() formats and tracepoint_string() strings are
- * autosaved in struct trace_bprintk_fmt, which are queued on
- * trace_bprintk_fmt_list.
- */
+/* module trace_printk() formats are autosaved on trace_bprintk_fmt_list. */
static LIST_HEAD(trace_bprintk_fmt_list);
+/* module tracepoint_string() copies live on tracepoint_string_list. */
+static LIST_HEAD(tracepoint_string_list);
-/* serialize accesses to trace_bprintk_fmt_list */
+/* serialize accesses to module format and tracepoint-string lists */
static DEFINE_MUTEX(btrace_mutex);
struct trace_bprintk_fmt {
struct list_head list;
const char *fmt;
- unsigned int type;
};
-#define TRACE_BPRINTK_TYPE BIT(0)
-#define TRACE_TRACEPOINT_TYPE BIT(1)
+struct tracepoint_string_entry {
+ struct list_head list;
+ struct module *mod;
+ const char *str;
+};
static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
{
@@ -54,24 +54,21 @@ static inline struct trace_bprintk_fmt *lookup_format(const char *fmt)
return NULL;
}
-static void hold_module_trace_format(const char **start, const char **end,
- unsigned int type)
+static void hold_module_trace_bprintk_format(const char **start, const char **end)
{
const char **iter;
char *fmt;
/* allocate the trace_printk per cpu buffers */
- if ((type & TRACE_BPRINTK_TYPE) && start != end)
+ if (start != end)
trace_printk_init_buffers();
mutex_lock(&btrace_mutex);
for (iter = start; iter < end; iter++) {
struct trace_bprintk_fmt *tb_fmt = lookup_format(*iter);
if (tb_fmt) {
- if (!IS_ERR(tb_fmt)) {
- tb_fmt->type |= type;
+ if (!IS_ERR(tb_fmt))
*iter = tb_fmt->fmt;
- }
continue;
}
@@ -83,7 +80,6 @@ static void hold_module_trace_format(const char **start, const char **end,
list_add_tail(&tb_fmt->list, &trace_bprintk_fmt_list);
strcpy(fmt, *iter);
tb_fmt->fmt = fmt;
- tb_fmt->type = type;
} else
kfree(tb_fmt);
}
@@ -93,89 +89,156 @@ static void hold_module_trace_format(const char **start, const char **end,
mutex_unlock(&btrace_mutex);
}
+static void hold_module_tracepoint_strings(struct module *mod,
+ const char **start,
+ const char **end)
+{
+ const char **iter;
+
+ mutex_lock(&btrace_mutex);
+ for (iter = start; iter < end; iter++) {
+ struct tracepoint_string_entry *tp_entry;
+ char *str;
+
+ tp_entry = kmalloc_obj(*tp_entry);
+ if (!tp_entry)
+ continue;
+
+ str = kstrdup(*iter, GFP_KERNEL);
+ if (!str) {
+ kfree(tp_entry);
+ continue;
+ }
+
+ tp_entry->mod = mod;
+ tp_entry->str = str;
+ list_add_tail(&tp_entry->list, &tracepoint_string_list);
+ *iter = tp_entry->str;
+ }
+ mutex_unlock(&btrace_mutex);
+}
+
+static void release_module_tracepoint_strings(struct module *mod)
+{
+ struct tracepoint_string_entry *tp_entry, *n;
+
+ mutex_lock(&btrace_mutex);
+ list_for_each_entry_safe(tp_entry, n, &tracepoint_string_list, list) {
+ if (tp_entry->mod != mod)
+ continue;
+ list_del(&tp_entry->list);
+ kfree(tp_entry->str);
+ kfree(tp_entry);
+ }
+ mutex_unlock(&btrace_mutex);
+}
+
static int module_trace_format_notify(struct notifier_block *self,
unsigned long val, void *data)
{
struct module *mod = data;
- if (val != MODULE_STATE_COMING)
- return NOTIFY_OK;
+ switch (val) {
+ case MODULE_STATE_COMING:
+ if (mod->num_trace_bprintk_fmt) {
+ const char **start = mod->trace_bprintk_fmt_start;
+ const char **end = start + mod->num_trace_bprintk_fmt;
- if (mod->num_trace_bprintk_fmt) {
- const char **start = mod->trace_bprintk_fmt_start;
- const char **end = start + mod->num_trace_bprintk_fmt;
+ hold_module_trace_bprintk_format(start, end);
+ }
+
+ if (mod->num_tracepoint_strings) {
+ const char **start = mod->tracepoint_strings_start;
+ const char **end = start + mod->num_tracepoint_strings;
- hold_module_trace_format(start, end, TRACE_BPRINTK_TYPE);
+ hold_module_tracepoint_strings(mod, start, end);
+ }
+ break;
+ case MODULE_STATE_GOING:
+ release_module_tracepoint_strings(mod);
+ break;
}
- if (mod->num_tracepoint_strings) {
- const char **start = mod->tracepoint_strings_start;
- const char **end = start + mod->num_tracepoint_strings;
+ return NOTIFY_OK;
+}
- hold_module_trace_format(start, end, TRACE_TRACEPOINT_TYPE);
+static const char **find_first_mod_entry(void)
+{
+ struct trace_bprintk_fmt *tb_fmt;
+ struct tracepoint_string_entry *tp_entry;
+
+ if (!list_empty(&trace_bprintk_fmt_list)) {
+ tb_fmt = list_first_entry(&trace_bprintk_fmt_list,
+ typeof(*tb_fmt), list);
+ return &tb_fmt->fmt;
}
- return NOTIFY_OK;
+ if (!list_empty(&tracepoint_string_list)) {
+ tp_entry = list_first_entry(&tracepoint_string_list,
+ typeof(*tp_entry), list);
+ return &tp_entry->str;
+ }
+
+ return NULL;
}
-/*
- * The debugfs/tracing/printk_formats file maps the addresses with
- * the ASCII formats that are used in the bprintk events in the
- * buffer. For userspace tools to be able to decode the events from
- * the buffer, they need to be able to map the address with the format.
- *
- * The addresses of the bprintk formats are in their own section
- * __trace_printk_fmt. But for modules we copy them into a link list.
- * The code to print the formats and their addresses passes around the
- * address of the fmt string. If the fmt address passed into the seq
- * functions is within the kernel core __trace_printk_fmt section, then
- * it simply uses the next pointer in the list.
- *
- * When the fmt pointer is outside the kernel core __trace_printk_fmt
- * section, then we need to read the link list pointers. The trick is
- * we pass the address of the string to the seq function just like
- * we do for the kernel core formats. To get back the structure that
- * holds the format, we simply use container_of() and then go to the
- * next format in the list.
- */
-static const char **
-find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
+static struct trace_bprintk_fmt *lookup_mod_format_ptr(const char **fmt_ptr)
{
- struct trace_bprintk_fmt *mod_fmt;
+ struct trace_bprintk_fmt *tb_fmt;
- if (list_empty(&trace_bprintk_fmt_list))
- return NULL;
+ list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
+ if (fmt_ptr == &tb_fmt->fmt)
+ return tb_fmt;
+ }
- /*
- * v will point to the address of the fmt record from t_next
- * v will be NULL from t_start.
- * If this is the first pointer or called from start
- * then we need to walk the list.
- */
- if (!v || start_index == *pos) {
- struct trace_bprintk_fmt *p;
-
- /* search the module list */
- list_for_each_entry(p, &trace_bprintk_fmt_list, list) {
- if (start_index == *pos)
- return &p->fmt;
- start_index++;
+ return NULL;
+}
+
+static struct tracepoint_string_entry *lookup_mod_tracepoint_ptr(const char **str_ptr)
+{
+ struct tracepoint_string_entry *tp_entry;
+
+ list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
+ if (str_ptr == &tp_entry->str)
+ return tp_entry;
+ }
+
+ return NULL;
+}
+
+static const char **find_next_mod_entry(int start_index, void *v, loff_t *pos)
+{
+ struct trace_bprintk_fmt *tb_fmt;
+ struct tracepoint_string_entry *tp_entry;
+
+ if (!v || start_index == *pos)
+ return find_first_mod_entry();
+
+ tb_fmt = lookup_mod_format_ptr(v);
+ if (tb_fmt) {
+ if (tb_fmt->list.next != &trace_bprintk_fmt_list) {
+ tb_fmt = list_next_entry(tb_fmt, list);
+ return &tb_fmt->fmt;
}
- /* pos > index */
+
+ if (!list_empty(&tracepoint_string_list)) {
+ tp_entry = list_first_entry(&tracepoint_string_list,
+ typeof(*tp_entry), list);
+ return &tp_entry->str;
+ }
+
return NULL;
}
- /*
- * v points to the address of the fmt field in the mod list
- * structure that holds the module print format.
- */
- mod_fmt = container_of(v, typeof(*mod_fmt), fmt);
- if (mod_fmt->list.next == &trace_bprintk_fmt_list)
+ tp_entry = lookup_mod_tracepoint_ptr(v);
+ if (!tp_entry)
return NULL;
- mod_fmt = container_of(mod_fmt->list.next, typeof(*mod_fmt), list);
+ if (tp_entry->list.next == &tracepoint_string_list)
+ return NULL;
- return &mod_fmt->fmt;
+ tp_entry = list_next_entry(tp_entry, list);
+ return &tp_entry->str;
}
static void format_mod_start(void)
@@ -195,8 +258,8 @@ module_trace_format_notify(struct notifier_block *self,
{
return NOTIFY_OK;
}
-static inline const char **
-find_next_mod_format(int start_index, void *v, const char **fmt, loff_t *pos)
+static inline const char **find_next_mod_entry(int start_index, void *v,
+ loff_t *pos)
{
return NULL;
}
@@ -274,7 +337,7 @@ bool trace_is_tracepoint_string(const char *str)
{
const char **ptr = __start___tracepoint_str;
#ifdef CONFIG_MODULES
- struct trace_bprintk_fmt *tb_fmt;
+ struct tracepoint_string_entry *tp_entry;
#endif
for (ptr = __start___tracepoint_str; ptr < __stop___tracepoint_str; ptr++) {
@@ -284,8 +347,8 @@ bool trace_is_tracepoint_string(const char *str)
#ifdef CONFIG_MODULES
mutex_lock(&btrace_mutex);
- list_for_each_entry(tb_fmt, &trace_bprintk_fmt_list, list) {
- if ((tb_fmt->type & TRACE_TRACEPOINT_TYPE) && str == tb_fmt->fmt) {
+ list_for_each_entry(tp_entry, &tracepoint_string_list, list) {
+ if (str == tp_entry->str) {
mutex_unlock(&btrace_mutex);
return true;
}
@@ -297,9 +360,8 @@ bool trace_is_tracepoint_string(const char *str)
static const char **find_next(void *v, loff_t *pos)
{
- const char **fmt = v;
int start_index;
- int last_index;
+ int next_index;
start_index = __stop___trace_bprintk_fmt - __start___trace_bprintk_fmt;
@@ -307,25 +369,19 @@ static const char **find_next(void *v, loff_t *pos)
return __start___trace_bprintk_fmt + *pos;
/*
- * The __tracepoint_str section is treated the same as the
- * __trace_printk_fmt section. The difference is that the
- * __trace_printk_fmt section should only be used by trace_printk()
- * in a debugging environment, as if anything exists in that section
- * the trace_prink() helper buffers are allocated, which would just
- * waste space in a production environment.
- *
- * The __tracepoint_str sections on the other hand are used by
- * tracepoints which need to map pointers to their strings to
- * the ASCII text for userspace.
+ * Built-in __tracepoint_str entries are exported directly from the
+ * core section. Module tracepoint_string() mappings are kept on a
+ * separate tracing-managed list below, because their lifetime is tied
+ * to module load/unload and differs from module trace_printk() formats.
*/
- last_index = start_index;
+ next_index = start_index;
start_index = __stop___tracepoint_str - __start___tracepoint_str;
- if (*pos < last_index + start_index)
- return __start___tracepoint_str + (*pos - last_index);
+ if (*pos < next_index + start_index)
+ return __start___tracepoint_str + (*pos - next_index);
- start_index += last_index;
- return find_next_mod_format(start_index, v, fmt, pos);
+ start_index += next_index;
+ return find_next_mod_entry(start_index, v, pos);
}
static void *
--
2.39.5 (Apple Git-154)
^ permalink raw reply related
* Re: [PATCH v2] tracing: preserve module tracepoint strings
From: Petr Pavlu @ 2026-04-13 9:40 UTC (permalink / raw)
To: Cao Ruichuang
Cc: rostedt, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel
In-Reply-To: <20260410051847.73259-1-create0818@163.com>
On 4/10/26 7:18 AM, Cao Ruichuang wrote:
> tracepoint_string() is documented as exporting constant strings
> through printk_formats, including when it is used from modules.
> That currently does not work.
>
> A small test module that calls
> tracepoint_string("tracepoint_string_test_module_string") loads
> successfully and gets a pointer back, but the string never appears
> in /sys/kernel/tracing/printk_formats. The loader only collects
> __trace_printk_fmt from modules and ignores __tracepoint_str.
>
> Collect module __tracepoint_str entries too, copy them to stable
> tracing-managed storage like module trace_printk formats, and let
> trace_is_tracepoint_string() recognize those copied strings. This
> makes module tracepoint strings visible through printk_formats and
> keeps them accepted by the trace string safety checks.
>
> Update the tracepoint_string() documentation to describe this
> module behavior explicitly, so the comment matches the preserved
> module-string mappings exported by tracing.
>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217196
> Signed-off-by: Cao Ruichuang <create0818@163.com>
> ---
> v2:
> - update tracepoint_string() documentation to describe the preserved
> module-string mapping explicitly
> - address Petr Pavlu's review about the comment not matching the
> implemented module behavior
I questioned in my previous comment whether the data associated with
tracepoint_string() could be dropped when the module that created it is
unloaded. Typically, modules should not leave any data behind when they
are removed. Note how kernel/trace/trace_events.c tracks event fields
using add_str_to_module() and releases them in
trace_module_remove_events(). In practice, I suppose this isn't a large
problem because the usage of tracepoint_string() is limited and one
won't typically load/unload different modules that use this facility.
Nonetheless, what is the reason for keeping the tracepoint_string()
data for unloaded modules?
--
Thanks,
Petr
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox