* [PATCH v7] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-14 2:55 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by modifying
funcgraph-retaddr to adopt the same implementation approach as
funcgraph-args, specifically by storing the return address in the
first entry of the args array.
As a result, both features now coexist seamlessly and function as
intended.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
v7:
- Remove nr_args to eliminate a multiplication instruction
- Link to v6: https://lore.kernel.org/all/20251114023453.2061590-1-dolinux.peng@gmail.com/
v6:
- Initialize retaddr to NULL to silence uninitialized variable warning.
- Use ARGS_OFFS(size) macro instead of args_offs variable to eliminate
"set but not used" warning
- Link to v5: https://lore.kernel.org/all/20251113072938.333657-1-dolinux.peng@gmail.com/
v5:
- Avoid unnecessary branch overhead by first verifying
CONFIG_FUNCTION_GRAPH_RETADDR is enabled prior to testing
the TRACE_GRAPH_PRINT_RETADDR flag
- Link to v4: https://lore.kernel.org/all/20251112093650.3345108-1-dolinux.peng@gmail.com/
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
- Resend to add missing add Acked-by tags
- Link to v3: https://lore.kernel.org/all/20251112034333.2901601-1-dolinux.peng@gmail.com/
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
- Link to v2: https://lore.kernel.org/all/20251111135432.2143993-1-dolinux.peng@gmail.com/
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
- Link to v1: https://lore.kernel.org/all/20251011164156.3678012-1-dolinux.peng@gmail.com/
---
include/linux/ftrace.h | 11 ---
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 140 ++++++++++++---------------
4 files changed, 64 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..18babcd66227 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,38 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr = 0;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+
+ if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
+ tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ size += sizeof(long);
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (!!fregs)
+ size += FTRACE_REGS_MAX_ARGS * sizeof(long);
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT)
+ entry->args[0] = retaddr;
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
if (fregs) {
- for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++) {
+ entry->args[i + ARGS_OFFS(size)] =
+ ftrace_regs_get_argument(fregs, i);
+ }
}
#endif
@@ -158,38 +187,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +208,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +258,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +622,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +795,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +853,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +868,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +949,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1003,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1011,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1188,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1415,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* [PATCH v6] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-14 2:34 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
From: Donglin Peng <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by modifying
funcgraph-retaddr to adopt the same implementation approach as
funcgraph-args, specifically by storing the return address in the
first entry of the args array.
As a result, both features now coexist seamlessly and function as
intended.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Signed-off-by: Donglin Peng <pengdonglin@xiaomi.com>
Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
---
v6:
- Initialize retaddr to NULL to silence uninitialized variable warning.
- Use ARGS_OFFS(size) macro instead of args_offs variable to eliminate
"set but not used" warning
- Link to v5: https://lore.kernel.org/all/20251113072938.333657-1-dolinux.peng@gmail.com/
v5:
- Avoid unnecessary branch overhead by first verifying
CONFIG_FUNCTION_GRAPH_RETADDR is enabled prior to testing
the TRACE_GRAPH_PRINT_RETADDR flag
- Link to v4: https://lore.kernel.org/all/20251112093650.3345108-1-dolinux.peng@gmail.com/
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
- Resend to add missing add Acked-by tags
- Link to v3: https://lore.kernel.org/all/20251112034333.2901601-1-dolinux.peng@gmail.com/
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
- Link to v2: https://lore.kernel.org/all/20251111135432.2143993-1-dolinux.peng@gmail.com/
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
- Link to v1: https://lore.kernel.org/all/20251011164156.3678012-1-dolinux.peng@gmail.com/
---
include/linux/ftrace.h | 11 ---
kernel/trace/trace.h | 4 -
kernel/trace/trace_entries.h | 6 +-
kernel/trace/trace_functions_graph.c | 142 ++++++++++++---------------
4 files changed, 66 insertions(+), 97 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..88cb54d73bdb 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1129,17 +1129,6 @@ struct ftrace_graph_ent {
int depth;
} __packed;
-/*
- * Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
- */
-struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
- unsigned long retaddr; /* Return address */
-} __packed;
-
/*
* Structure that defines a return function trace.
* It's already packed but the attribute "packed" is needed
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 85eabb454bee..9fac291b913a 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -955,10 +955,6 @@ extern void graph_trace_close(struct trace_iterator *iter);
extern int __trace_graph_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx);
-extern int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..593a74663c65 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __dynamic_array(unsigned long, args )
),
F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
- (void *)__entry->retaddr)
+ (void *)__entry->args[0])
);
#else
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index a7f4b9a47a71..31dfede72368 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -16,6 +16,15 @@
#include "trace.h"
#include "trace_output.h"
+#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
+#define HAVE_RETADDR 1
+#define ARGS_OFFS(args_size) \
+ ((args_size) > FTRACE_REGS_MAX_ARGS * sizeof(long) ? 1 : 0)
+#else
+#define HAVE_RETADDR 0
+#define ARGS_OFFS(args_size) 0
+#endif
+
/* When set, irq functions will be ignored */
static int ftrace_graph_skip_irqs;
@@ -27,21 +36,25 @@ struct fgraph_cpu_data {
unsigned long enter_funcs[FTRACE_RETFUNC_DEPTH];
};
+/*
+ * fgraph_retaddr_ent_entry and ftrace_graph_ent_entry share layout, ent
+ * member repurposed for storage
+ */
struct fgraph_ent_args {
struct ftrace_graph_ent_entry ent;
- /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
- unsigned long args[FTRACE_REGS_MAX_ARGS];
+ /*
+ * Force the sizeof of args[] to have (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR)
+ * entries with the first entry storing the return address for
+ * TRACE_GRAPH_RETADDR_ENT.
+ */
+ unsigned long args[FTRACE_REGS_MAX_ARGS + HAVE_RETADDR];
};
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
- union {
- struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
- };
+ struct fgraph_ent_args ent;
struct ftrace_graph_ret_entry ret;
int failed;
int cpu;
@@ -127,22 +140,40 @@ static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct ftrace_graph_ent_entry *entry;
- int size;
+ unsigned long retaddr = 0;
+ int size = sizeof(*entry);
+ int type = TRACE_GRAPH_ENT;
+ int nr_args = 0;
+
+ if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
+ tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
+ retaddr = ftrace_graph_top_ret_addr(current);
+ type = TRACE_GRAPH_RETADDR_ENT;
+ nr_args += 1;
+ }
/* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
- size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
+ if (!!fregs)
+ nr_args += FTRACE_REGS_MAX_ARGS;
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_ENT, size, trace_ctx);
+ size += nr_args * sizeof(long);
+ event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
entry->graph_ent = *trace;
+ /* Store the retaddr in args[0] */
+ if (type == TRACE_GRAPH_RETADDR_ENT)
+ entry->args[0] = retaddr;
+
#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
if (fregs) {
- for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
- entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++) {
+ entry->args[i + ARGS_OFFS(size)] =
+ ftrace_regs_get_argument(fregs, i);
+ }
}
#endif
@@ -158,38 +189,6 @@ int __trace_graph_entry(struct trace_array *tr,
return __graph_entry(tr, trace, trace_ctx, NULL);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- struct ring_buffer_event *event;
- struct trace_buffer *buffer = tr->array_buffer.buffer;
- struct fgraph_retaddr_ent_entry *entry;
-
- event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
- if (!event)
- return 0;
- entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
- trace_buffer_unlock_commit_nostack(buffer, event);
-
- return 1;
-}
-#else
-int __trace_graph_retaddr_entry(struct trace_array *tr,
- struct ftrace_graph_ent *trace,
- unsigned int trace_ctx,
- unsigned long retaddr)
-{
- return 1;
-}
-#endif
-
static inline int ftrace_graph_ignore_irqs(void)
{
if (!ftrace_graph_skip_irqs || trace_recursion_test(TRACE_IRQ_BIT))
@@ -211,7 +210,6 @@ static int graph_entry(struct ftrace_graph_ent *trace,
struct trace_array *tr = gops->private;
struct fgraph_times *ftimes;
unsigned int trace_ctx;
- int ret = 0;
if (*task_var & TRACE_GRAPH_NOTRACE)
return 0;
@@ -262,15 +260,7 @@ static int graph_entry(struct ftrace_graph_ent *trace,
return 1;
trace_ctx = tracing_gen_ctx();
- if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
- tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
- unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
- } else {
- ret = __graph_entry(tr, trace, trace_ctx, fregs);
- }
-
- return ret;
+ return __graph_entry(tr, trace, trace_ctx, fregs);
}
int trace_graph_entry(struct ftrace_graph_ent *trace,
@@ -634,13 +624,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->ent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->ent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -811,21 +797,21 @@ print_graph_duration(struct trace_array *tr, unsigned long long duration,
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
#define __TRACE_GRAPH_PRINT_RETADDR TRACE_GRAPH_PRINT_RETADDR
-static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_entry *entry,
- u32 trace_flags, bool comment)
+static void print_graph_retaddr(struct trace_seq *s, unsigned long retaddr, u32 trace_flags,
+ bool comment)
{
if (comment)
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym(s, entry->graph_ent.retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
+ seq_print_ip_sym(s, retaddr, trace_flags | TRACE_ITER_SYM_OFFSET);
if (comment)
trace_seq_puts(s, " */");
}
#else
#define __TRACE_GRAPH_PRINT_RETADDR 0
-#define print_graph_retaddr(_seq, _entry, _tflags, _comment) do { } while (0)
+#define print_graph_retaddr(_seq, _retaddr, _tflags, _comment) do { } while (0)
#endif
#if defined(CONFIG_FUNCTION_GRAPH_RETVAL) || defined(CONFIG_FUNCTION_GRAPH_RETADDR)
@@ -869,10 +855,12 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
trace_seq_printf(s, "%ps", func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, (unsigned long)func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ (unsigned long)func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
if (print_retval || print_retaddr)
trace_seq_puts(s, " /*");
@@ -882,8 +870,7 @@ static void print_graph_retval(struct trace_seq *s, struct ftrace_graph_ent_entr
}
if (print_retaddr)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- trace_flags, false);
+ print_graph_retaddr(s, entry->args[0], trace_flags, false);
if (print_retval) {
if (hex_format || (err_code == 0))
@@ -964,10 +951,12 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size),
+ ret_func);
trace_seq_putc(s, ';');
- } else
+ } else {
trace_seq_puts(s, "();");
+ }
}
trace_seq_putc(s, '\n');
@@ -1016,7 +1005,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, entry->args + ARGS_OFFS(args_size), func);
else
trace_seq_puts(s, "()");
@@ -1024,8 +1013,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
if (flags & __TRACE_GRAPH_PRINT_RETADDR &&
entry->ent.type == TRACE_GRAPH_RETADDR_ENT)
- print_graph_retaddr(s, (struct fgraph_retaddr_ent_entry *)entry,
- tr->trace_flags, true);
+ print_graph_retaddr(s, entry->args[0], tr->trace_flags, true);
trace_seq_putc(s, '\n');
if (trace_seq_has_overflowed(s))
@@ -1202,7 +1190,7 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
* it can be safely saved at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ u8 save_buf[sizeof(*entry) + (FTRACE_REGS_MAX_ARGS + HAVE_RETADDR) * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1429,16 +1417,12 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
trace_assign_type(field, entry);
return print_graph_entry(field, s, iter, flags);
}
-#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((typeof(field))rfield, s, iter, flags);
}
-#endif
case TRACE_GRAPH_RET: {
struct ftrace_graph_ret_entry *field;
trace_assign_type(field, entry);
--
2.34.1
^ permalink raw reply related
* RE: [PATCH v3 19/21] scsi: fnic: Switch to use %ptSp
From: Karan Tilak Kumar (kartilak) @ 2025-11-13 22:34 UTC (permalink / raw)
To: Andy Shevchenko, Corey Minyard, Christian König,
Dr. David Alan Gilbert, Alex Deucher, Thomas Zimmermann,
Dmitry Baryshkov, Rob Clark, Matthew Brost, Ulf Hansson,
Aleksandr Loktionov, Vitaly Lifshits, Manivannan Sadhasivam,
Niklas Cassel, Calvin Owens, Vadim Fedorenko, Sagi Maimon,
Martin K. Petersen, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
openipmi-developer@lists.sourceforge.net,
linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
linaro-mm-sig@lists.linaro.org, amd-gfx@lists.freedesktop.org,
linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org,
intel-xe@lists.freedesktop.org, linux-mmc@vger.kernel.org,
netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
linux-pci@vger.kernel.org, linux-s390@vger.kernel.org,
linux-scsi@vger.kernel.org, linux-staging@lists.linux.dev,
ceph-devel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat (satishkh), Sesidhar Baddela (sebaddel),
James E.J. Bottomley, Mauro Carvalho Chehab, Greg Kroah-Hartman,
Xiubo Li, Ilya Dryomov, Masami Hiramatsu, Mathieu Desnoyers,
Andrew Morton
In-Reply-To: <20251113150217.3030010-20-andriy.shevchenko@linux.intel.com>
On Thursday, November 13, 2025 6:33 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
> Use %ptSp instead of open coded variants to print content of
> struct timespec64 in human readable format.
>
> Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/scsi/fnic/fnic_trace.c | 52 ++++++++++++++--------------------
> 1 file changed, 22 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c
> index cdc6b12b1ec2..0a849a195a8e 100644
> --- a/drivers/scsi/fnic/fnic_trace.c
> +++ b/drivers/scsi/fnic/fnic_trace.c
> @@ -138,9 +138,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
> */
> len += scnprintf(fnic_dbgfs_prt->buffer + len,
> (trace_max_pages * PAGE_SIZE * 3) - len,
> - "%16llu.%09lu %-50s %8x %8x %16llx %16llx "
> - "%16llx %16llx %16llx\n", (u64)val.tv_sec,
> - val.tv_nsec, str, tbp->host_no, tbp->tag,
> + "%ptSp %-50s %8x %8x %16llx %16llx %16llx %16llx %16llx\n",
> + &val, str, tbp->host_no, tbp->tag,
> tbp->data[0], tbp->data[1], tbp->data[2],
> tbp->data[3], tbp->data[4]);
> rd_idx++;
> @@ -180,9 +179,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
> */
> len += scnprintf(fnic_dbgfs_prt->buffer + len,
> (trace_max_pages * PAGE_SIZE * 3) - len,
> - "%16llu.%09lu %-50s %8x %8x %16llx %16llx "
> - "%16llx %16llx %16llx\n", (u64)val.tv_sec,
> - val.tv_nsec, str, tbp->host_no, tbp->tag,
> + "%ptSp %-50s %8x %8x %16llx %16llx %16llx %16llx %16llx\n",
> + &val, str, tbp->host_no, tbp->tag,
> tbp->data[0], tbp->data[1], tbp->data[2],
> tbp->data[3], tbp->data[4]);
> rd_idx++;
> @@ -215,30 +213,26 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
> {
> int len = 0;
> int buf_size = debug->buf_size;
> - struct timespec64 val1, val2;
> + struct timespec64 val, val1, val2;
> int i = 0;
>
> - ktime_get_real_ts64(&val1);
> + ktime_get_real_ts64(&val);
> len = scnprintf(debug->debug_buffer + len, buf_size - len,
> "------------------------------------------\n"
> "\t\tTime\n"
> "------------------------------------------\n");
>
> + val1 = timespec64_sub(val, stats->stats_timestamps.last_reset_time);
> + val2 = timespec64_sub(val, stats->stats_timestamps.last_read_time);
> len += scnprintf(debug->debug_buffer + len, buf_size - len,
> - "Current time : [%lld:%ld]\n"
> - "Last stats reset time: [%lld:%09ld]\n"
> - "Last stats read time: [%lld:%ld]\n"
> - "delta since last reset: [%lld:%ld]\n"
> - "delta since last read: [%lld:%ld]\n",
> - (s64)val1.tv_sec, val1.tv_nsec,
> - (s64)stats->stats_timestamps.last_reset_time.tv_sec,
> - stats->stats_timestamps.last_reset_time.tv_nsec,
> - (s64)stats->stats_timestamps.last_read_time.tv_sec,
> - stats->stats_timestamps.last_read_time.tv_nsec,
> - (s64)timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_sec,
> - timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_nsec,
> - (s64)timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_sec,
> - timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_nsec);
> + "Current time : [%ptSp]\n"
> + "Last stats reset time: [%ptSp]\n"
> + "Last stats read time: [%ptSp]\n"
> + "delta since last reset: [%ptSp]\n"
> + "delta since last read: [%ptSp]\n",
> + &val,
> + &stats->stats_timestamps.last_reset_time, &val1,
> + &stats->stats_timestamps.last_read_time, &val2);
>
> stats->stats_timestamps.last_read_time = val1;
>
> @@ -416,8 +410,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
> jiffies_to_timespec64(stats->misc_stats.last_ack_time, &val2);
>
> len += scnprintf(debug->debug_buffer + len, buf_size - len,
> - "Last ISR time: %llu (%8llu.%09lu)\n"
> - "Last ACK time: %llu (%8llu.%09lu)\n"
> + "Last ISR time: %llu (%ptSp)\n"
> + "Last ACK time: %llu (%ptSp)\n"
> "Max ISR jiffies: %llu\n"
> "Max ISR time (ms) (0 denotes < 1 ms): %llu\n"
> "Corr. work done: %llu\n"
> @@ -437,10 +431,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
> "Number of rport not ready: %lld\n"
> "Number of receive frame errors: %lld\n"
> "Port speed (in Mbps): %lld\n",
> - (u64)stats->misc_stats.last_isr_time,
> - (s64)val1.tv_sec, val1.tv_nsec,
> - (u64)stats->misc_stats.last_ack_time,
> - (s64)val2.tv_sec, val2.tv_nsec,
> + (u64)stats->misc_stats.last_isr_time, &val1,
> + (u64)stats->misc_stats.last_ack_time, &val2,
> (u64)atomic64_read(&stats->misc_stats.max_isr_jiffies),
> (u64)atomic64_read(&stats->misc_stats.max_isr_time_ms),
> (u64)atomic64_read(&stats->misc_stats.corr_work_done),
> @@ -857,8 +849,8 @@ void copy_and_format_trace_data(struct fc_trace_hdr *tdata,
> len = *orig_len;
>
> len += scnprintf(fnic_dbgfs_prt->buffer + len, max_size - len,
> - "%ptTs.%09lu ns%8x %c%8x\t",
> - &tdata->time_stamp.tv_sec, tdata->time_stamp.tv_nsec,
> + "%ptSs ns%8x %c%8x\t",
> + &tdata->time_stamp,
> tdata->host_no, tdata->frame_type, tdata->frame_len);
>
> fc_trace = (char *)FC_TRACE_ADDRESS(tdata);
> --
> 2.50.1
>
>
Acked-by: Karan Tilak Kumar <kartilak@cisco.com>
Thanks for the change, Andy.
Can you please advise how I can compile test this change?
Regards,
Karan
^ permalink raw reply
* Re: [PATCH v16 4/4] perf tools: Merge deferred user callchains
From: Namhyung Kim @ 2025-11-13 21:42 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Steven Rostedt, Steven Rostedt, linux-kernel, linux-trace-kernel,
bpf, x86, Masami Hiramatsu, Mathieu Desnoyers, Josh Poimboeuf,
Ingo Molnar, Jiri Olsa, Arnaldo Carvalho de Melo, Thomas Gleixner,
Andrii Nakryiko, Indu Bhagat, Jose E. Marchesi, Beau Belgrave,
Jens Remus, Linus Torvalds, Andrew Morton, Florian Weimer,
Sam James, Kees Cook, Carlos O'Donell
In-Reply-To: <20251024130203.GC3245006@noisy.programming.kicks-ass.net>
Hello,
Sorry for the delay. And I'm happy that the kernel part is merge to the
tip tree! :)
On Fri, Oct 24, 2025 at 03:02:03PM +0200, Peter Zijlstra wrote:
> On Thu, Oct 02, 2025 at 01:49:38PM -0400, Steven Rostedt wrote:
> > On Mon, 08 Sep 2025 13:53:23 -0400
> > Steven Rostedt <rostedt@kernel.org> wrote:
> >
> > > +static int evlist__deliver_deferred_samples(struct evlist *evlist,
> > > + const struct perf_tool *tool,
> > > + union perf_event *event,
> > > + struct perf_sample *sample,
> > > + struct machine *machine)
> > > +{
> > > + struct deferred_event *de, *tmp;
> > > + struct evsel *evsel;
> > > + int ret = 0;
> > > +
> > > + if (!tool->merge_deferred_callchains) {
> > > + evsel = evlist__id2evsel(evlist, sample->id);
> > > + return tool->callchain_deferred(tool, event, sample,
> > > + evsel, machine);
> > > + }
> > > +
> > > + list_for_each_entry_safe(de, tmp, &evlist->deferred_samples, list) {
> > > + struct perf_sample orig_sample;
> >
> > orig_sample is not initialized and can then contain junk.
Yep.
> >
> > > +
> > > + ret = evlist__parse_sample(evlist, de->event, &orig_sample);
But here you call evlist__parse_sample() and evsel__parse_sample() which
should initialize the sample properly.
> > > + if (ret < 0) {
> > > + pr_err("failed to parse original sample\n");
> > > + break;
> > > + }
> > > +
> > > + if (sample->tid != orig_sample.tid)
> > > + continue;
> > > +
> > > + if (event->callchain_deferred.cookie == orig_sample.deferred_cookie)
> > > + sample__merge_deferred_callchain(&orig_sample, sample);
> >
> > The sample__merge_deferred_callchain() initializes both
> > orig_sample.deferred_callchain and the callchain. But now that it's not
> > being called, it can cause the below free to happen with junk as the
> > callchain. This needs:
> >
> > else
> > orig_sample.deferred_callchain = false;
>
> Ah, so I saw crashes from here and just deleted both free()s and got on
> with things ;-)
I don't understand how it can have the garbage. But having the else
part would be safer.
Thanks,
Namhyung
> > > +
> > > + evsel = evlist__id2evsel(evlist, orig_sample.id);
> > > + ret = evlist__deliver_sample(evlist, tool, de->event,
> > > + &orig_sample, evsel,> machine); +
> > > + if (orig_sample.deferred_callchain)
> > > + free(orig_sample.callchain);
> > > +
> > > + list_del(&de->list);
> > > + free(de);
> > > +
> > > + if (ret)
> > > + break;
> > > + }
> > > + return ret;
> > > +}
> >
> > -- Steve
^ permalink raw reply
* Re: [PATCH v5] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: kernel test robot @ 2025-11-13 18:33 UTC (permalink / raw)
To: Donglin Peng, rostedt
Cc: llvm, oe-kbuild-all, linux-trace-kernel, linux-kernel,
Donglin Peng, Sven Schnelle, Masami Hiramatsu
In-Reply-To: <20251113072938.333657-1-dolinux.peng@gmail.com>
Hi Donglin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.18-rc5]
[also build test WARNING on linus/master]
[cannot apply to trace/for-next next-20251113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Donglin-Peng/function_graph-Enable-funcgraph-args-and-funcgraph-retaddr-to-work-simultaneously/20251113-153332
base: v6.18-rc5
patch link: https://lore.kernel.org/r/20251113072938.333657-1-dolinux.peng%40gmail.com
patch subject: [PATCH v5] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
config: sparc64-randconfig-001-20251113 (https://download.01.org/0day-ci/archive/20251114/202511140224.loIXI9Us-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251114/202511140224.loIXI9Us-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511140224.loIXI9Us-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/trace/trace_functions_graph.c:146:19: warning: variable 'args_offs' set but not used [-Wunused-but-set-variable]
146 | int nr_args = 0, args_offs = 0;
| ^
kernel/trace/trace_functions_graph.c:169:20: warning: variable 'retaddr' is uninitialized when used here [-Wuninitialized]
169 | entry->args[0] = retaddr;
| ^~~~~~~
kernel/trace/trace_functions_graph.c:143:23: note: initialize the variable 'retaddr' to silence this warning
143 | unsigned long retaddr;
| ^
| = 0
2 warnings generated.
vim +/args_offs +146 kernel/trace/trace_functions_graph.c
132
133 static void
134 print_graph_duration(struct trace_array *tr, unsigned long long duration,
135 struct trace_seq *s, u32 flags);
136
137 static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
138 unsigned int trace_ctx, struct ftrace_regs *fregs)
139 {
140 struct ring_buffer_event *event;
141 struct trace_buffer *buffer = tr->array_buffer.buffer;
142 struct ftrace_graph_ent_entry *entry;
143 unsigned long retaddr;
144 int size = sizeof(*entry);
145 int type = TRACE_GRAPH_ENT;
> 146 int nr_args = 0, args_offs = 0;
147
148 if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
149 tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
150 retaddr = ftrace_graph_top_ret_addr(current);
151 type = TRACE_GRAPH_RETADDR_ENT;
152 nr_args += 1;
153 }
154
155 /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
156 if (!!fregs)
157 nr_args += FTRACE_REGS_MAX_ARGS;
158
159 size += nr_args * sizeof(long);
160 event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
161 if (!event)
162 return 0;
163
164 entry = ring_buffer_event_data(event);
165 entry->graph_ent = *trace;
166
167 /* Store the retaddr in args[0] */
168 if (type == TRACE_GRAPH_RETADDR_ENT) {
169 entry->args[0] = retaddr;
170 args_offs += 1;
171 }
172
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
From: Alexei Starovoitov @ 2025-11-13 17:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: bot+bpf-ci, rostedt, Florent Revest, Mark Rutland, bpf, LKML,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu,
Martin KaFai Lau, Eduard, Yonghong Song, Chris Mason,
Ihor Solodrai
In-Reply-To: <aRYAhDqGsOHZzTL-@krava>
On Thu, Nov 13, 2025 at 8:00 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Nov 13, 2025 at 01:02:17PM +0000, bot+bpf-ci@kernel.org wrote:
> > > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > > index 433c36c3a..bacb6d9ab 100644
> > > --- a/include/linux/ftrace.h
> > > +++ b/include/linux/ftrace.h
> > > @@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> > >
> > > int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > > int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
> > >
> > > void ftrace_stub_direct_tramp(void);
> > >
> > > @@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > > return -ENODEV;
> > > }
> > >
> > > +int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > The stub function name doesn't match the declaration. Should this be
> > update_ftrace_direct_mod() instead of modify_ftrace_direct_hash()?
>
> it should, my bad
>
> >
> > > +{
> > > + return -ENODEV;
> > > +}
> > > +
> >
> > [ ... ]
> >
> > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > > index 345f3fdd2..627d05b57 100644
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -6445,6 +6445,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > > return err;
> > > }
> > >
> > > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > > +{
> > > + struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
> > > + struct ftrace_func_entry *entry, *tmp;
> > > + static struct ftrace_ops tmp_ops = {
> > > + .func = ftrace_stub,
> > > + .flags = FTRACE_OPS_FL_STUB,
> > > + };
> > > + unsigned long size, i;
> > > + int err;
> > > +
> > > + if (!hash_count(hash))
> > > + return 0;
> > > + if (check_direct_multi(ops))
> > > + return -EINVAL;
> > > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > > + return -EINVAL;
> > > + if (direct_functions == EMPTY_HASH)
> > > + return -EINVAL;
> > > +
> > > + if (do_direct_lock)
> > > + mutex_lock(&direct_mutex);
> > > +
> > > + /* Enable the tmp_ops to have the same functions as the direct ops */
> > > + ftrace_ops_init(&tmp_ops);
> > > + tmp_ops.func_hash = ops->func_hash;
> > > +
> > > + err = register_ftrace_function_nolock(&tmp_ops);
> > > + if (err)
> > > + goto unlock;
> > > +
> > > + /*
> > > + * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > > + * ops->ops_func for the ops. This is needed because the above
> > > + * register_ftrace_function_nolock() worked on tmp_ops.
> > > + */
> > > + err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
> > ^^^^
> > Does __ftrace_hash_update_ipmodify() accept a fourth argument? The
> > function signature shows it only takes three parameters (ops, old_hash,
> > new_hash). This looks like a compilation error.
>
> the whole patchset is based on bpf-next/master plus Song's livepatch
> fixes which change modify_ftrace_direct_hash function, it's mentioned
> in the cover letter
Ohh. Will send bpf PR to Linus today and merge into bpf-next afterwards.
^ permalink raw reply
* Re: [PATCH v5] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: kernel test robot @ 2025-11-13 17:41 UTC (permalink / raw)
To: Donglin Peng, rostedt
Cc: llvm, oe-kbuild-all, linux-trace-kernel, linux-kernel,
Donglin Peng, Sven Schnelle, Masami Hiramatsu
In-Reply-To: <20251113072938.333657-1-dolinux.peng@gmail.com>
Hi Donglin,
kernel test robot noticed the following build warnings:
[auto build test WARNING on v6.18-rc5]
[also build test WARNING on linus/master]
[cannot apply to trace/for-next next-20251113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Donglin-Peng/function_graph-Enable-funcgraph-args-and-funcgraph-retaddr-to-work-simultaneously/20251113-153332
base: v6.18-rc5
patch link: https://lore.kernel.org/r/20251113072938.333657-1-dolinux.peng%40gmail.com
patch subject: [PATCH v5] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
config: x86_64-kexec (https://download.01.org/0day-ci/archive/20251114/202511140111.X1AImPQE-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251114/202511140111.X1AImPQE-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511140111.X1AImPQE-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> kernel/trace/trace_functions_graph.c:169:20: warning: variable 'retaddr' is uninitialized when used here [-Wuninitialized]
169 | entry->args[0] = retaddr;
| ^~~~~~~
kernel/trace/trace_functions_graph.c:143:23: note: initialize the variable 'retaddr' to silence this warning
143 | unsigned long retaddr;
| ^
| = 0
1 warning generated.
vim +/retaddr +169 kernel/trace/trace_functions_graph.c
132
133 static void
134 print_graph_duration(struct trace_array *tr, unsigned long long duration,
135 struct trace_seq *s, u32 flags);
136
137 static int __graph_entry(struct trace_array *tr, struct ftrace_graph_ent *trace,
138 unsigned int trace_ctx, struct ftrace_regs *fregs)
139 {
140 struct ring_buffer_event *event;
141 struct trace_buffer *buffer = tr->array_buffer.buffer;
142 struct ftrace_graph_ent_entry *entry;
143 unsigned long retaddr;
144 int size = sizeof(*entry);
145 int type = TRACE_GRAPH_ENT;
146 int nr_args = 0, args_offs = 0;
147
148 if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
149 tracer_flags_is_set(TRACE_GRAPH_PRINT_RETADDR)) {
150 retaddr = ftrace_graph_top_ret_addr(current);
151 type = TRACE_GRAPH_RETADDR_ENT;
152 nr_args += 1;
153 }
154
155 /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
156 if (!!fregs)
157 nr_args += FTRACE_REGS_MAX_ARGS;
158
159 size += nr_args * sizeof(long);
160 event = trace_buffer_lock_reserve(buffer, type, size, trace_ctx);
161 if (!event)
162 return 0;
163
164 entry = ring_buffer_event_data(event);
165 entry->graph_ent = *trace;
166
167 /* Store the retaddr in args[0] */
168 if (type == TRACE_GRAPH_RETADDR_ENT) {
> 169 entry->args[0] = retaddr;
170 args_offs += 1;
171 }
172
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Steven Rostedt @ 2025-11-13 16:14 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113160739.DUvh9i_o@linutronix.de>
On Thu, 13 Nov 2025 17:07:39 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2025-11-13 10:51:06 [-0500], Steven Rostedt wrote:
> > Yes, because they are only tested in sched_switch and fork and exit tracepoints.
> >
> > Although, this was written when tracepoint callbacks were always called
> > under preempt disable. Perhaps we need to change that call to:
> >
> > tracepoint_synchronize_unregister()
> >
> > Or add a preempt_disable() around the callers.
>
> Please don't. Please do a regular rcu_read_lock() ;)
> I tried to get rid of the preempt_disable() around tracepoints so that
> the attached BPF callbacks are not invoked with disabled preemption. I
> haven't followed up here in a while but I think Paul's SRCU work goes
> in the right direction.
I meant just reading the pid lists, which are usually called from
tracepoints that are in preempt_disabled locations.
Anyway, I can add rcu_read_lock() around the callers of it.
>
> > I'm very nervous about using RCU here. It will add a lot more corner cases
> > that needs to be accounted for. The complexity doesn't appear to be worth
> > it. I'd rather just keep the raw spin locks than to convert it to RCU.
> >
> > The seqcount makes sense to me. It's simple and keeps the same paradigm as
> > what we have. What's wrong with using it?
>
> I'm fine with it once you explained under what conditions retry can
> happen. Thank you.
Thanks,
-- Steve
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Sebastian Andrzej Siewior @ 2025-11-13 16:07 UTC (permalink / raw)
To: Steven Rostedt
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113105106.4270f6ac@gandalf.local.home>
On 2025-11-13 10:51:06 [-0500], Steven Rostedt wrote:
> Yes, because they are only tested in sched_switch and fork and exit tracepoints.
>
> Although, this was written when tracepoint callbacks were always called
> under preempt disable. Perhaps we need to change that call to:
>
> tracepoint_synchronize_unregister()
>
> Or add a preempt_disable() around the callers.
Please don't. Please do a regular rcu_read_lock() ;)
I tried to get rid of the preempt_disable() around tracepoints so that
the attached BPF callbacks are not invoked with disabled preemption. I
haven't followed up here in a while but I think Paul's SRCU work goes
in the right direction.
> I'm very nervous about using RCU here. It will add a lot more corner cases
> that needs to be accounted for. The complexity doesn't appear to be worth
> it. I'd rather just keep the raw spin locks than to convert it to RCU.
>
> The seqcount makes sense to me. It's simple and keeps the same paradigm as
> what we have. What's wrong with using it?
I'm fine with it once you explained under what conditions retry can
happen. Thank you.
> -- Steve
Sebastian
^ permalink raw reply
* Re: [PATCHv2 bpf-next 4/8] ftrace: Add update_ftrace_direct_del function
From: Jiri Olsa @ 2025-11-13 16:00 UTC (permalink / raw)
To: bot+bpf-ci
Cc: rostedt, revest, mark.rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, ast, daniel, andrii,
menglong8.dong, song, martin.lau, eddyz87, yonghong.song, clm,
ihor.solodrai
In-Reply-To: <367c6cc9ffa12937ce18f37fc1271e3d17a3f7aa6ea6d1152cc8a41539994865@mail.kernel.org>
On Thu, Nov 13, 2025 at 01:02:21PM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index ded3a306a..433c36c3a 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -543,6 +543,7 @@ int modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr);
> > int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> >
> > int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > +int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> >
> > void ftrace_stub_direct_tramp(void);
>
> [ ... ]
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 311ce6406..345f3fdd2 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
>
> [ ... ]
>
> > +int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > +{
> > + struct ftrace_hash *new_hash = NULL, *filter_hash = NULL, *free_hash = NULL;
> > + struct ftrace_func_entry *del, *entry;
> > + unsigned long size, i;
> > + int err = -EINVAL;
> > +
> > + if (!hash_count(hash))
> > + return 0;
> > + if (check_direct_multi(ops))
> > + return -EINVAL;
> > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > + return -EINVAL;
> > + if (direct_functions == EMPTY_HASH)
> > + return -EINVAL;
> > +
> > + mutex_lock(&direct_mutex);
> > +
> > + /* Make sure requested entries are already registered. */
> > + size = 1 << hash->size_bits;
> > + for (i = 0; i < size; i++) {
> > + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > + del = __ftrace_lookup_ip(direct_functions, entry->ip);
> > + if (!del || del->direct != entry->direct)
> > + goto out_unlock;
> > + }
> > + }
> > +
> > + err = -ENOMEM;
> > + filter_hash = hash_sub(ops->func_hash->filter_hash, hash);
> > + if (!filter_hash)
> > + goto out_unlock;
> > +
> > + new_hash = hash_sub(direct_functions, hash);
> > + if (!new_hash)
> > + goto out_unlock;
>
> Can this leak new_hash? If hash_sub() succeeds for both filter_hash
> and new_hash, but then either unregister_ftrace_function() or
> ftrace_update_ops() fails below, we goto out_unlock without freeing
> new_hash. The cleanup code only frees filter_hash:
>
> > +
> > + /* If there's nothing left, we need to unregister the ops. */
> > + if (ftrace_hash_empty(filter_hash)) {
> > + err = unregister_ftrace_function(ops);
> > + /* cleanup for possible another register call */
> > + ops->func = NULL;
> > + ops->trampoline = 0;
> > + ftrace_free_filter(ops);
> > + ops->func_hash->filter_hash = NULL;
> > + } else {
> > + err = ftrace_update_ops(ops, filter_hash, EMPTY_HASH);
> > + }
> > +
> > + if (!err) {
> > + free_hash = direct_functions;
> > + rcu_assign_pointer(direct_functions, new_hash);
> > + }
> > +
> > + out_unlock:
> > + mutex_unlock(&direct_mutex);
> > +
> > + if (free_hash && free_hash != EMPTY_HASH)
> > + call_rcu_tasks(&free_hash->rcu, register_ftrace_direct_cb);
> > + if (filter_hash)
> > + free_ftrace_hash(filter_hash);
> > +
> > + return err;
> > +}
>
> If err != 0, new_hash remains allocated but is never freed. Should the
> cleanup code add free_ftrace_hash(new_hash) when it's non-NULL?
yep, that's a miss, will fix
thanks,
jirka
^ permalink raw reply
* Re: [PATCHv2 bpf-next 5/8] ftrace: Add update_ftrace_direct_mod function
From: Jiri Olsa @ 2025-11-13 16:00 UTC (permalink / raw)
To: bot+bpf-ci
Cc: rostedt, revest, mark.rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, ast, daniel, andrii,
menglong8.dong, song, martin.lau, eddyz87, yonghong.song, clm,
ihor.solodrai
In-Reply-To: <c3260fe9e3d8ad79c75a6e8281f9fae5580beb3fcdd08e2015f417e11ec0a1b2@mail.kernel.org>
On Thu, Nov 13, 2025 at 01:02:17PM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 433c36c3a..bacb6d9ab 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -544,6 +544,7 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> >
> > int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash);
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock);
> >
> > void ftrace_stub_direct_tramp(void);
> >
> > @@ -581,6 +582,11 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > return -ENODEV;
> > }
> >
> > +int modify_ftrace_direct_hash(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> ^^^^^^^^^^^^^^^^^^^^^^^^^^
> The stub function name doesn't match the declaration. Should this be
> update_ftrace_direct_mod() instead of modify_ftrace_direct_hash()?
it should, my bad
>
> > +{
> > + return -ENODEV;
> > +}
> > +
>
> [ ... ]
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 345f3fdd2..627d05b57 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6445,6 +6445,74 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > return err;
> > }
> >
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > +{
> > + struct ftrace_hash *orig_hash = ops->func_hash->filter_hash;
> > + struct ftrace_func_entry *entry, *tmp;
> > + static struct ftrace_ops tmp_ops = {
> > + .func = ftrace_stub,
> > + .flags = FTRACE_OPS_FL_STUB,
> > + };
> > + unsigned long size, i;
> > + int err;
> > +
> > + if (!hash_count(hash))
> > + return 0;
> > + if (check_direct_multi(ops))
> > + return -EINVAL;
> > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > + return -EINVAL;
> > + if (direct_functions == EMPTY_HASH)
> > + return -EINVAL;
> > +
> > + if (do_direct_lock)
> > + mutex_lock(&direct_mutex);
> > +
> > + /* Enable the tmp_ops to have the same functions as the direct ops */
> > + ftrace_ops_init(&tmp_ops);
> > + tmp_ops.func_hash = ops->func_hash;
> > +
> > + err = register_ftrace_function_nolock(&tmp_ops);
> > + if (err)
> > + goto unlock;
> > +
> > + /*
> > + * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > + * ops->ops_func for the ops. This is needed because the above
> > + * register_ftrace_function_nolock() worked on tmp_ops.
> > + */
> > + err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
> ^^^^
> Does __ftrace_hash_update_ipmodify() accept a fourth argument? The
> function signature shows it only takes three parameters (ops, old_hash,
> new_hash). This looks like a compilation error.
the whole patchset is based on bpf-next/master plus Song's livepatch
fixes which change modify_ftrace_direct_hash function, it's mentioned
in the cover letter
>
> > + if (err)
> > + goto out;
>
> [ ... ]
>
> > +
> > +out:
> > + /* Removing the tmp_ops will add the updated direct callers to the functions */
> > + unregister_ftrace_function(&tmp_ops);
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Can this deadlock when do_direct_lock is true? Looking at the code:
>
> update_ftrace_direct_mod() with do_direct_lock=true
> -> mutex_lock(&direct_mutex)
> -> register_ftrace_function_nolock(&tmp_ops) succeeds
> -> __ftrace_hash_update_ipmodify() fails
> -> goto out (still holding direct_mutex)
> -> unregister_ftrace_function(&tmp_ops)
> -> cleanup_direct_functions_after_ipmodify()
> -> mutex_lock(&direct_mutex) /* deadlock */
>
> The unregister path tries to reacquire direct_mutex that we're already
> holding.
I don't think this can happen because tmp_ops does not have
FTRACE_OPS_FL_IPMODIFY flag set
thanks,
jirka
>
> > +
> > +unlock:
> > + if (do_direct_lock)
> > + mutex_unlock(&direct_mutex);
> > + return err;
> > +}
> > +
> > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19332026793
^ permalink raw reply
* Re: [PATCHv2 bpf-next 3/8] ftrace: Add update_ftrace_direct_add function
From: Jiri Olsa @ 2025-11-13 15:59 UTC (permalink / raw)
To: bot+bpf-ci
Cc: rostedt, revest, mark.rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, ast, daniel, andrii,
menglong8.dong, song, martin.lau, eddyz87, yonghong.song, clm,
ihor.solodrai
In-Reply-To: <c341189e76b2cf9bb2fe712c074bdd652f73edd5ea2f409655b2d78a62bd332f@mail.kernel.org>
On Thu, Nov 13, 2025 at 01:02:18PM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index e23e6a859..ded3a306a 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
>
> [ ... ]
>
> > @@ -568,6 +570,11 @@ static inline int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned l
> > return -ENODEV;
> > }
> >
> > +int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > +{
> > + return -ENODEV;
> > +}
> > +
> > /*
> > * This must be implemented by the architecture.
> > * It is the way the ftrace direct_ops helper, when called
>
> Should this stub function have static inline qualifiers? All the other
> stub functions in this section (ftrace_find_rec_direct,
> register_ftrace_direct, unregister_ftrace_direct, modify_ftrace_direct,
> and modify_ftrace_direct_nolock) use static inline. Without it, this
> could cause multiple definition linker errors when the header is
> included in multiple compilation units.
yep, also the other ones are missing it, will add.. thanks
jirka
>
>
> ---
> AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
> See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
>
> CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19332026793
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Steven Rostedt @ 2025-11-13 15:51 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113153508.9mYBNQlL@linutronix.de>
On Thu, 13 Nov 2025 16:35:08 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > A couple of issues. One, the chunks are fully used. There's no place to put
> > a "rcu_head" in them. Well, we may be able to make use of them.
>
> This could be the first (16?) bytes of the memory chunk.
Yes, but this will make things more complex.
>
> > Second, if there's a lot of tasks exiting and forking, we can easily run
> > out of chunks that are waiting to be "freed" via call_rcu().
>
> but this is a general RCU problem and not new here. The task_struct and
> everything around it (including stack) is RCU freed.
But this doesn't happen under memory pressure. Allocations are not
performed if a chunk is not available. If a chunk isn't available, then we
just stop tracing the filtered task. Hmm, it currently silently fails.
Perhaps I need to add a printk when this happens.
> > > This adding/ add-on-fork, removing and remove-on-exit is the only write
> > > side?
> >
> > That and manual writes to the set_ftrace_pid file.
>
> This looks like minimal. I miss understood then that context switch can
> also contribute to it.
sched_switch is only a reader, not a writer.
> And the callers of trace_pid_list_is_set() are always in the RCU read
> section then? I assume so, since it wouldn't make sense otherwise.
Yes, because they are only tested in sched_switch and fork and exit tracepoints.
Although, this was written when tracepoint callbacks were always called
under preempt disable. Perhaps we need to change that call to:
tracepoint_synchronize_unregister()
Or add a preempt_disable() around the callers.
I'm very nervous about using RCU here. It will add a lot more corner cases
that needs to be accounted for. The complexity doesn't appear to be worth
it. I'd rather just keep the raw spin locks than to convert it to RCU.
The seqcount makes sense to me. It's simple and keeps the same paradigm as
what we have. What's wrong with using it?
-- Steve
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Sebastian Andrzej Siewior @ 2025-11-13 15:35 UTC (permalink / raw)
To: Steven Rostedt
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113102445.3e70c1ec@gandalf.local.home>
On 2025-11-13 10:24:45 [-0500], Steven Rostedt wrote:
> On Thu, 13 Nov 2025 16:17:29 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
>
> > On 2025-11-13 10:05:24 [-0500], Steven Rostedt wrote:
> > > This means that the chunks are not being freed and we can't be doing
> > > synchronize_rcu() in every exit.
> >
> > You don't have to, you can do call_rcu().
>
> But the chunk isn't being freed. They may be used right away.
Not if you avoid using it until after the rcu callback.
> > > > So I *think* the RCU approach should be doable and cover this.
> > >
> > > Where would you put the synchronize_rcu()? In do_exit()?
> >
> > simply call_rcu() and let it move to the freelist.
>
> A couple of issues. One, the chunks are fully used. There's no place to put
> a "rcu_head" in them. Well, we may be able to make use of them.
This could be the first (16?) bytes of the memory chunk.
> Second, if there's a lot of tasks exiting and forking, we can easily run
> out of chunks that are waiting to be "freed" via call_rcu().
but this is a general RCU problem and not new here. The task_struct and
everything around it (including stack) is RCU freed.
> >
> > > Also understanding what this is used for helps in understanding the scope
> > > of protection needed.
> > >
> > > The pid_list is created when you add anything into one of the pid files in
> > > tracefs. Let's use /sys/kernel/tracing/set_ftrace_pid:
> > >
> > > # cd /sys/kernel/tracing
> > > # echo $$ > set_ftrace_pid
> > > # echo 1 > options/function-fork
> > > # cat set_ftrace_pid
> > > 2716
> > > 2936
> > > # cat set_ftrace_pid
> > > 2716
> > > 2945
> > >
> > > What the above did was to create a pid_list for the function tracer. I
> > > added the bash process pid using $$ (2716). Then when I cat the file, it
> > > showed the pid for the bash process as well as the pid for the cat process,
> > > as the cat process is a child of the bash process. The function-fork option
> > > means to add any child process to the set_ftrace_pid if the parent is
> > > already in the list. It also means to remove the pid if a process in the
> > > list exits.
> >
> > This adding/ add-on-fork, removing and remove-on-exit is the only write
> > side?
>
> That and manual writes to the set_ftrace_pid file.
This looks like minimal. I miss understood then that context switch can
also contribute to it.
> > > What we are protecting against is when one chunk is freed, but then
> > > allocated again for a different set of PIDs. Where the reader has the chunk,
> > > it was freed and re-allocated and the bit that is about to be checked
> > > doesn't represent the bit it is checking for.
> >
> > This I assumed.
> > And the kfree() at the end can not happen while there is still a reader?
>
> Correct. That's done by the pid_list user:
>
> In clear_ftrace_pids():
>
> /* Wait till all users are no longer using pid filtering */
> synchronize_rcu();
>
> if ((type & TRACE_PIDS) && pid_list)
> trace_pid_list_free(pid_list);
>
> if ((type & TRACE_NO_PIDS) && no_pid_list)
> trace_pid_list_free(no_pid_list);
And the callers of trace_pid_list_is_set() are always in the RCU read
section then? I assume so, since it wouldn't make sense otherwise.
> -- Steve
Sebastian
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Steven Rostedt @ 2025-11-13 15:35 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113073420.yko6jYcI@linutronix.de>
On Thu, 13 Nov 2025 08:34:20 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> > + do {
> > + seq = read_seqcount_begin(&pid_list->seqcount);
> > + ret = false;
> > + upper_chunk = pid_list->upper[upper1];
> > + if (upper_chunk) {
> > + lower_chunk = upper_chunk->data[upper2];
> > + if (lower_chunk)
> > + ret = test_bit(lower, lower_chunk->data);
> > + }
> > + } while (read_seqcount_retry(&pid_list->seqcount, seq));
>
> How is this better? Any numbers?
> If the write side is busy and the lock is handed over from one CPU to
> another then it is possible that the reader spins here and does several
> loops, right?
I think the chances of that is very slim. The writes are at fork and exit
and manually writing to one of the set_*_pid files.
The readers are at every sched_switch. Currently we just use
raw_spin_locks. But that forces a serialization of every sched_switch!
Which on big machines could cause a huge latency.
This approach allows multiple sched_switches to happen at the same time.
> And in this case, how accurate would it be? I mean the result could
> change right after the sequence here is completed because the write side
> got active again. How bad would it be if there would be no locking and
> RCU ensures that the chunks (and data) don't disappear while looking at
> it?
As I mentioned the use case for this, it is very accurate. That's because
the writers are updating the pid bits for themselves. If you are checking
for pid 123, that means task 123 is about to run. If bit 123 is being added
or removed, it would only be done by task 123 or its parent.
The exception to this rule is if a user manually adds or removes a pid from
the set_*_pid file. But that has other races that we don't really care
about. It's known that the update made there may take some milliseconds to
update.
-- Steve
^ permalink raw reply
* Re: [PATCH 1/2] mm/khugepaged: do synchronous writeback for MADV_COLLAPSE
From: David Hildenbrand (Red Hat) @ 2025-11-13 15:30 UTC (permalink / raw)
To: Garg, Shivank, Matthew Wilcox
Cc: Lorenzo Stoakes, Andrew Morton, Zi Yan, Baolin Wang,
Liam R . Howlett, Nico Pache, Ryan Roberts, Dev Jain, Barry Song,
Lance Yang, Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
Zach O'Keefe, linux-mm, linux-kernel, linux-trace-kernel,
Branden Moore
In-Reply-To: <e38e8059-a62b-4ad5-8316-5af02a0cc1b6@amd.com>
On 11.11.25 06:58, Garg, Shivank wrote:
>
>
> On 11/10/2025 7:58 PM, Matthew Wilcox wrote:
>> On Mon, Nov 10, 2025 at 07:50:17PM +0530, Garg, Shivank wrote:
>>> The issue is copying those binary to a freshly mounted filesystem.
>>> The page cache folios remain dirty until background writeback completes.
>>>
>>> Reproduces 100% for me: fresh XFS/EXT4 mount -> copy binary -> execute -> MADV_COLLAPSE fails.
>>
>> Yes, but this is an uncommon thing to do. Really, it's the kind of
>> thing you do when you're testing something (like, whether ext4 supports
>> large folios, and whether that yields a performance improvement).
>> It's more reasonable to change userspace than the kernel to solve this
>> problem you're having.
>
> Fair point.
>
> You're right that this is primarily a testing scenario, though it may also
> potentially affect JIT compilers writing executables (also uncommon) but more
> research is needed.
>
> For userspace workarounds, calling fsync() before MADV_COLLAPSE works.
Right. But do we want document that any caller of MADV_COLLAPSE should
issue an fsync() if MADV_COLLAPSE fails to try again?
IMHO this just reveals a problem that might also be triggered in a
container that just got downloaded or after upgrading a package, no?
--
Cheers
David
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Steven Rostedt @ 2025-11-13 15:24 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113151729.4Zky6d-t@linutronix.de>
On Thu, 13 Nov 2025 16:17:29 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> On 2025-11-13 10:05:24 [-0500], Steven Rostedt wrote:
> > This means that the chunks are not being freed and we can't be doing
> > synchronize_rcu() in every exit.
>
> You don't have to, you can do call_rcu().
But the chunk isn't being freed. They may be used right away.
> So if the kfree() is not an issue, it is just the use of the block from
> the freelist which must not point to a wrong item? And therefore the
> seqcount?
Correct.
>
> > > So I *think* the RCU approach should be doable and cover this.
> >
> > Where would you put the synchronize_rcu()? In do_exit()?
>
> simply call_rcu() and let it move to the freelist.
A couple of issues. One, the chunks are fully used. There's no place to put
a "rcu_head" in them. Well, we may be able to make use of them.
Second, if there's a lot of tasks exiting and forking, we can easily run
out of chunks that are waiting to be "freed" via call_rcu().
>
> > Also understanding what this is used for helps in understanding the scope
> > of protection needed.
> >
> > The pid_list is created when you add anything into one of the pid files in
> > tracefs. Let's use /sys/kernel/tracing/set_ftrace_pid:
> >
> > # cd /sys/kernel/tracing
> > # echo $$ > set_ftrace_pid
> > # echo 1 > options/function-fork
> > # cat set_ftrace_pid
> > 2716
> > 2936
> > # cat set_ftrace_pid
> > 2716
> > 2945
> >
> > What the above did was to create a pid_list for the function tracer. I
> > added the bash process pid using $$ (2716). Then when I cat the file, it
> > showed the pid for the bash process as well as the pid for the cat process,
> > as the cat process is a child of the bash process. The function-fork option
> > means to add any child process to the set_ftrace_pid if the parent is
> > already in the list. It also means to remove the pid if a process in the
> > list exits.
>
> This adding/ add-on-fork, removing and remove-on-exit is the only write
> side?
That and manual writes to the set_ftrace_pid file.
> > What we are protecting against is when one chunk is freed, but then
> > allocated again for a different set of PIDs. Where the reader has the chunk,
> > it was freed and re-allocated and the bit that is about to be checked
> > doesn't represent the bit it is checking for.
>
> This I assumed.
> And the kfree() at the end can not happen while there is still a reader?
Correct. That's done by the pid_list user:
In clear_ftrace_pids():
/* Wait till all users are no longer using pid filtering */
synchronize_rcu();
if ((type & TRACE_PIDS) && pid_list)
trace_pid_list_free(pid_list);
if ((type & TRACE_NO_PIDS) && no_pid_list)
trace_pid_list_free(no_pid_list);
-- Steve
^ permalink raw reply
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Sebastian Andrzej Siewior @ 2025-11-13 15:17 UTC (permalink / raw)
To: Steven Rostedt
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113100524.5c5f6bdc@gandalf.local.home>
On 2025-11-13 10:05:24 [-0500], Steven Rostedt wrote:
> This means that the chunks are not being freed and we can't be doing
> synchronize_rcu() in every exit.
You don't have to, you can do call_rcu().
> > Additionally it would guarantee that the buffer is not released in
> > trace_pid_list_free(). I don't see how the seqcount ensures that the
> > buffer is not gone. I mean you could have a reader and the retry would
> > force you to do another loop but before that happens you dereference the
> > upper_chunk pointer which could be reused.
>
> This protection has nothing to do with trace_pid_list_free(). In fact,
> you'll notice that function doesn't even have any locking. That's because
> the pid_list itself is removed from view and RCU synchronization happens
> before that function is called.
>
> The protection in trace_pid_list_is_set() is only to synchronize with the
> adding and removing of the bits in the updates in exit and fork as well as
> with the user manually writing into the set_*_pid files.
So if the kfree() is not an issue, it is just the use of the block from
the freelist which must not point to a wrong item? And therefore the
seqcount?
> > So I *think* the RCU approach should be doable and cover this.
>
> Where would you put the synchronize_rcu()? In do_exit()?
simply call_rcu() and let it move to the freelist.
> Also understanding what this is used for helps in understanding the scope
> of protection needed.
>
> The pid_list is created when you add anything into one of the pid files in
> tracefs. Let's use /sys/kernel/tracing/set_ftrace_pid:
>
> # cd /sys/kernel/tracing
> # echo $$ > set_ftrace_pid
> # echo 1 > options/function-fork
> # cat set_ftrace_pid
> 2716
> 2936
> # cat set_ftrace_pid
> 2716
> 2945
>
> What the above did was to create a pid_list for the function tracer. I
> added the bash process pid using $$ (2716). Then when I cat the file, it
> showed the pid for the bash process as well as the pid for the cat process,
> as the cat process is a child of the bash process. The function-fork option
> means to add any child process to the set_ftrace_pid if the parent is
> already in the list. It also means to remove the pid if a process in the
> list exits.
This adding/ add-on-fork, removing and remove-on-exit is the only write
side?
> When I enable function tracing, it will only trace the bash process and any
> of its children:
>
> # echo 0 > tracing_on
> # echo function > current_tracer
> # cat set_ftrace_pid ; echo 0 > tracing_on
> 2716
> 2989
> # cat trace
> [..]
> bash-2716 [003] ..... 36854.662833: rcu_read_lock_held <-mtree_range_walk
> bash-2716 [003] ..... 36854.662834: rcu_lockdep_current_cpu_online <-rcu_read_lock_held
> bash-2716 [003] ..... 36854.662834: rcu_read_lock_held <-vma_start_read
> ##### CPU 6 buffer started ####
> cat-2989 [006] d..2. 36854.662834: ret_from_fork <-ret_from_fork_asm
> bash-2716 [003] ..... 36854.662835: rcu_lockdep_current_cpu_online <-rcu_read_lock_held
> cat-2989 [006] d..2. 36854.662836: schedule_tail <-ret_from_fork
> bash-2716 [003] ..... 36854.662836: __rcu_read_unlock <-lock_vma_under_rcu
> cat-2989 [006] d..2. 36854.662836: finish_task_switch.isra.0 <-schedule_tail
> bash-2716 [003] ..... 36854.662836: handle_mm_fault <-do_user_addr_fault
> [..]
>
> It would be way too expensive to check the pid_list at *every* function
> call. But luckily we don't have to. Instead, we set a per-cpu flag in the
> instance trace_array on sched_switch if the next pid is in the pid_list and
> clear it if it is not. (See ftrace_filter_pid_sched_switch_probe()).
>
> This means, the bit being checked in the pid_list is always for a task that
> is about to run.
>
> The bit being cleared, is always for that task that is exiting (except for
> the case of manual updates).
>
> What we are protecting against is when one chunk is freed, but then
> allocated again for a different set of PIDs. Where the reader has the chunk,
> it was freed and re-allocated and the bit that is about to be checked
> doesn't represent the bit it is checking for.
This I assumed.
And the kfree() at the end can not happen while there is still a reader?
…
> And if the "lower" bit matches the set_bit from CPU2, we have a false
> positive. Although, this race is highly unlikely, we should still protect
> against it (it could happen on a VM vCPU that was preempted in
> trace_pid_list_is_set()).
>
> -- Steve
Sebastian
^ permalink raw reply
* [for-trace:latency/for-next PATCH] rv: Fix compilation if !CONFIG_RV_REACTORS
From: Gabriele Monaco @ 2025-11-13 15:06 UTC (permalink / raw)
To: rostedt, Steven Rostedt, Gabriele Monaco, Thomas Weißschuh,
linux-trace-kernel, linux-kernel
Cc: lkp, oe-kbuild-all
In-Reply-To: <c4bb3aa4a73c0ec815bdc30c104b6f90fae5ed39.camel@redhat.com>
The kernel test robot spotted a compilation error if reactors are
disabled.
Fix the warning by keeping LTL monitor variable as always static.
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202511131948.vxi5mdjU-lkp@intel.com/
Fixes: 4f739ed19d22 ("rv: Pass va_list to reactors")
Signed-off-by: Gabriele Monaco <gmonaco@redhat.com>
---
This patch fixes a patch that was just merged to trace:latency/for-next.
It builds locally with both configurations. I started another kernel
robot run and, if it passes, I'm going to send a pull request with only
this.
include/rv/ltl_monitor.h | 5 -----
1 file changed, 5 deletions(-)
diff --git a/include/rv/ltl_monitor.h b/include/rv/ltl_monitor.h
index 00c42b36f961..eff60cd61106 100644
--- a/include/rv/ltl_monitor.h
+++ b/include/rv/ltl_monitor.h
@@ -17,12 +17,7 @@
#endif
#define RV_MONITOR_NAME CONCATENATE(rv_, MONITOR_NAME)
-
-#ifdef CONFIG_RV_REACTORS
static struct rv_monitor RV_MONITOR_NAME;
-#else
-extern struct rv_monitor RV_MONITOR_NAME;
-#endif
static int ltl_monitor_slot = RV_PER_TASK_MONITOR_INIT;
base-commit: 69d8895cb9a9f6450374577af8584c2e21cb5a9f
--
2.51.1
^ permalink raw reply related
* Re: [PATCH v3] trace/pid_list: optimize pid_list->lock contention
From: Steven Rostedt @ 2025-11-13 15:05 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Yongliang Gao, mhiramat, mathieu.desnoyers, linux-kernel,
linux-trace-kernel, frankjpliu, Yongliang Gao, Huang Cun
In-Reply-To: <20251113141515.iZSIDK0T@linutronix.de>
On Thu, 13 Nov 2025 15:15:15 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> I don't see why RCU work shouldn't work here.
> If a pid is removed then it might result that a chunk cleared/ removed
> then upper_chunk/ lower_chunk can become NULL. The buffer itself can be
> reused and point to something else. It could lear to a false outcome in
> test_bit(). This is handled by read_seqcount_retry().
>
> You could assign upper1, upper2, to NULL/ buffer as now and the removal
> (in put_lower_chunk(), put_upper_chunk()) delay to RCU so it happens
> after the grace period. That would allow you to iterate over it in
> trace_pid_list_is_set() lockless since the buffer will not disappear and
> will not be reused for another task until after all reader left.
The chunks are reused. When they are cleared, they go to a free-list, and
when a new chunk is needed, it is taken from the free-list so that we do
not need to allocate.
When all the bits in a chunk are cleared it moves to the free-list. This
can happen in exit, when a task exits and it clears its pid bit from the
pid_list.
Then a chunk can be allocated in fork, when a task whose pid is in the
pid_list adds its child to the list as well.
This means that the chunks are not being freed and we can't be doing
synchronize_rcu() in every exit.
>
> Additionally it would guarantee that the buffer is not released in
> trace_pid_list_free(). I don't see how the seqcount ensures that the
> buffer is not gone. I mean you could have a reader and the retry would
> force you to do another loop but before that happens you dereference the
> upper_chunk pointer which could be reused.
This protection has nothing to do with trace_pid_list_free(). In fact,
you'll notice that function doesn't even have any locking. That's because
the pid_list itself is removed from view and RCU synchronization happens
before that function is called.
The protection in trace_pid_list_is_set() is only to synchronize with the
adding and removing of the bits in the updates in exit and fork as well as
with the user manually writing into the set_*_pid files.
>
> So I *think* the RCU approach should be doable and cover this.
Where would you put the synchronize_rcu()? In do_exit()?
Also understanding what this is used for helps in understanding the scope
of protection needed.
The pid_list is created when you add anything into one of the pid files in
tracefs. Let's use /sys/kernel/tracing/set_ftrace_pid:
# cd /sys/kernel/tracing
# echo $$ > set_ftrace_pid
# echo 1 > options/function-fork
# cat set_ftrace_pid
2716
2936
# cat set_ftrace_pid
2716
2945
What the above did was to create a pid_list for the function tracer. I
added the bash process pid using $$ (2716). Then when I cat the file, it
showed the pid for the bash process as well as the pid for the cat process,
as the cat process is a child of the bash process. The function-fork option
means to add any child process to the set_ftrace_pid if the parent is
already in the list. It also means to remove the pid if a process in the
list exits.
When I enable function tracing, it will only trace the bash process and any
of its children:
# echo 0 > tracing_on
# echo function > current_tracer
# cat set_ftrace_pid ; echo 0 > tracing_on
2716
2989
# cat trace
[..]
bash-2716 [003] ..... 36854.662833: rcu_read_lock_held <-mtree_range_walk
bash-2716 [003] ..... 36854.662834: rcu_lockdep_current_cpu_online <-rcu_read_lock_held
bash-2716 [003] ..... 36854.662834: rcu_read_lock_held <-vma_start_read
##### CPU 6 buffer started ####
cat-2989 [006] d..2. 36854.662834: ret_from_fork <-ret_from_fork_asm
bash-2716 [003] ..... 36854.662835: rcu_lockdep_current_cpu_online <-rcu_read_lock_held
cat-2989 [006] d..2. 36854.662836: schedule_tail <-ret_from_fork
bash-2716 [003] ..... 36854.662836: __rcu_read_unlock <-lock_vma_under_rcu
cat-2989 [006] d..2. 36854.662836: finish_task_switch.isra.0 <-schedule_tail
bash-2716 [003] ..... 36854.662836: handle_mm_fault <-do_user_addr_fault
[..]
It would be way too expensive to check the pid_list at *every* function
call. But luckily we don't have to. Instead, we set a per-cpu flag in the
instance trace_array on sched_switch if the next pid is in the pid_list and
clear it if it is not. (See ftrace_filter_pid_sched_switch_probe()).
This means, the bit being checked in the pid_list is always for a task that
is about to run.
The bit being cleared, is always for that task that is exiting (except for
the case of manual updates).
What we are protecting against is when one chunk is freed, but then
allocated again for a different set of PIDs. Where the reader has the chunk,
it was freed and re-allocated and the bit that is about to be checked
doesn't represent the bit it is checking for.
CPU1 CPU2
---- ----
trace_pid_list_is_set() {
lower_chunk = upper_chunk->data[upper2];
do_exit() {
trace_pid_list_clear() {
lower_chunk = upper_chunk->data[upper2]; // same lower_chunk
clear_bit(lower, lower_chunk->data);
if (find_first_bit(lower_chunk->data, LOWER_MAX) >= LOWER_MAX)
put_lower_chunk(pid_list, lower_chunk);
[..]
}
}
fork() {
trace_pid_list_set() {
lower_chunk = get_lower_chunk(pid_list);
upper_chunk->data[upper2] = lower_chunk; // upper2 is a different value here
set_bit(lower, lower_chunk->data);
}
}
ret = test_bit(lower, lower_chunk->data);
And if the "lower" bit matches the set_bit from CPU2, we have a false
positive. Although, this race is highly unlikely, we should still protect
against it (it could happen on a VM vCPU that was preempted in
trace_pid_list_is_set()).
-- Steve
^ permalink raw reply
* [PATCH v3 21/21] tracing: Switch to use %ptSp
From: Andy Shevchenko @ 2025-11-13 14:32 UTC (permalink / raw)
To: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Ulf Hansson, Andy Shevchenko, Aleksandr Loktionov,
Vitaly Lifshits, Manivannan Sadhasivam, Niklas Cassel,
Calvin Owens, Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-1-andriy.shevchenko@linux.intel.com>
Use %ptSp instead of open coded variants to print content of
struct timespec64 in human readable format.
Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
kernel/trace/trace_output.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/trace/trace_output.c b/kernel/trace/trace_output.c
index ebbab3e9622b..cc2d3306bb60 100644
--- a/kernel/trace/trace_output.c
+++ b/kernel/trace/trace_output.c
@@ -1490,12 +1490,12 @@ trace_hwlat_print(struct trace_iterator *iter, int flags,
trace_assign_type(field, entry);
- trace_seq_printf(s, "#%-5u inner/outer(us): %4llu/%-5llu ts:%lld.%09ld count:%d",
+ trace_seq_printf(s, "#%-5u inner/outer(us): %4llu/%-5llu ts:%ptSp count:%d",
field->seqnum,
field->duration,
field->outer_duration,
- (long long)field->timestamp.tv_sec,
- field->timestamp.tv_nsec, field->count);
+ &field->timestamp,
+ field->count);
if (field->nmi_count) {
/*
--
2.50.1
^ permalink raw reply related
* [PATCH v3 20/21] scsi: snic: Switch to use %ptSp
From: Andy Shevchenko @ 2025-11-13 14:32 UTC (permalink / raw)
To: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Ulf Hansson, Andy Shevchenko, Aleksandr Loktionov,
Vitaly Lifshits, Manivannan Sadhasivam, Niklas Cassel,
Calvin Owens, Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-1-andriy.shevchenko@linux.intel.com>
Use %ptSp instead of open coded variants to print content of
struct timespec64 in human readable format.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/scsi/snic/snic_debugfs.c | 10 ++++------
drivers/scsi/snic/snic_trc.c | 5 ++---
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/snic/snic_debugfs.c b/drivers/scsi/snic/snic_debugfs.c
index 9dd975b36b5b..edf3e5ef28a6 100644
--- a/drivers/scsi/snic/snic_debugfs.c
+++ b/drivers/scsi/snic/snic_debugfs.c
@@ -282,8 +282,8 @@ snic_stats_show(struct seq_file *sfp, void *data)
jiffies_to_timespec64(stats->misc.last_ack_time, &last_ack_tms);
seq_printf(sfp,
- "Last ISR Time : %llu (%8llu.%09lu)\n"
- "Last Ack Time : %llu (%8llu.%09lu)\n"
+ "Last ISR Time : %llu (%ptSp)\n"
+ "Last Ack Time : %llu (%ptSp)\n"
"Ack ISRs : %llu\n"
"IO Cmpl ISRs : %llu\n"
"Err Notify ISRs : %llu\n"
@@ -298,10 +298,8 @@ snic_stats_show(struct seq_file *sfp, void *data)
"Queue Ramp Down : %lld\n"
"Queue Last Queue Depth : %lld\n"
"Target Not Ready : %lld\n",
- (u64) stats->misc.last_isr_time,
- last_isr_tms.tv_sec, last_isr_tms.tv_nsec,
- (u64)stats->misc.last_ack_time,
- last_ack_tms.tv_sec, last_ack_tms.tv_nsec,
+ (u64) stats->misc.last_isr_time, &last_isr_tms,
+ (u64) stats->misc.last_ack_time, &last_ack_tms,
(u64) atomic64_read(&stats->misc.ack_isr_cnt),
(u64) atomic64_read(&stats->misc.cmpl_isr_cnt),
(u64) atomic64_read(&stats->misc.errnotify_isr_cnt),
diff --git a/drivers/scsi/snic/snic_trc.c b/drivers/scsi/snic/snic_trc.c
index c2e5ab7e976c..6bad1ea9a6a7 100644
--- a/drivers/scsi/snic/snic_trc.c
+++ b/drivers/scsi/snic/snic_trc.c
@@ -56,9 +56,8 @@ snic_fmt_trc_data(struct snic_trc_data *td, char *buf, int buf_sz)
jiffies_to_timespec64(td->ts, &tmspec);
len += snprintf(buf, buf_sz,
- "%llu.%09lu %-25s %3d %4x %16llx %16llx %16llx %16llx %16llx\n",
- tmspec.tv_sec,
- tmspec.tv_nsec,
+ "%ptSp %-25s %3d %4x %16llx %16llx %16llx %16llx %16llx\n",
+ &tmspec,
td->fn,
td->hno,
td->tag,
--
2.50.1
^ permalink raw reply related
* [PATCH v3 19/21] scsi: fnic: Switch to use %ptSp
From: Andy Shevchenko @ 2025-11-13 14:32 UTC (permalink / raw)
To: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Ulf Hansson, Andy Shevchenko, Aleksandr Loktionov,
Vitaly Lifshits, Manivannan Sadhasivam, Niklas Cassel,
Calvin Owens, Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-1-andriy.shevchenko@linux.intel.com>
Use %ptSp instead of open coded variants to print content of
struct timespec64 in human readable format.
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/scsi/fnic/fnic_trace.c | 52 ++++++++++++++--------------------
1 file changed, 22 insertions(+), 30 deletions(-)
diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c
index cdc6b12b1ec2..0a849a195a8e 100644
--- a/drivers/scsi/fnic/fnic_trace.c
+++ b/drivers/scsi/fnic/fnic_trace.c
@@ -138,9 +138,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
*/
len += scnprintf(fnic_dbgfs_prt->buffer + len,
(trace_max_pages * PAGE_SIZE * 3) - len,
- "%16llu.%09lu %-50s %8x %8x %16llx %16llx "
- "%16llx %16llx %16llx\n", (u64)val.tv_sec,
- val.tv_nsec, str, tbp->host_no, tbp->tag,
+ "%ptSp %-50s %8x %8x %16llx %16llx %16llx %16llx %16llx\n",
+ &val, str, tbp->host_no, tbp->tag,
tbp->data[0], tbp->data[1], tbp->data[2],
tbp->data[3], tbp->data[4]);
rd_idx++;
@@ -180,9 +179,8 @@ int fnic_get_trace_data(fnic_dbgfs_t *fnic_dbgfs_prt)
*/
len += scnprintf(fnic_dbgfs_prt->buffer + len,
(trace_max_pages * PAGE_SIZE * 3) - len,
- "%16llu.%09lu %-50s %8x %8x %16llx %16llx "
- "%16llx %16llx %16llx\n", (u64)val.tv_sec,
- val.tv_nsec, str, tbp->host_no, tbp->tag,
+ "%ptSp %-50s %8x %8x %16llx %16llx %16llx %16llx %16llx\n",
+ &val, str, tbp->host_no, tbp->tag,
tbp->data[0], tbp->data[1], tbp->data[2],
tbp->data[3], tbp->data[4]);
rd_idx++;
@@ -215,30 +213,26 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
{
int len = 0;
int buf_size = debug->buf_size;
- struct timespec64 val1, val2;
+ struct timespec64 val, val1, val2;
int i = 0;
- ktime_get_real_ts64(&val1);
+ ktime_get_real_ts64(&val);
len = scnprintf(debug->debug_buffer + len, buf_size - len,
"------------------------------------------\n"
"\t\tTime\n"
"------------------------------------------\n");
+ val1 = timespec64_sub(val, stats->stats_timestamps.last_reset_time);
+ val2 = timespec64_sub(val, stats->stats_timestamps.last_read_time);
len += scnprintf(debug->debug_buffer + len, buf_size - len,
- "Current time : [%lld:%ld]\n"
- "Last stats reset time: [%lld:%09ld]\n"
- "Last stats read time: [%lld:%ld]\n"
- "delta since last reset: [%lld:%ld]\n"
- "delta since last read: [%lld:%ld]\n",
- (s64)val1.tv_sec, val1.tv_nsec,
- (s64)stats->stats_timestamps.last_reset_time.tv_sec,
- stats->stats_timestamps.last_reset_time.tv_nsec,
- (s64)stats->stats_timestamps.last_read_time.tv_sec,
- stats->stats_timestamps.last_read_time.tv_nsec,
- (s64)timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_sec,
- timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_nsec,
- (s64)timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_sec,
- timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_nsec);
+ "Current time : [%ptSp]\n"
+ "Last stats reset time: [%ptSp]\n"
+ "Last stats read time: [%ptSp]\n"
+ "delta since last reset: [%ptSp]\n"
+ "delta since last read: [%ptSp]\n",
+ &val,
+ &stats->stats_timestamps.last_reset_time, &val1,
+ &stats->stats_timestamps.last_read_time, &val2);
stats->stats_timestamps.last_read_time = val1;
@@ -416,8 +410,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
jiffies_to_timespec64(stats->misc_stats.last_ack_time, &val2);
len += scnprintf(debug->debug_buffer + len, buf_size - len,
- "Last ISR time: %llu (%8llu.%09lu)\n"
- "Last ACK time: %llu (%8llu.%09lu)\n"
+ "Last ISR time: %llu (%ptSp)\n"
+ "Last ACK time: %llu (%ptSp)\n"
"Max ISR jiffies: %llu\n"
"Max ISR time (ms) (0 denotes < 1 ms): %llu\n"
"Corr. work done: %llu\n"
@@ -437,10 +431,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug,
"Number of rport not ready: %lld\n"
"Number of receive frame errors: %lld\n"
"Port speed (in Mbps): %lld\n",
- (u64)stats->misc_stats.last_isr_time,
- (s64)val1.tv_sec, val1.tv_nsec,
- (u64)stats->misc_stats.last_ack_time,
- (s64)val2.tv_sec, val2.tv_nsec,
+ (u64)stats->misc_stats.last_isr_time, &val1,
+ (u64)stats->misc_stats.last_ack_time, &val2,
(u64)atomic64_read(&stats->misc_stats.max_isr_jiffies),
(u64)atomic64_read(&stats->misc_stats.max_isr_time_ms),
(u64)atomic64_read(&stats->misc_stats.corr_work_done),
@@ -857,8 +849,8 @@ void copy_and_format_trace_data(struct fc_trace_hdr *tdata,
len = *orig_len;
len += scnprintf(fnic_dbgfs_prt->buffer + len, max_size - len,
- "%ptTs.%09lu ns%8x %c%8x\t",
- &tdata->time_stamp.tv_sec, tdata->time_stamp.tv_nsec,
+ "%ptSs ns%8x %c%8x\t",
+ &tdata->time_stamp,
tdata->host_no, tdata->frame_type, tdata->frame_len);
fc_trace = (char *)FC_TRACE_ADDRESS(tdata);
--
2.50.1
^ permalink raw reply related
* [PATCH v3 15/21] PCI: epf-test: Switch to use %ptSp
From: Andy Shevchenko @ 2025-11-13 14:32 UTC (permalink / raw)
To: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Ulf Hansson, Andy Shevchenko, Aleksandr Loktionov,
Vitaly Lifshits, Manivannan Sadhasivam, Niklas Cassel,
Calvin Owens, Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-1-andriy.shevchenko@linux.intel.com>
Use %ptSp instead of open coded variants to print content of
struct timespec64 in human readable format.
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pci/endpoint/functions/pci-epf-test.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index b05e8db575c3..debd235253c5 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -331,9 +331,8 @@ static void pci_epf_test_print_rate(struct pci_epf_test *epf_test,
rate = div64_u64(size * NSEC_PER_SEC, ns * 1000);
dev_info(&epf_test->epf->dev,
- "%s => Size: %llu B, DMA: %s, Time: %llu.%09u s, Rate: %llu KB/s\n",
- op, size, dma ? "YES" : "NO",
- (u64)ts.tv_sec, (u32)ts.tv_nsec, rate);
+ "%s => Size: %llu B, DMA: %s, Time: %ptSp s, Rate: %llu KB/s\n",
+ op, size, dma ? "YES" : "NO", &ts, rate);
}
static void pci_epf_test_copy(struct pci_epf_test *epf_test,
--
2.50.1
^ permalink raw reply related
* [PATCH v3 14/21] net: dsa: sja1105: Switch to use %ptSp
From: Andy Shevchenko @ 2025-11-13 14:32 UTC (permalink / raw)
To: Corey Minyard, Christian König, Dr. David Alan Gilbert,
Alex Deucher, Thomas Zimmermann, Dmitry Baryshkov, Rob Clark,
Matthew Brost, Ulf Hansson, Andy Shevchenko, Aleksandr Loktionov,
Vitaly Lifshits, Manivannan Sadhasivam, Niklas Cassel,
Calvin Owens, Vadim Fedorenko, Sagi Maimon, Martin K. Petersen,
Karan Tilak Kumar, Hans Verkuil, Casey Schaufler, Steven Rostedt,
Petr Mladek, Viacheslav Dubeyko, Max Kellermann, linux-doc,
linux-kernel, openipmi-developer, linux-media, dri-devel,
linaro-mm-sig, amd-gfx, linux-arm-msm, freedreno, intel-xe,
linux-mmc, netdev, intel-wired-lan, linux-pci, linux-s390,
linux-scsi, linux-staging, ceph-devel, linux-trace-kernel
Cc: Rasmus Villemoes, Sergey Senozhatsky, Jonathan Corbet,
Sumit Semwal, Gustavo Padovan, David Airlie, Simona Vetter,
Maarten Lankhorst, Maxime Ripard, Dmitry Baryshkov, Abhinav Kumar,
Jessica Zhang, Sean Paul, Marijn Suijten, Konrad Dybcio,
Lucas De Marchi, Thomas Hellström, Rodrigo Vivi,
Vladimir Oltean, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Tony Nguyen, Przemek Kitszel,
Krzysztof Wilczyński, Kishon Vijay Abraham I, Bjorn Helgaas,
Rodolfo Giometti, Jonathan Lemon, Richard Cochran,
Stefan Haberland, Jan Hoeppner, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Christian Borntraeger, Sven Schnelle,
Satish Kharat, Sesidhar Baddela, James E.J. Bottomley,
Mauro Carvalho Chehab, Greg Kroah-Hartman, Xiubo Li, Ilya Dryomov,
Masami Hiramatsu, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251113150217.3030010-1-andriy.shevchenko@linux.intel.com>
Use %ptSp instead of open coded variants to print content of
struct timespec64 in human readable format.
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/net/dsa/sja1105/sja1105_tas.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_tas.c b/drivers/net/dsa/sja1105/sja1105_tas.c
index d7818710bc02..d5949d2c3e71 100644
--- a/drivers/net/dsa/sja1105/sja1105_tas.c
+++ b/drivers/net/dsa/sja1105/sja1105_tas.c
@@ -775,9 +775,8 @@ static void sja1105_tas_state_machine(struct work_struct *work)
base_time_ts = ns_to_timespec64(base_time);
now_ts = ns_to_timespec64(now);
- dev_dbg(ds->dev, "OPER base time %lld.%09ld (now %lld.%09ld)\n",
- base_time_ts.tv_sec, base_time_ts.tv_nsec,
- now_ts.tv_sec, now_ts.tv_nsec);
+ dev_dbg(ds->dev, "OPER base time %ptSp (now %ptSp)\n",
+ &base_time_ts, &now_ts);
break;
@@ -798,8 +797,7 @@ static void sja1105_tas_state_machine(struct work_struct *work)
if (now < tas_data->oper_base_time) {
/* TAS has not started yet */
diff = ns_to_timespec64(tas_data->oper_base_time - now);
- dev_dbg(ds->dev, "time to start: [%lld.%09ld]",
- diff.tv_sec, diff.tv_nsec);
+ dev_dbg(ds->dev, "time to start: [%ptSp]", &diff);
break;
}
--
2.50.1
^ permalink raw reply related
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