* Re: [PATCHv5 bpf-next 5/9] ftrace: Add update_ftrace_direct_del function
From: Steven Rostedt @ 2025-12-18 1:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251215211402.353056-6-jolsa@kernel.org>
On Mon, 15 Dec 2025 22:13:58 +0100
Jiri Olsa <jolsa@kernel.org> wrote:
> +/**
> + * hash_sub - substracts @b from @a and returns the result
> + * @a: struct ftrace_hash object
> + * @b: struct ftrace_hash object
> + *
> + * Returns struct ftrace_hash object on success, NULL on error.
> + */
> +static struct ftrace_hash *hash_sub(struct ftrace_hash *a, struct ftrace_hash *b)
> +{
> + struct ftrace_func_entry *entry, *del;
> + struct ftrace_hash *sub;
> + int size, i;
> +
> + sub = alloc_and_copy_ftrace_hash(a->size_bits, a);
> + if (!sub)
> + goto error;
Again, this can be just return NULL;
> +
> + size = 1 << b->size_bits;
> + for (i = 0; i < size; i++) {
You can make this for (int i = 0; ...) too.
> + hlist_for_each_entry(entry, &b->buckets[i], hlist) {
> + del = __ftrace_lookup_ip(sub, entry->ip);
> + if (WARN_ON_ONCE(!del))
> + goto error;
And you can remove the error label here too:
if (WARN_ON_ONCE(!del)) {
free_ftrace_hash(sub);
return NULL;
}
> + remove_hash_entry(sub, del);
> + kfree(del);
> + }
> + }
> + return sub;
> +
> + error:
> + free_ftrace_hash(sub);
> + return NULL;
> +}
> +
> +int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> +{
> + struct ftrace_hash *old_direct_functions = NULL, *new_direct_functions;
> + struct ftrace_hash *old_filter_hash, *new_filter_hash = NULL;
> + struct ftrace_func_entry *del, *entry;
One variable per line.
> + unsigned long size, i;
> + int err = -EINVAL;
> +
> + if (!hash_count(hash))
> + return -EINVAL;
> + 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);
> +
> + old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> +
> + if (!hash_count(old_filter_hash))
> + goto out_unlock;
> +
> + /* Make sure requested entries are already registered. */
> + size = 1 << hash->size_bits;
> + for (i = 0; i < size; i++) {
for (int i = 0; ...)
-- Steve
> + 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;
> + new_filter_hash = hash_sub(old_filter_hash, hash);
> + if (!new_filter_hash)
> + goto out_unlock;
> +
> + new_direct_functions = hash_sub(direct_functions, hash);
> + if (!new_direct_functions)
> + goto out_unlock;
> +
> + /* If there's nothing left, we need to unregister the ops. */
> + if (ftrace_hash_empty(new_filter_hash)) {
> + err = unregister_ftrace_function(ops);
> + if (!err) {
> + /* 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, new_filter_hash, EMPTY_HASH);
> + /*
> + * new_filter_hash is dup-ed, so we need to release it anyway,
> + * old_filter_hash either stays on error or is already released
> + */
> + }
> +
> + if (err) {
> + /* free the new_direct_functions */
> + old_direct_functions = new_direct_functions;
> + } else {
> + rcu_assign_pointer(direct_functions, new_direct_functions);
> + }
> +
> + out_unlock:
> + mutex_unlock(&direct_mutex);
> +
> + if (old_direct_functions && old_direct_functions != EMPTY_HASH)
> + call_rcu_tasks(&old_direct_functions->rcu, register_ftrace_direct_cb);
> + free_ftrace_hash(new_filter_hash);
> +
> + return err;
> +}
> +
> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>
> /**
^ permalink raw reply
* Re: [PATCHv5 bpf-next 4/9] ftrace: Add update_ftrace_direct_add function
From: Steven Rostedt @ 2025-12-18 1:39 UTC (permalink / raw)
To: Jiri Olsa
Cc: Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251215211402.353056-5-jolsa@kernel.org>
On Mon, 15 Dec 2025 22:13:57 +0100
Jiri Olsa <jolsa@kernel.org> wrote:
> +/**
> + * hash_add - adds two struct ftrace_hash and returns the result
> + * @a: struct ftrace_hash object
> + * @b: struct ftrace_hash object
> + *
> + * Returns struct ftrace_hash object on success, NULL on error.
> + */
> +static struct ftrace_hash *hash_add(struct ftrace_hash *a, struct ftrace_hash *b)
> +{
> + struct ftrace_func_entry *entry;
> + struct ftrace_hash *add;
> + int size, i;
> +
> + size = hash_count(a) + hash_count(b);
> + if (size > 32)
> + size = 32;
> +
> + add = alloc_and_copy_ftrace_hash(fls(size), a);
> + if (!add)
> + goto error;
You can just return NULL here, as add is NULL.
> +
> + size = 1 << b->size_bits;
> + for (i = 0; i < size; i++) {
> + hlist_for_each_entry(entry, &b->buckets[i], hlist) {
> + if (add_hash_entry_direct(add, entry->ip, entry->direct) == NULL)
> + goto error;
Could remove the error and have:
if (add_hash_entry_direct(add, entry->ip, entry->direct) == NULL) {
free_ftrace_hash(add);
return NULL;
}
> + }
> + }
> + return add;
> +
> + error:
> + free_ftrace_hash(add);
> + return NULL;
> +}
> +
Non static functions require a kerneldoc header.
> +int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash)
> +{
> + struct ftrace_hash *old_direct_functions = NULL, *new_direct_functions;
> + struct ftrace_hash *old_filter_hash, *new_filter_hash = NULL;
BTW, I prefer to not double up on variables. That is to have each on
their own lines. Makes it easier to read for me.
> + struct ftrace_func_entry *entry;
> + int i, size, err = -EINVAL;
Even here.
> + bool reg;
> +
> + if (!hash_count(hash))
> + return -EINVAL;
> +
> + mutex_lock(&direct_mutex);
> +
> + /* Make sure requested entries are not already registered. */
> + size = 1 << hash->size_bits;
> + for (i = 0; i < size; i++) {
If you want, you can remove the i declaration and use for(int i = 0; ... here.
> + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> + if (__ftrace_lookup_ip(direct_functions, entry->ip))
> + goto out_unlock;
> + }
> + }
> +
> + old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> +
> + /* If there's nothing in filter_hash we need to register the ops. */
> + reg = hash_count(old_filter_hash) == 0;
> + if (reg) {
> + if (ops->func || ops->trampoline)
> + goto out_unlock;
> + if (ops->flags & FTRACE_OPS_FL_ENABLED)
> + goto out_unlock;
> + }
> +
> + err = -ENOMEM;
> + new_filter_hash = hash_add(old_filter_hash, hash);
> + if (!new_filter_hash)
> + goto out_unlock;
> +
> + new_direct_functions = hash_add(direct_functions, hash);
> + if (!new_direct_functions)
> + goto out_unlock;
> +
> + old_direct_functions = direct_functions;
> + rcu_assign_pointer(direct_functions, new_direct_functions);
> +
> + if (reg) {
> + ops->func = call_direct_funcs;
> + ops->flags |= MULTI_FLAGS;
> + ops->trampoline = FTRACE_REGS_ADDR;
> + ops->local_hash.filter_hash = new_filter_hash;
> +
> + err = register_ftrace_function_nolock(ops);
> + if (err) {
> + /* restore old filter on error */
> + ops->local_hash.filter_hash = old_filter_hash;
> +
> + /* cleanup for possible another register call */
> + ops->func = NULL;
> + ops->trampoline = 0;
> + } else {
> + new_filter_hash = old_filter_hash;
> + }
> + } else {
> + err = ftrace_update_ops(ops, new_filter_hash, EMPTY_HASH);
> + /*
> + * new_filter_hash is dup-ed, so we need to release it anyway,
> + * old_filter_hash either stays on error or is already released
> + */
> + }
> +
> + if (err) {
> + /* reset direct_functions and free the new one */
> + rcu_assign_pointer(direct_functions, old_direct_functions);
> + old_direct_functions = new_direct_functions;
> + }
> +
> + out_unlock:
> + mutex_unlock(&direct_mutex);
> +
> + if (old_direct_functions && old_direct_functions != EMPTY_HASH)
> + call_rcu_tasks(&old_direct_functions->rcu, register_ftrace_direct_cb);
> + if (new_filter_hash)
free_ftrace_hash() checks for NULL, so you don't need the above if statement.
> + free_ftrace_hash(new_filter_hash);
> +
> + return err;
-- Steve
> +}
> +
> #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>
> /**
^ permalink raw reply
* Re: [PATCH v4 1/7] LoongArch: ftrace: Refactor register restoration in ftrace_common_return
From: Chenghao Duan @ 2025-12-18 1:26 UTC (permalink / raw)
To: bot+bpf-ci
Cc: yangtiezhu, rostedt, mhiramat, mark.rutland, hengqi.chen,
chenhuacai, kernel, zhangtianyang, masahiroy, linux-kernel,
loongarch, bpf, youling.tang, jianghaoran, vincent.mc.li,
linux-trace-kernel, ast, andrii, daniel, martin.lau, eddyz87,
yonghong.song, clm, ihor.solodrai
In-Reply-To: <5c7c7239a6f89e685f69093ff84fe686c75cf6b9b051dcbf812841f00ab3bc31@mail.kernel.org>
On Wed, Dec 17, 2025 at 06:55:00AM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/arch/loongarch/kernel/mcount_dyn.S b/arch/loongarch/kernel/mcount_dyn.S
> > index d6b474ad1d5e..5729c20e5b8b 100644
> > --- a/arch/loongarch/kernel/mcount_dyn.S
> > +++ b/arch/loongarch/kernel/mcount_dyn.S
> > @@ -94,7 +94,6 @@ SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL)
> > * at the callsite, so there is no need to restore the T series regs.
> > */
> > ftrace_common_return:
> > - PTR_L ra, sp, PT_R1
> > PTR_L a0, sp, PT_R4
> > PTR_L a1, sp, PT_R5
> > PTR_L a2, sp, PT_R6
> > @@ -104,12 +103,17 @@ ftrace_common_return:
> > PTR_L a6, sp, PT_R10
> > PTR_L a7, sp, PT_R11
> > PTR_L fp, sp, PT_R22
> > - PTR_L t0, sp, PT_ERA
> > PTR_L t1, sp, PT_R13
> > - PTR_ADDI sp, sp, PT_SIZE
> > bnez t1, .Ldirect
> > +
> > + PTR_L ra, sp, PT_R1
> > + PTR_L t0, sp, PT_ERA
> > + PTR_ADDI sp, sp, PT_SIZE
> > jr t0
> > .Ldirect:
> > + PTR_L t0, sp, PT_R1
> > + PTR_L ra, sp, PT_ERA
> ^^^^^^^^^^^^^^^^^^^
> Does this break existing ftrace direct trampolines?
>
> Before this change, direct trampolines received:
> ra = parent return address (from PT_R1)
> t0 = traced function address (from PT_ERA)
>
> After this change, direct trampolines receive:
> t0 = parent return address (from PT_R1)
> ra = traced function address (from PT_ERA)
>
> The sample trampolines in samples/ftrace/ were written for the old
> convention. For example, samples/ftrace/ftrace-direct.c does:
>
> my_tramp:
> st.d $t0, $sp, 8 # Save what it thinks is traced func addr
> st.d $ra, $sp, 16 # Save what it thinks is parent addr
> bl my_direct_func
> ld.d $t0, $sp, 8 # Restore
> ld.d $ra, $sp, 16 # Restore
> jr $t0 # Jump to what it thinks is traced function
>
> With the new convention, the sample would save parent address in the
> t0 slot, restore it to t0, and then jump to the parent instead of the
> traced function, skipping the traced function entirely.
>
> This was fixed later in commit 055e392f3be7 "LoongArch: ftrace: Adjust
> register stack restore order in direct call trampolines" which updated
> all the samples to match the new convention. However, any out-of-tree
> direct trampolines written before this commit would be broken by this
> change.
>
> Was this ABI break intentional? The commit message mentions matching
> "the state when ftrace was entered" but doesn't explain why breaking
> compatibility was necessary. RISC-V uses the opposite convention
> (t0 = traced func, ra = parent), which the original LoongArch code
> matched.
The patch "LoongArch: ftrace: Refactor register restoration in
ftrace_common_return" changed the conventions for t0 and ra after
entering the direct call function, which is exactly the reason forthe
changes made in this patch.
Chenghao
>
> > + PTR_ADDI sp, sp, PT_SIZE
> > jr t1
> > SYM_CODE_END(ftrace_common)
>
> [ ... ]
>
> > @@ -161,6 +165,8 @@ SYM_CODE_END(return_to_handler)
> > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> > SYM_CODE_START(ftrace_stub_direct_tramp)
> > UNWIND_HINT_UNDEFINED
> > - jr t0
> > + move t1, ra
> > + move ra, t0
> > + jr t1
> > SYM_CODE_END(ftrace_stub_direct_tramp)
> > #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/20293821855
^ permalink raw reply
* Re: [PATCHv5 bpf-next 3/9] ftrace: Export some of hash related functions
From: Steven Rostedt @ 2025-12-18 1:07 UTC (permalink / raw)
To: Jiri Olsa
Cc: Florent Revest, Mark Rutland, bpf, linux-kernel,
linux-trace-kernel, linux-arm-kernel, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Menglong Dong, Song Liu
In-Reply-To: <20251215211402.353056-4-jolsa@kernel.org>
On Mon, 15 Dec 2025 22:13:56 +0100
Jiri Olsa <jolsa@kernel.org> wrote:
> index 505b7d3f5641..c0a72fcae1f6 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -82,6 +82,7 @@ static inline void early_trace_init(void) { }
>
> struct module;
> struct ftrace_hash;
> +struct ftrace_func_entry;
>
> #if defined(CONFIG_FUNCTION_TRACER) && defined(CONFIG_MODULES) && \
> defined(CONFIG_DYNAMIC_FTRACE)
> @@ -405,6 +406,14 @@ enum ftrace_ops_cmd {
> typedef int (*ftrace_ops_func_t)(struct ftrace_ops *op, enum ftrace_ops_cmd cmd);
>
> #ifdef CONFIG_DYNAMIC_FTRACE
> +
> +#define FTRACE_HASH_DEFAULT_BITS 10
> +
> +struct ftrace_hash *alloc_ftrace_hash(int size_bits);
> +void free_ftrace_hash(struct ftrace_hash *hash);
> +struct ftrace_func_entry *add_hash_entry_direct(struct ftrace_hash *hash,
As this is no longer static and is exported to other users within the
kernel, it should be renamed to: add_ftrace_hash_entry_direct()
to keep the namespace unique.
-- Steve
> + unsigned long ip, unsigned long direct);
> +
> /* The hash used to know what functions callbacks trace */
> struct ftrace_ops_hash {
> struct ftrace_hash __rcu *notrace_hash;
^ permalink raw reply
* Re: [PATCH v3 01/13] rv: Unify DA event handling functions across monitor types
From: Nam Cao @ 2025-12-18 1:04 UTC (permalink / raw)
To: Gabriele Monaco, linux-kernel, Steven Rostedt, Gabriele Monaco,
linux-trace-kernel
Cc: Tomas Glozar, Juri Lelli, Clark Williams, John Kacur
In-Reply-To: <20251205131621.135513-2-gmonaco@redhat.com>
Gabriele Monaco <gmonaco@redhat.com> writes:
> The DA event handling functions are mostly duplicated because the
> per-task monitors need to propagate the task to use the pid in the trace
> events. This is a maintenance burden for a little advantage.
> The task can be obtained with some pointer arithmetic from the da_mon,
> hence only the function tracing events really need to differ.
>
> Unify all code handling the events, create da_trace_event() and
> da_trace_error() that only call the tracepoint function.
> Propagate the monitor id through the calls, the do_trace_ functions use
> the id (pid) in case of per-task monitors but ignore it for implicit
> monitors.
I think this overexplains. I would just explain the "WHY", then the
general idea how the patch solves the problem. Let the diff speaks for
the rest.
Perhaps something like:
The DA event handling functions are mostly duplicated for
differerent monitor types, because per-task monitors' functions
require a task_struct parameter while the others do not.
Unify the functions, handle the difference by always passing a
da_id_type which is the task's pid for per-task monitor but is
ignored for the other types.
Regardless:
Reviewed-by: Nam Cao <namcao@linutronix.de>
Nam
^ permalink raw reply
* Re: [PATCH 12/12] mm/damon/core: add trace point for damos stat per apply interval
From: SeongJae Park @ 2025-12-17 23:52 UTC (permalink / raw)
To: Steven Rostedt
Cc: SeongJae Park, Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers,
damon, linux-kernel, linux-mm, linux-trace-kernel
In-Reply-To: <20251217174851.28e7c4e5@gandalf.local.home>
On Wed, 17 Dec 2025 17:48:51 -0500 Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 16 Dec 2025 00:01:25 -0800
> SeongJae Park <sj@kernel.org> wrote:
>
> > + TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu "
> > + "nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu "
> > + "qt_exceeds=%lu nr_snapshots=%lu",
>
> Nit, but it's been stated that strings should not be broken up because of
> the column limit.
I also found checkpatch.pl shows warning for that. I was ignoring that since I
was thinking that's just a recommendation. But as I got more than one warning,
now I'd like to fix this.
Andrew, could you please add below attaching fixup patch?
>
> > + __entry->context_idx, __entry->scheme_idx,
> > + __entry->nr_tried, __entry->sz_tried,
> > + __entry->nr_applied, __entry->sz_applied,
> > + __entry->sz_ops_filter_passed, __entry->qt_exceeds,
> > + __entry->nr_snapshots)
> > +);
> > +
> > TRACE_EVENT(damos_esz,
> >
> > TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 8908aec6670f..68dd2f7acba2 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2256,6 +2256,22 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
> > quota->min_score = score;
> > }
> >
> > +static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
> > +{
> > + unsigned int cidx = 0, sidx = 0;
> > + struct damos *siter;
> > +
> > + if (!trace_damos_stat_after_apply_interval_enabled())
> > + return;
>
> Other than that, from a tracing POV:
>
> Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Thank you! :)
Thanks,
SJ
[...]
=== >8 ===
From 67a7762ee1ade258c53913ea4ecf9bafd4746ea9 Mon Sep 17 00:00:00 2001
From: SeongJae Park <sj@kernel.org>
Date: Wed, 17 Dec 2025 15:42:06 -0800
Subject: [PATCH] mm/damon: do not break the string for damos_stat tracepoint
The format string for damos_stat tracepoint is broken up to multiple
lines. Strings shouldn't be broken up due to the column limit, though.
Update the string to be put on a single line.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: SeongJae Park <sj@kernel.org>
---
include/trace/events/damon.h | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
index 24fc402ab3c8..bf25ee07cb48 100644
--- a/include/trace/events/damon.h
+++ b/include/trace/events/damon.h
@@ -40,9 +40,7 @@ TRACE_EVENT(damos_stat_after_apply_interval,
__entry->nr_snapshots = stat->nr_snapshots;
),
- TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu "
- "nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu "
- "qt_exceeds=%lu nr_snapshots=%lu",
+ TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu qt_exceeds=%lu nr_snapshots=%lu",
__entry->context_idx, __entry->scheme_idx,
__entry->nr_tried, __entry->sz_tried,
__entry->nr_applied, __entry->sz_applied,
--
2.47.3
^ permalink raw reply related
* Re: [PATCH 12/12] mm/damon/core: add trace point for damos stat per apply interval
From: Steven Rostedt @ 2025-12-17 22:48 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, Masami Hiramatsu, Mathieu Desnoyers, damon,
linux-kernel, linux-mm, linux-trace-kernel
In-Reply-To: <20251216080128.42991-13-sj@kernel.org>
On Tue, 16 Dec 2025 00:01:25 -0800
SeongJae Park <sj@kernel.org> wrote:
> + TP_printk("ctx_idx=%u scheme_idx=%u nr_tried=%lu sz_tried=%lu "
> + "nr_applied=%lu sz_tried=%lu sz_ops_filter_passed=%lu "
> + "qt_exceeds=%lu nr_snapshots=%lu",
Nit, but it's been stated that strings should not be broken up because of
the column limit.
> + __entry->context_idx, __entry->scheme_idx,
> + __entry->nr_tried, __entry->sz_tried,
> + __entry->nr_applied, __entry->sz_applied,
> + __entry->sz_ops_filter_passed, __entry->qt_exceeds,
> + __entry->nr_snapshots)
> +);
> +
> TRACE_EVENT(damos_esz,
>
> TP_PROTO(unsigned int context_idx, unsigned int scheme_idx,
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 8908aec6670f..68dd2f7acba2 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -2256,6 +2256,22 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
> quota->min_score = score;
> }
>
> +static void damos_trace_stat(struct damon_ctx *c, struct damos *s)
> +{
> + unsigned int cidx = 0, sidx = 0;
> + struct damos *siter;
> +
> + if (!trace_damos_stat_after_apply_interval_enabled())
> + return;
Other than that, from a tracing POV:
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
> +
> + damon_for_each_scheme(siter, c) {
> + if (siter == s)
> + break;
> + sidx++;
> + }
> + trace_damos_stat_after_apply_interval(cidx, sidx, &s->stat);
> +}
> +
> static void kdamond_apply_schemes(struct damon_ctx *c)
> {
> struct damon_target *t;
> @@ -2297,6 +2313,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
> (s->apply_interval_us ? s->apply_interval_us :
> c->attrs.aggr_interval) / sample_interval;
> s->last_applied = NULL;
> + damos_trace_stat(c, s);
> }
> mutex_unlock(&c->walk_control_lock);
> }
^ permalink raw reply
* Re: [PATCH v3] kallsyms: Always initialize modbuildid
From: Steven Rostedt @ 2025-12-17 22:30 UTC (permalink / raw)
To: Maurice Hieronymus
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, georges.aureau,
bpf, linux-kernel, linux-trace-kernel
In-Reply-To: <20251210170347.28053-1-mhi@mailbox.org>
On Wed, 10 Dec 2025 18:03:45 +0100
Maurice Hieronymus <mhi@mailbox.org> wrote:
> include/linux/filter.h | 6 ++++--
> include/linux/ftrace.h | 4 ++--
> kernel/kallsyms.c | 4 ++--
> kernel/trace/ftrace.c | 8 +++++++-
> 4 files changed, 15 insertions(+), 7 deletions(-)
Also split this up into two patches. Then I can take the one for ftrace and
the networking folks can take the bpf update.
-- Steve
^ permalink raw reply
* Re: [PATCH v3] kallsyms: Always initialize modbuildid
From: Steven Rostedt @ 2025-12-17 22:27 UTC (permalink / raw)
To: Maurice Hieronymus
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, georges.aureau,
bpf, linux-kernel, linux-trace-kernel
In-Reply-To: <20251210170347.28053-1-mhi@mailbox.org>
On Wed, 10 Dec 2025 18:03:45 +0100
Maurice Hieronymus <mhi@mailbox.org> wrote:
> @@ -7761,6 +7761,12 @@ ftrace_mod_address_lookup(unsigned long addr, unsigned long *size,
> if (ret) {
> if (modname)
> *modname = mod_map->mod->name;
> + if (modbuildid)
> +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
IS_ENABLED() is for use within C code. This should simply be:
#ifdef CONFIG_STACKTRACE_BUILD_ID
-- Steve
> + *modbuildid = mod_map->mod->build_id;
> +#else
> + *modbuildid = NULL;
> +#endif
> break;
> }
> }
^ permalink raw reply
* Re: [PATCH v2 1/2] mm: vmscan: add cgroup IDs to vmscan tracepoints
From: Steven Rostedt @ 2025-12-17 22:21 UTC (permalink / raw)
To: Thomas Ballasi
Cc: Masami Hiramatsu, Andrew Morton, linux-mm, linux-trace-kernel
In-Reply-To: <20251216140252.11864-2-tballasi@linux.microsoft.com>
On Tue, 16 Dec 2025 06:02:51 -0800
Thomas Ballasi <tballasi@linux.microsoft.com> wrote:
> ---
> include/trace/events/vmscan.h | 65 +++++++++++++++++++++--------------
> mm/vmscan.c | 17 ++++-----
> 2 files changed, 48 insertions(+), 34 deletions(-)
>
> diff --git a/include/trace/events/vmscan.h b/include/trace/events/vmscan.h
> index d2123dd960d59..afc9f80d03f34 100644
> --- a/include/trace/events/vmscan.h
> +++ b/include/trace/events/vmscan.h
> @@ -114,85 +114,92 @@ TRACE_EVENT(mm_vmscan_wakeup_kswapd,
>
> DECLARE_EVENT_CLASS(mm_vmscan_direct_reclaim_begin_template,
>
> - TP_PROTO(int order, gfp_t gfp_flags),
> + TP_PROTO(int order, gfp_t gfp_flags, unsigned short memcg_id),
>
> - TP_ARGS(order, gfp_flags),
> + TP_ARGS(order, gfp_flags, memcg_id),
>
> TP_STRUCT__entry(
> __field( int, order )
> __field( unsigned long, gfp_flags )
> + __field( unsigned short, memcg_id )
> ),
Hmm, the above adds some holes. Note, events are at a minimum, 4 bytes
aligend. On 64bit, they can be 8 byte aligned. Still, above is the same as:
struct {
int order;
unsigned long gfp_flags;
unsigned short memcg_id;
};
See the issue? Perhaps it may be better to add the memcg_id in between the
order and gfp_flags?
-- Steve
^ permalink raw reply
* Re: [PATCH v3 0/7] kallsyms: Prevent invalid access when showing module buildid
From: Andrew Morton @ 2025-12-17 21:10 UTC (permalink / raw)
To: Petr Mladek
Cc: Petr Pavlu, Steven Rostedt, Alexei Starovoitov, Kees Cook,
Aaron Tomlin, Daniel Borkmann, John Fastabend, Masami Hiramatsu,
Mark Rutland, Luis Chamberlain, Daniel Gomez, Sami Tolvanen,
linux-kernel, bpf, linux-modules, linux-trace-kernel
In-Reply-To: <aUFl9n3b8DWnYGyJ@pathway.suse.cz>
On Tue, 16 Dec 2025 15:00:22 +0100 Petr Mladek <pmladek@suse.com> wrote:
> I wonder who could take this patchset.
>
> IMHO, the failed test report is bogus. The system went out of memory.
> Anyway, the info provided by the mail is not enough for debugging.
>
> IMHO. this patchset is ready for linux-next. Unfortunately, kallsyms
> do not have any dedicated maintainer. I though about Kees (hardening)
> or Andrew (core stuff). Or I could take it via printk tree.
I seem to be a usual kallsyms patch monkey so I scooped it up, thanks.
If you or Kees prefer to take it then I'll drop the mm.git copy when I
get notified of the duplication by the linux-next maintainer.
^ permalink raw reply
* Re: [PATCH v3 0/7] kallsyms: Prevent invalid access when showing module buildid
From: Andrew Morton @ 2025-12-17 21:09 UTC (permalink / raw)
To: Petr Mladek
Cc: Petr Pavlu, Steven Rostedt, Alexei Starovoitov, Kees Cook,
Aaron Tomlin, Daniel Borkmann, John Fastabend, Masami Hiramatsu,
Mark Rutland, Luis Chamberlain, Daniel Gomez, Sami Tolvanen,
linux-kernel, bpf, linux-modules, linux-trace-kernel
In-Reply-To: <20251128135920.217303-1-pmladek@suse.com>
On Fri, 28 Nov 2025 14:59:13 +0100 Petr Mladek <pmladek@suse.com> wrote:
> This patchset is cleaning up kallsyms code related to module buildid.
> It is fixing an invalid access when printing backtraces, see [v1] for
> more details:
>
> ...
>
> [v1] https://lore.kernel.org/r/20251105142319.1139183-1-pmladek@suse.com
> [v2] https://lore.kernel.org/r/20251112142003.182062-1-pmladek@suse.com
>
It's best to avoid sending people off to the WWW to understand a
patchset - better that the git history be self-contained. So when
staging this for mm.git I scooped the relevant material from [1] and
added it to your cover letter, as below. Looks OK?
From: Petr Mladek <pmladek@suse.com>
Subject: kallsyms: clean up @namebuf initialization in kallsyms_lookup_buildid()
Date: Fri, 28 Nov 2025 14:59:14 +0100
Patch series "kallsyms: Prevent invalid access when showing module
buildid", v3.
We have seen nested crashes in __sprint_symbol(), see below. They seem to
be caused by an invalid pointer to "buildid". This patchset cleans up
kallsyms code related to module buildid and fixes this invalid access when
printing backtraces.
I made an audit of __sprint_symbol() and found several situations
when the buildid might be wrong:
+ bpf_address_lookup() does not set @modbuildid
+ ftrace_mod_address_lookup() does not set @modbuildid
+ __sprint_symbol() does not take rcu_read_lock and
the related struct module might get removed before
mod->build_id is printed.
This patchset solves these problems:
+ 1st, 2nd patches are preparatory
+ 3rd, 4th, 6th patches fix the above problems
+ 5th patch cleans up a suspicious initialization code.
This is the backtrace, we have seen. But it is not really important.
The problems fixed by the patchset are obvious:
crash64> bt [62/2029]
PID: 136151 TASK: ffff9f6c981d4000 CPU: 367 COMMAND: "btrfs"
#0 [ffffbdb687635c28] machine_kexec at ffffffffb4c845b3
#1 [ffffbdb687635c80] __crash_kexec at ffffffffb4d86a6a
#2 [ffffbdb687635d08] hex_string at ffffffffb51b3b61
#3 [ffffbdb687635d40] crash_kexec at ffffffffb4d87964
#4 [ffffbdb687635d50] oops_end at ffffffffb4c41fc8
#5 [ffffbdb687635d70] do_trap at ffffffffb4c3e49a
#6 [ffffbdb687635db8] do_error_trap at ffffffffb4c3e6a4
#7 [ffffbdb687635df8] exc_stack_segment at ffffffffb5666b33
#8 [ffffbdb687635e20] asm_exc_stack_segment at ffffffffb5800cf9
...
This patch (of 7)
The function kallsyms_lookup_buildid() initializes the given @namebuf by
clearing the first and the last byte. It is not clear why.
The 1st byte makes sense because some callers ignore the return code and
expect that the buffer contains a valid string, for example:
- function_stat_show()
- kallsyms_lookup()
- kallsyms_lookup_buildid()
The initialization of the last byte does not make much sense because it
can later be overwritten. Fortunately, it seems that all called functions
behave correctly:
- kallsyms_expand_symbol() explicitly adds the trailing '\0'
at the end of the function.
- All *__address_lookup() functions either use the safe strscpy()
or they do not touch the buffer at all.
Document the reason for clearing the first byte. And remove the useless
initialization of the last byte.
Link: https://lkml.kernel.org/r/20251128135920.217303-2-pmladek@suse.com
Signed-off-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkman <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: Kees Cook <kees@kernel.org>
Cc: Luis Chamberalin <mcgrof@kernel.org>
Cc: Marc Rutland <mark.rutland@arm.com>
Cc: "Masami Hiramatsu (Google)" <mhiramat@kernel.org>
Cc: Petr Pavlu <petr.pavlu@suse.com>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---
kernel/kallsyms.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--- a/kernel/kallsyms.c~kallsyms-clean-up-namebuf-initialization-in-kallsyms_lookup_buildid
+++ a/kernel/kallsyms.c
@@ -355,7 +355,12 @@ static int kallsyms_lookup_buildid(unsig
{
int ret;
- namebuf[KSYM_NAME_LEN - 1] = 0;
+ /*
+ * kallsyms_lookus() returns pointer to namebuf on success and
+ * NULL on error. But some callers ignore the return value.
+ * Instead they expect @namebuf filled either with valid
+ * or empty string.
+ */
namebuf[0] = 0;
if (is_ksym_addr(addr)) {
_
^ permalink raw reply
* Re: [PATCH] unwind: Show that entries of struct unwind_cache is not bound by nr_entries
From: Steven Rostedt @ 2025-12-17 20:11 UTC (permalink / raw)
To: Kees Cook
Cc: LKML, Linux Trace Kernel, Thorsten Blum, Josh Poimboeuf,
Peter Zijlstra, Gustavo A. R. Silva, David Laight
In-Reply-To: <202511171303.1623D77@keescook>
On Mon, 17 Nov 2025 13:28:59 -0800
Kees Cook <kees@kernel.org> wrote:
> struct unwind_cache {
> struct_group_tagged(unwind_cache_hdr, hdr,
> unsigned long unwind_completed;
> unsigned int nr_entries;
> );
> unsigned long entries[(SZ_4K - sizeof(struct unwind_cache_hdr)) / sizeof(long)];
> };
This may help automated tooling, but it is horrendous to read. I value
readability much higher than static analyzers.
Hence, I'm leaving the code as is, and just keep NAKing patches that try to
add __counted_by() to entries.
-- Steve
>
> #define UNWIND_MAX_ENTRIES ARRAY_SIZE(((struct unwind_cache*)NULL)->entries)
>
> And this checks out for me:
>
> UNWIND_MAX_ENTRIES:510
> sizeof(struct unwind_cache):4096
>
> No hiding things from the compiler, and you can treat "entries" like a
> real array (since it is one now).
^ permalink raw reply
* Re: [PATCH] tracing: Allow perf to read synthetic events
From: Steven Rostedt @ 2025-12-17 19:08 UTC (permalink / raw)
To: Ian Rogers
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Peter Zijlstra
In-Reply-To: <CAP-5=fUFZaDTEDytOVW_R-v+fys11ojuoCbs9c2UQqvxDYUxGg@mail.gmail.com>
On Wed, 17 Dec 2025 10:58:25 -0800
Ian Rogers <irogers@google.com> wrote:
> This is great! Any chance of an example of using it? I'd like to add a test.
I tested it with:
# trace-cmd sqlhist -e -n futex_wait select TIMESTAMP_DELTA_USECS as lat from sys_enter_futex as start join sys_exit_futex as end on start.common_pid = end.common_pid
# perf record -e synthetic:futex_wait
# perf script
bash 1043 [002] 49.501707: synthetic:futex_wait: lat=8
in:imuxsock 610 [005] 58.904344: synthetic:futex_wait: lat=27
rs:main Q:Reg 612 [007] 58.904478: synthetic:futex_wait: lat=10847333
rs:main Q:Reg 612 [007] 58.904507: synthetic:futex_wait: lat=6
ls 1050 [004] 59.283561: synthetic:futex_wait: lat=8
in:imklog 611 [004] 69.084959: synthetic:futex_wait: lat=55
rs:main Q:Reg 612 [007] 69.085014: synthetic:futex_wait: lat=10180378
rs:main Q:Reg 612 [007] 69.085041: synthetic:futex_wait: lat=5
But feel free to use any other synthetic event or perf command.
-- Steve
^ permalink raw reply
* Re: [PATCH] tracing: Allow perf to read synthetic events
From: Ian Rogers @ 2025-12-17 18:58 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Linux Trace Kernel, Masami Hiramatsu, Mathieu Desnoyers,
Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Peter Zijlstra
In-Reply-To: <20251217113920.50b56246@gandalf.local.home>
On Wed, Dec 17, 2025 at 8:37 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: Steven Rostedt <rostedt@goodmis.org>
>
> Currently, perf can not enable synthetic events. When it does, it either
> causes a warning in the kernel or errors with "no such device".
>
> Add the necessary code to allow perf to also attach to synthetic events.
>
> Reported-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
This is great! Any chance of an example of using it? I'd like to add a test.
Thanks,
Ian
> ---
> kernel/trace/trace_events_synth.c | 121 +++++++++++++++++++++++-------
> 1 file changed, 94 insertions(+), 27 deletions(-)
>
> diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
> index 4554c458b78c..026e06f28958 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -493,28 +493,19 @@ static unsigned int trace_stack(struct synth_trace_event *entry,
> return len;
> }
>
> -static notrace void trace_event_raw_event_synth(void *__data,
> - u64 *var_ref_vals,
> - unsigned int *var_ref_idx)
> +static __always_inline int get_field_size(struct synth_event *event,
> + u64 *var_ref_vals,
> + unsigned int *var_ref_idx)
> {
> - unsigned int i, n_u64, val_idx, len, data_size = 0;
> - struct trace_event_file *trace_file = __data;
> - struct synth_trace_event *entry;
> - struct trace_event_buffer fbuffer;
> - struct trace_buffer *buffer;
> - struct synth_event *event;
> - int fields_size = 0;
> -
> - event = trace_file->event_call->data;
> -
> - if (trace_trigger_soft_disabled(trace_file))
> - return;
> + int fields_size;
>
> fields_size = event->n_u64 * sizeof(u64);
>
> - for (i = 0; i < event->n_dynamic_fields; i++) {
> + for (int i = 0; i < event->n_dynamic_fields; i++) {
> unsigned int field_pos = event->dynamic_fields[i]->field_pos;
> char *str_val;
> + int val_idx;
> + int len;
>
> val_idx = var_ref_idx[field_pos];
> str_val = (char *)(long)var_ref_vals[val_idx];
> @@ -529,18 +520,18 @@ static notrace void trace_event_raw_event_synth(void *__data,
>
> fields_size += len;
> }
> + return fields_size;
> +}
>
> - /*
> - * Avoid ring buffer recursion detection, as this event
> - * is being performed within another event.
> - */
> - buffer = trace_file->tr->array_buffer.buffer;
> - guard(ring_buffer_nest)(buffer);
> -
> - entry = trace_event_buffer_reserve(&fbuffer, trace_file,
> - sizeof(*entry) + fields_size);
> - if (!entry)
> - return;
> +static __always_inline void write_synth_entry(struct synth_event *event,
> + struct synth_trace_event *entry,
> + u64 *var_ref_vals,
> + unsigned int *var_ref_idx)
> +{
> + int data_size = 0;
> + int i, n_u64;
> + int val_idx;
> + int len;
>
> for (i = 0, n_u64 = 0; i < event->n_fields; i++) {
> val_idx = var_ref_idx[i];
> @@ -581,10 +572,83 @@ static notrace void trace_event_raw_event_synth(void *__data,
> n_u64++;
> }
> }
> +}
> +
> +static notrace void trace_event_raw_event_synth(void *__data,
> + u64 *var_ref_vals,
> + unsigned int *var_ref_idx)
> +{
> + struct trace_event_file *trace_file = __data;
> + struct synth_trace_event *entry;
> + struct trace_event_buffer fbuffer;
> + struct trace_buffer *buffer;
> + struct synth_event *event;
> + int fields_size;
> +
> + event = trace_file->event_call->data;
> +
> + if (trace_trigger_soft_disabled(trace_file))
> + return;
> +
> + fields_size = get_field_size(event, var_ref_vals, var_ref_idx);
> +
> + /*
> + * Avoid ring buffer recursion detection, as this event
> + * is being performed within another event.
> + */
> + buffer = trace_file->tr->array_buffer.buffer;
> + guard(ring_buffer_nest)(buffer);
> +
> + entry = trace_event_buffer_reserve(&fbuffer, trace_file,
> + sizeof(*entry) + fields_size);
> + if (!entry)
> + return;
> +
> + write_synth_entry(event, entry, var_ref_vals, var_ref_idx);
>
> trace_event_buffer_commit(&fbuffer);
> }
>
> +#ifdef CONFIG_PERF_EVENTS
> +static notrace void perf_event_raw_event_synth(void *__data,
> + u64 *var_ref_vals,
> + unsigned int *var_ref_idx)
> +{
> + struct trace_event_call *call = __data;
> + struct synth_trace_event *entry;
> + struct hlist_head *perf_head;
> + struct synth_event *event;
> + struct pt_regs *regs;
> + int fields_size;
> + size_t size;
> + int context;
> +
> + event = call->data;
> +
> + perf_head = this_cpu_ptr(call->perf_events);
> +
> + if (!perf_head || hlist_empty(perf_head))
> + return;
> +
> + fields_size = get_field_size(event, var_ref_vals, var_ref_idx);
> +
> + size = ALIGN(sizeof(*entry) + fields_size, 8);
> +
> + entry = perf_trace_buf_alloc(size, ®s, &context);
> +
> + if (unlikely(!entry))
> + return;
> +
> + write_synth_entry(event, entry, var_ref_vals, var_ref_idx);
> +
> + perf_fetch_caller_regs(regs);
> +
> + perf_trace_buf_submit(entry, size, context,
> + call->event.type, 1, regs,
> + perf_head, NULL);
> +}
> +#endif
> +
> static void free_synth_event_print_fmt(struct trace_event_call *call)
> {
> if (call) {
> @@ -911,6 +975,9 @@ static int register_synth_event(struct synth_event *event)
> call->flags = TRACE_EVENT_FL_TRACEPOINT;
> call->class->reg = synth_event_reg;
> call->class->probe = trace_event_raw_event_synth;
> +#ifdef CONFIG_PERF_EVENTS
> + call->class->perf_probe = perf_event_raw_event_synth;
> +#endif
> call->data = event;
> call->tp = event->tp;
>
> --
> 2.51.0
>
^ permalink raw reply
* Re: [PATCH v2] tracing: add kernel documentation for trace_array_set_clr_event, trace_set_clr_event and supporting functions
From: Steven Rostedt @ 2025-12-17 18:37 UTC (permalink / raw)
To: Gabriele Paoloni
Cc: mhiramat, mathieu.desnoyers, linux-kernel, linux-trace-kernel
In-Reply-To: <20250814122141.109076-1-gpaoloni@redhat.com>
On Thu, 14 Aug 2025 14:21:41 +0200
Gabriele Paoloni <gpaoloni@redhat.com> wrote:
Hi Gabriele,
As this patch is still sitting in patchwork, and I like the comments, I
want to move it forward. I know we are still discussing posting
requirements, but regardless of that, this has some good information that I
want to add anyway. But the patch itself needs some clean up.
Note, this email is about documenting the functions and has nothing to do
with the discussions about requirements and such.
First, the subject requires some changes. One, the tracing subsystem uses
capitalized subjects. Two, the subject can be shorter:
tracing: Add kerneldoc to set_clr_event() functions
> As per Linux Kernel documentation guidelines
> (https://docs.kernel.org/doc-guide/kernel-doc.html),
> <<Every function that is exported to loadable modules using
> EXPORT_SYMBOL or EXPORT_SYMBOL_GPL should have a kernel-doc
> comment>>;
> hence this patch adds detailed kernel-doc headers documentation
Change logs should be in imperative mode and never say "this patch".
> for trace_array_set_clr_event(), trace_set_clr_event() and the
> main functions in the respective call-trees that support their
> functionalities.
>
> For each of the documented functions, as part of the extensive
> description, a set of "Function's expectations" and "Assumptions
> of Use" are described in a way that facilitate:
>
> 1) evaluating the current code and any proposed modification
> to behave as described;
>
> 2) writing kernel tests to verify the code to behave as described.
>
Actually, the above can simply be replaced with:
Add kernedoc to the following functions:
<list of functions>
> Signed-off-by: Gabriele Paoloni <gpaoloni@redhat.com>
> ---
> Changes from v1:
> - Added "Context:" information
> - Added "Assumptions of Use"
> - __ftrace_event_enable_disable Function's expectations have been
> rewritten to replace the soft mode flag with the soft mode reference
> counter
> - Addressed other comments from https://lore.kernel.org/linux-trace-kernel/20250620085618.4489-1-gpaoloni@redhat.com/T/#u
> ---
> kernel/trace/trace_events.c | 150 ++++++++++++++++++++++++++++++++----
> 1 file changed, 133 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c
> index 9f3e9537417d..9bad5e9669df 100644
> --- a/kernel/trace/trace_events.c
> +++ b/kernel/trace/trace_events.c
> @@ -763,6 +763,58 @@ void trace_event_enable_tgid_record(bool enable)
> } while_for_each_event_file();
> }
>
> +/**
> + * __ftrace_event_enable_disable - enable or disable a trace event
> + * @file: trace event file associated with the event.
> + * @enable: 0 or 1 respectively to disable/enable the event.
> + * @soft_disable: 1 or 0 respectively to mark if the enable parameter IS or
> + * IS NOT a soft enable/disable.
The parameter description should be short oneliners. Any more detail should
be in the body of the kerneldoc. Also, please be consistent. @enable uses
"0 or 1" and @soft_disable uses "1 or 0".
> + *
> + * Function's expectations:
Also, thinking about this particular function. It should describe what the
intent is. Something to the effect of:
This function is used to enable or disable trace events for a given
instance. The @file maps the event to a specific instance. Normally,
this will be called with @soft_disabled = 0, where @enable set to 1 will
enabled the event to start recording, and if @enable is set to 0, will
disable the event. The @enable is a on/off switch, where calling this
function multiple times with @enable = 1 will enable the event the
first time, and the then calling this with @enable = 0 will disable the
event, regardless of the number of times it was called with @enable for
a given @file.
As there are triggers that can enable tracing in a context that cannot
modify kernel text, the triggers when enabled must "soft enable" the
events before it has to enable them in one of those prohibited
contexts. The code would call this function with @soft_disable set
before it needs to enable the event. If the event is not already
enabled, the @file flags SOFT_DISABLED bit is set and the event is
enabled but the callback will see the SOFT_DISABLED set and not do
anything.
When the trigger needs the event to be fully enabled, it only needs to
clear the SOFT_DISABLED flag, which will cause the event to start
recording.
Unlike the @enable flag, the @soft_disable has a counter, as multiple
triggers may need to use it. For every time this is called with
@enable=1 and @soft_disable=1, it requires a @enable=0 and
@soft_disable=1 to fully disable the event.
The above can replace the below.
> + * - If soft_disable is 1 a soft mode reference counter associated with the
> + * trace event shall be increased or decreased according to the enable
> + * parameter being 1 (enable) or 0 (disable) respectively.
> + * If the soft mode reference counter is > 0 before the increase or after
> + * the decrease, no other actions shall be taken.
> + *
> + * - if soft_disable is 1 and the soft mode reference counter is 0 before
> + * the increase or after the decrease, an enable value set to 0 or 1 shall
> + * result in disabling or enabling the use of trace_buffered_event
> + * respectively.
> + *
> + * - If soft_disable is 1 and enable is 0 and the soft mode reference counter
> + * reaches zero and if the soft disabled flag is set (i.e. if the event was
> + * previously enabled with soft_disable = 1), tracing for the trace point
> + * event shall be disabled and the soft disabled flag shall be cleared.
> + *
> + * - If soft_disable is 0 and enable is 0, tracing for the trace point event
> + * shall be disabled only if the soft mode reference counter is 0.
> + * Additionally the soft disabled flag shall be set or cleared according to
> + * the soft mode reference counter being greater than 0 or 0 respectively.
> + *
> + * - If enable is 1, tracing for the trace point event shall be enabled (if
> + * previously disabled); in addition, if soft_disable is 1 and the soft mode
> + * reference counter is 0 before the increase, the soft disabled flag shall
> + * be set.
> + *
> + * - When enabling or disabling tracing for the trace point event
> + * the flags associated with comms and tgids shall be checked and, if set,
> + * respectively tracing of comms and tgdis at sched_switch shall be
> + * enabled/disabled.
> + *
> + * Assumptions of Use:
> + * - for thread-safe execution, event_mutex shall be locked before calling
> + * this function;
> + * - the file input pointer is assumed to be a valid one;
> + * - the enable input parameter shall not be set to any value other than 0
> + * or 1.
> + *
> + * Context: process context.
> + *
> + * Return:
> + * * 0 on success
> + * * any error returned by the event register or unregister callbacks
Note, the error document here should not reference other functions.
Should just say "Negative on error" or something similar.
> + */
The rest can be updated similar.
Are you OK with this, or would you prefer that I make these changes?
-- Steve
> static int __ftrace_event_enable_disable(struct trace_event_file *file,
> int enable, int soft_disable)
> {
> @@ -1296,8 +1348,46 @@ static void remove_event_file_dir(struct trace_event_file *file)
> event_file_put(file);
> }
>
> -/*
> - * __ftrace_set_clr_event(NULL, NULL, NULL, set) will set/unset all events.
> +/**
> + * __ftrace_set_clr_event_nolock - enable or disable an event within a system.
> + * @tr: target trace_array containing the events list.
> + * @match: target system or event name (NULL for any).
> + * @sub: target system name (NULL for any).
> + * @event: target event name (NULL for any).
> + * @set: 1 to enable, 0 to disable.
> + * @mod: target module name (NULL for any).
> + *
> + * Function's expectations:
> + * - If mod is set, the mod name shall be sanitized by replacing all '-' with
> + * '_' to match the modules' naming convention used in the Kernel.
> + *
> + * - From the events' list in the input tr, the ensemble of events to be enabled
> + * or disabled shall be selected according to the input match, sub, event and
> + * mod parameters. Each of these parameters, if set, shall restrict the events
> + * ensemble to those with a matching parameter's name.
> + *
> + * - For each of the selected events the IGNORE_ENABLE flag shall be checked,
> + * and, if not set, ftrace_event_enable_disable shall be invoked passing the
> + * input set parameter to either enable or disable the event.
> + *
> + * Assumptions of Use:
> + * - for thread-safe execution, event_mutex shall be locked before calling
> + * this function;
> + * - the tr input pointer is assumed to be a valid one;
> + * - the set input parameter shall not be set to any value other than 0
> + * or 1.
> + *
> + * NOTE: __ftrace_set_clr_event_nolock(NULL, NULL, NULL, set, NULL) will
> + * set/unset all events.
> + *
> + * Context: process context.
> + *
> + * Return:
> + * * 0 on success
> + * * %-EINVAL - the input parameters do not match any registered event
> + * * %-ENOMEM - memory allocation fails for the module pointer
> + * * any value returned by the first call to ftrace_event_enable_disable that
> + * returned an error
> */
> static int
> __ftrace_set_clr_event_nolock(struct trace_array *tr, const char *match,
> @@ -1440,16 +1530,32 @@ int ftrace_set_clr_event(struct trace_array *tr, char *buf, int set)
> }
>
> /**
> - * trace_set_clr_event - enable or disable an event
> - * @system: system name to match (NULL for any system)
> - * @event: event name to match (NULL for all events, within system)
> - * @set: 1 to enable, 0 to disable
> + * trace_set_clr_event - enable or disable an event within a system.
> + * @system: system name (NULL for any system).
> + * @event: event name (NULL for all events, within system).
> + * @set: 1 to enable, 0 to disable.
> *
> * This is a way for other parts of the kernel to enable or disable
> * event recording.
> *
> - * Returns 0 on success, -EINVAL if the parameters do not match any
> - * registered events.
> + * Function's expectations:
> + * - This function shall retrieve the pointer of the global trace array (global
> + * tracer) and pass it, along the rest of input parameters, to
> + * __ftrace_set_clr_event_nolock.
> + *
> + * - This function shall properly lock/unlock the global event_mutex
> + * before/after invoking ftrace_set_clr_event_nolock.
> + *
> + * Assumptions of Use:
> + * - the set input parameter shall not be set to any value other than 0
> + * or 1.
> + *
> + * Context: process context, locks and unlocks event_mutex.
> + *
> + * Return:
> + * * 0 on success
> + * * %-ENODEV - the global tracer cannot be retrieved
> + * * any other error condition returned by __ftrace_set_clr_event_nolock
> */
> int trace_set_clr_event(const char *system, const char *event, int set)
> {
> @@ -1463,17 +1569,27 @@ int trace_set_clr_event(const char *system, const char *event, int set)
> EXPORT_SYMBOL_GPL(trace_set_clr_event);
>
> /**
> - * trace_array_set_clr_event - enable or disable an event for a trace array.
> - * @tr: concerned trace array.
> - * @system: system name to match (NULL for any system)
> - * @event: event name to match (NULL for all events, within system)
> - * @enable: true to enable, false to disable
> + * trace_array_set_clr_event - enable or disable an event within a system for
> + * a trace array.
> + * @tr: input trace array.
> + * @system: system name (NULL for any system).
> + * @event: event name (NULL for all events, within system).
> + * @enable: true to enable, false to disable.
> *
> - * This is a way for other parts of the kernel to enable or disable
> - * event recording.
> + * This is a way for other parts of the kernel to enable or disable event
> + * recording.
> + *
> + * Function's expectations:
> + * - This function shall properly lock/unlock the global event_mutex
> + * before/after invoking ftrace_set_clr_event_nolock passing along the same
> + * input parameters.
> + *
> + * Context: process context, locks and unlocks event_mutex.
> *
> - * Returns 0 on success, -EINVAL if the parameters do not match any
> - * registered events.
> + * Return:
> + * * 0 on success
> + * * %-ENOENT - the input tr is NULL
> + * * any other error condition returned by __ftrace_set_clr_event_nolock
> */
> int trace_array_set_clr_event(struct trace_array *tr, const char *system,
> const char *event, bool enable)
^ permalink raw reply
* Re: [PATCH 2/4] cgroup: Introduce cgroup_level() helper
From: bot+bpf-ci @ 2025-12-17 16:46 UTC (permalink / raw)
To: mkoutny, linux-block, linux-kernel, cgroups, linux-trace-kernel,
bpf, netfilter-devel, coreteam, netdev
Cc: mkoutny, yukuai, axboe, tj, josef, hannes, rostedt, mhiramat,
mathieu.desnoyers, ast, daniel, andrii, martin.lau, eddyz87, song,
yonghong.song, john.fastabend, kpsingh, sdf, haoluo, jolsa, pablo,
kadlec, fw, phil, davem, edumazet, kuba, pabeni, horms, ast,
andrii, daniel, martin.lau, eddyz87, yonghong.song, clm,
ihor.solodrai
In-Reply-To: <20251217162744.352391-3-mkoutny@suse.com>
[-- Attachment #1: Type: text/plain, Size: 1062 bytes --]
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index a0416927d33d..b4eebe61dca7 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
[ ... ]
> @@ -3003,7 +3003,7 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
> init_waitqueue_head(&iocg->waitq);
> hrtimer_setup(&iocg->waitq_timer, iocg_waitq_timer_fn, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
>
> - iocg->level = blkg->blkcg->css.cgroup->level;
> + iocg->level = cgroup_level(blkg->blkcg->css.cgroup)
^
Is this missing a semicolon? The line should end with a semicolon to
make this valid C syntax. This will prevent the kernel from building
in the ioc_pd_init() function.
>
> for (tblkg = blkg; tblkg; tblkg = tblkg->parent) {
> struct ioc_gq *tiocg = blkg_to_iocg(tblkg);
---
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/20310204023
^ permalink raw reply
* [PATCH] tracing: Allow perf to read synthetic events
From: Steven Rostedt @ 2025-12-17 16:39 UTC (permalink / raw)
To: LKML, Linux Trace Kernel
Cc: Masami Hiramatsu, Mathieu Desnoyers, Ian Rogers,
Arnaldo Carvalho de Melo, Jiri Olsa, Namhyung Kim, Peter Zijlstra
From: Steven Rostedt <rostedt@goodmis.org>
Currently, perf can not enable synthetic events. When it does, it either
causes a warning in the kernel or errors with "no such device".
Add the necessary code to allow perf to also attach to synthetic events.
Reported-by: Ian Rogers <irogers@google.com>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
kernel/trace/trace_events_synth.c | 121 +++++++++++++++++++++++-------
1 file changed, 94 insertions(+), 27 deletions(-)
diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c
index 4554c458b78c..026e06f28958 100644
--- a/kernel/trace/trace_events_synth.c
+++ b/kernel/trace/trace_events_synth.c
@@ -493,28 +493,19 @@ static unsigned int trace_stack(struct synth_trace_event *entry,
return len;
}
-static notrace void trace_event_raw_event_synth(void *__data,
- u64 *var_ref_vals,
- unsigned int *var_ref_idx)
+static __always_inline int get_field_size(struct synth_event *event,
+ u64 *var_ref_vals,
+ unsigned int *var_ref_idx)
{
- unsigned int i, n_u64, val_idx, len, data_size = 0;
- struct trace_event_file *trace_file = __data;
- struct synth_trace_event *entry;
- struct trace_event_buffer fbuffer;
- struct trace_buffer *buffer;
- struct synth_event *event;
- int fields_size = 0;
-
- event = trace_file->event_call->data;
-
- if (trace_trigger_soft_disabled(trace_file))
- return;
+ int fields_size;
fields_size = event->n_u64 * sizeof(u64);
- for (i = 0; i < event->n_dynamic_fields; i++) {
+ for (int i = 0; i < event->n_dynamic_fields; i++) {
unsigned int field_pos = event->dynamic_fields[i]->field_pos;
char *str_val;
+ int val_idx;
+ int len;
val_idx = var_ref_idx[field_pos];
str_val = (char *)(long)var_ref_vals[val_idx];
@@ -529,18 +520,18 @@ static notrace void trace_event_raw_event_synth(void *__data,
fields_size += len;
}
+ return fields_size;
+}
- /*
- * Avoid ring buffer recursion detection, as this event
- * is being performed within another event.
- */
- buffer = trace_file->tr->array_buffer.buffer;
- guard(ring_buffer_nest)(buffer);
-
- entry = trace_event_buffer_reserve(&fbuffer, trace_file,
- sizeof(*entry) + fields_size);
- if (!entry)
- return;
+static __always_inline void write_synth_entry(struct synth_event *event,
+ struct synth_trace_event *entry,
+ u64 *var_ref_vals,
+ unsigned int *var_ref_idx)
+{
+ int data_size = 0;
+ int i, n_u64;
+ int val_idx;
+ int len;
for (i = 0, n_u64 = 0; i < event->n_fields; i++) {
val_idx = var_ref_idx[i];
@@ -581,10 +572,83 @@ static notrace void trace_event_raw_event_synth(void *__data,
n_u64++;
}
}
+}
+
+static notrace void trace_event_raw_event_synth(void *__data,
+ u64 *var_ref_vals,
+ unsigned int *var_ref_idx)
+{
+ struct trace_event_file *trace_file = __data;
+ struct synth_trace_event *entry;
+ struct trace_event_buffer fbuffer;
+ struct trace_buffer *buffer;
+ struct synth_event *event;
+ int fields_size;
+
+ event = trace_file->event_call->data;
+
+ if (trace_trigger_soft_disabled(trace_file))
+ return;
+
+ fields_size = get_field_size(event, var_ref_vals, var_ref_idx);
+
+ /*
+ * Avoid ring buffer recursion detection, as this event
+ * is being performed within another event.
+ */
+ buffer = trace_file->tr->array_buffer.buffer;
+ guard(ring_buffer_nest)(buffer);
+
+ entry = trace_event_buffer_reserve(&fbuffer, trace_file,
+ sizeof(*entry) + fields_size);
+ if (!entry)
+ return;
+
+ write_synth_entry(event, entry, var_ref_vals, var_ref_idx);
trace_event_buffer_commit(&fbuffer);
}
+#ifdef CONFIG_PERF_EVENTS
+static notrace void perf_event_raw_event_synth(void *__data,
+ u64 *var_ref_vals,
+ unsigned int *var_ref_idx)
+{
+ struct trace_event_call *call = __data;
+ struct synth_trace_event *entry;
+ struct hlist_head *perf_head;
+ struct synth_event *event;
+ struct pt_regs *regs;
+ int fields_size;
+ size_t size;
+ int context;
+
+ event = call->data;
+
+ perf_head = this_cpu_ptr(call->perf_events);
+
+ if (!perf_head || hlist_empty(perf_head))
+ return;
+
+ fields_size = get_field_size(event, var_ref_vals, var_ref_idx);
+
+ size = ALIGN(sizeof(*entry) + fields_size, 8);
+
+ entry = perf_trace_buf_alloc(size, ®s, &context);
+
+ if (unlikely(!entry))
+ return;
+
+ write_synth_entry(event, entry, var_ref_vals, var_ref_idx);
+
+ perf_fetch_caller_regs(regs);
+
+ perf_trace_buf_submit(entry, size, context,
+ call->event.type, 1, regs,
+ perf_head, NULL);
+}
+#endif
+
static void free_synth_event_print_fmt(struct trace_event_call *call)
{
if (call) {
@@ -911,6 +975,9 @@ static int register_synth_event(struct synth_event *event)
call->flags = TRACE_EVENT_FL_TRACEPOINT;
call->class->reg = synth_event_reg;
call->class->probe = trace_event_raw_event_synth;
+#ifdef CONFIG_PERF_EVENTS
+ call->class->perf_probe = perf_event_raw_event_synth;
+#endif
call->data = event;
call->tp = event->tp;
--
2.51.0
^ permalink raw reply related
* [PATCH 2/4] cgroup: Introduce cgroup_level() helper
From: Michal Koutný @ 2025-12-17 16:27 UTC (permalink / raw)
To: linux-block, linux-kernel, cgroups, linux-trace-kernel, bpf,
netfilter-devel, coreteam, netdev
Cc: Michal Koutný, Yu Kuai, Jens Axboe, Tejun Heo, Josef Bacik,
Johannes Weiner, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Stanislav Fomichev,
Hao Luo, Jiri Olsa, Pablo Neira Ayuso, Jozsef Kadlecsik,
Florian Westphal, Phil Sutter, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Simon Horman
In-Reply-To: <20251217162744.352391-1-mkoutny@suse.com>
This is a no functional change to hide physical storage of cgroup's
level and it allows subesequent conversion of the storage.
Signed-off-by: Michal Koutný <mkoutny@suse.com>
---
block/bfq-iosched.c | 2 +-
block/blk-iocost.c | 4 ++--
include/linux/cgroup.h | 18 +++++++++++++++---
include/trace/events/cgroup.h | 8 ++++----
kernel/bpf/helpers.c | 2 +-
kernel/cgroup/cgroup.c | 4 ++--
net/netfilter/nft_socket.c | 2 +-
7 files changed, 26 insertions(+), 14 deletions(-)
diff --git a/block/bfq-iosched.c b/block/bfq-iosched.c
index 4a8d3d96bfe49..f293bab068274 100644
--- a/block/bfq-iosched.c
+++ b/block/bfq-iosched.c
@@ -601,7 +601,7 @@ static bool bfqq_request_over_limit(struct bfq_data *bfqd,
goto out;
/* +1 for bfqq entity, root cgroup not included */
- depth = bfqg_to_blkg(bfqq_group(bfqq))->blkcg->css.cgroup->level + 1;
+ depth = cgroup_level(bfqg_to_blkg(bfqq_group(bfqq))->blkcg->css.cgroup) + 1;
if (depth > alloc_depth) {
spin_unlock_irq(&bfqd->lock);
if (entities != inline_entities)
diff --git a/block/blk-iocost.c b/block/blk-iocost.c
index a0416927d33dc..b4eebe61dca7f 100644
--- a/block/blk-iocost.c
+++ b/block/blk-iocost.c
@@ -2962,7 +2962,7 @@ static void ioc_cpd_free(struct blkcg_policy_data *cpd)
static struct blkg_policy_data *ioc_pd_alloc(struct gendisk *disk,
struct blkcg *blkcg, gfp_t gfp)
{
- int levels = blkcg->css.cgroup->level + 1;
+ int levels = cgroup_level(blkcg->css.cgroup) + 1;
struct ioc_gq *iocg;
iocg = kzalloc_node(struct_size(iocg, ancestors, levels), gfp,
@@ -3003,7 +3003,7 @@ static void ioc_pd_init(struct blkg_policy_data *pd)
init_waitqueue_head(&iocg->waitq);
hrtimer_setup(&iocg->waitq_timer, iocg_waitq_timer_fn, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
- iocg->level = blkg->blkcg->css.cgroup->level;
+ iocg->level = cgroup_level(blkg->blkcg->css.cgroup)
for (tblkg = blkg; tblkg; tblkg = tblkg->parent) {
struct ioc_gq *tiocg = blkg_to_iocg(tblkg);
diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index bc892e3b37eea..0290878ebad26 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -525,6 +525,18 @@ static inline struct cgroup *cgroup_parent(struct cgroup *cgrp)
return NULL;
}
+/**
+ * cgroup_level - cgroup depth
+ * @cgrp: cgroup
+ *
+ * The depth this cgroup is at. The root is at depth zero and each step down
+ * the hierarchy increments the level.
+ */
+static inline int cgroup_level(struct cgroup *cgrp)
+{
+ return cgrp->level;
+}
+
/**
* cgroup_is_descendant - test ancestry
* @cgrp: the cgroup to be tested
@@ -537,9 +549,9 @@ static inline struct cgroup *cgroup_parent(struct cgroup *cgrp)
static inline bool cgroup_is_descendant(struct cgroup *cgrp,
struct cgroup *ancestor)
{
- if (cgrp->root != ancestor->root || cgrp->level < ancestor->level)
+ if (cgrp->root != ancestor->root || cgroup_level(cgrp) < cgroup_level(ancestor))
return false;
- return cgrp->ancestors[ancestor->level] == ancestor;
+ return cgrp->ancestors[cgroup_level(ancestor)] == ancestor;
}
/**
@@ -556,7 +568,7 @@ static inline bool cgroup_is_descendant(struct cgroup *cgrp,
static inline struct cgroup *cgroup_ancestor(struct cgroup *cgrp,
int ancestor_level)
{
- if (ancestor_level < 0 || ancestor_level > cgrp->level)
+ if (ancestor_level < 0 || ancestor_level > cgroup_level(cgrp))
return NULL;
return cgrp->ancestors[ancestor_level];
}
diff --git a/include/trace/events/cgroup.h b/include/trace/events/cgroup.h
index ba9229af9a343..0a1bc91754b5e 100644
--- a/include/trace/events/cgroup.h
+++ b/include/trace/events/cgroup.h
@@ -67,7 +67,7 @@ DECLARE_EVENT_CLASS(cgroup,
TP_fast_assign(
__entry->root = cgrp->root->hierarchy_id;
__entry->id = cgroup_id(cgrp);
- __entry->level = cgrp->level;
+ __entry->level = cgroup_level(cgrp);
__assign_str(path);
),
@@ -136,7 +136,7 @@ DECLARE_EVENT_CLASS(cgroup_migrate,
TP_fast_assign(
__entry->dst_root = dst_cgrp->root->hierarchy_id;
__entry->dst_id = cgroup_id(dst_cgrp);
- __entry->dst_level = dst_cgrp->level;
+ __entry->dst_level = cgroup_level(dst_cgrp);
__assign_str(dst_path);
__entry->pid = task->pid;
__assign_str(comm);
@@ -180,7 +180,7 @@ DECLARE_EVENT_CLASS(cgroup_event,
TP_fast_assign(
__entry->root = cgrp->root->hierarchy_id;
__entry->id = cgroup_id(cgrp);
- __entry->level = cgrp->level;
+ __entry->level = cgroup_level(cgrp);
__assign_str(path);
__entry->val = val;
),
@@ -221,7 +221,7 @@ DECLARE_EVENT_CLASS(cgroup_rstat,
TP_fast_assign(
__entry->root = cgrp->root->hierarchy_id;
__entry->id = cgroup_id(cgrp);
- __entry->level = cgrp->level;
+ __entry->level = cgroup_level(cgrp);
__entry->cpu = cpu;
__entry->contended = contended;
),
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index db72b96f9c8c8..b825f6e0a1c29 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -2577,7 +2577,7 @@ __bpf_kfunc struct cgroup *bpf_cgroup_ancestor(struct cgroup *cgrp, int level)
{
struct cgroup *ancestor;
- if (level > cgrp->level || level < 0)
+ if (level > cgroup_level(cgrp) || level < 0)
return NULL;
/* cgrp's refcnt could be 0 here, but ancestors can still be accessed */
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 554a02ee298ba..e011f1dd6d87f 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -5843,7 +5843,7 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
struct cgroup_root *root = parent->root;
struct cgroup *cgrp, *tcgrp;
struct kernfs_node *kn;
- int i, level = parent->level + 1;
+ int i, level = cgroup_level(parent) + 1;
int ret;
/* allocate the cgroup and its ID, 0 is reserved for the root */
@@ -5884,7 +5884,7 @@ static struct cgroup *cgroup_create(struct cgroup *parent, const char *name,
goto out_stat_exit;
for (tcgrp = cgrp; tcgrp; tcgrp = cgroup_parent(tcgrp))
- cgrp->ancestors[tcgrp->level] = tcgrp;
+ cgrp->ancestors[cgroup_level(tcgrp)] = tcgrp;
/*
* New cgroup inherits effective freeze counter, and
diff --git a/net/netfilter/nft_socket.c b/net/netfilter/nft_socket.c
index 36affbb697c2f..a5b0340924efb 100644
--- a/net/netfilter/nft_socket.c
+++ b/net/netfilter/nft_socket.c
@@ -64,7 +64,7 @@ static noinline int nft_socket_cgroup_subtree_level(void)
if (IS_ERR(cgrp))
return PTR_ERR(cgrp);
- level = cgrp->level;
+ level = cgroup_level(cgrp);
cgroup_put(cgrp);
--
2.52.0
^ permalink raw reply related
* [PATCH 0/4] Use __counted_by for ancestor arrays
From: Michal Koutný @ 2025-12-17 16:27 UTC (permalink / raw)
To: linux-block, bpf, linux-trace-kernel, netfilter-devel, netdev,
coreteam, linux-hardening, linux-kernel, cgroups
Cc: Michal Koutný, Hao Luo, Mathieu Desnoyers,
Alexei Starovoitov, Phil Sutter, Yonghong Song, Jens Axboe,
Jozsef Kadlecsik, Steven Rostedt, Martin KaFai Lau, KP Singh,
Eric Dumazet, Florian Westphal, Jiri Olsa, Stanislav Fomichev,
Song Liu, David S. Miller, Simon Horman, John Fastabend,
Johannes Weiner, Daniel Borkmann, Andrii Nakryiko, Tejun Heo,
Josef Bacik, Paolo Abeni, Gustavo A. R. Silva, Pablo Neira Ayuso,
Eduard Zingerman, Yu Kuai, Masami Hiramatsu, Kees Cook,
Jakub Kicinski
The trick with utilizing space in cgroup_root for cgroup::ancetors flex
array was an obstacle for kernel reworks for
-Wflex-array-member-not-at-end.
The first patch fixes that, then I wanted to utilize __counted_by for
this flex array which required some more rework how cgroup level is
evaluated.
Similar flex array is also in struct ioc_gq where it was tempting to
simply use __counted_by(level), however, this would be off-by-one as it
has semantics like cgroup's level (0 == root).
Proper adjustment for __counted_by() would either need similar
level/ancestor helpers or abstracted macros for ancestors up/down
iterations.
I only made a simple comment fixup since I'm not sure about benefit of
__counted_by for structs that aren't sized based on direct user input.
Michal Koutný (4):
cgroup: Eliminate cgrp_ancestor_storage in cgroup_root
cgroup: Introduce cgroup_level() helper
cgroup: Use __counted_by for cgroup::ancestors
blk-iocost: Correct comment ioc_gq::level
block/bfq-iosched.c | 2 +-
block/blk-iocost.c | 6 ++---
include/linux/cgroup-defs.h | 43 +++++++++++++++++++----------------
include/linux/cgroup.h | 18 ++++++++++++---
include/trace/events/cgroup.h | 8 +++----
kernel/bpf/helpers.c | 2 +-
kernel/cgroup/cgroup.c | 9 ++++----
net/netfilter/nft_socket.c | 2 +-
8 files changed, 53 insertions(+), 37 deletions(-)
base-commit: 8f0b4cce4481fb22653697cced8d0d04027cb1e8
--
2.52.0
^ permalink raw reply
* Re: [PATCH v6 4/6] perf script: Display PERF_RECORD_CALLCHAIN_DEFERRED
From: Namhyung Kim @ 2025-12-17 16:00 UTC (permalink / raw)
To: Jens Remus
Cc: Arnaldo Carvalho de Melo, Ian Rogers, James Clark, Jiri Olsa,
Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Steven Rostedt, Josh Poimboeuf, Indu Bhagat,
Mathieu Desnoyers, linux-trace-kernel, bpf, Heiko Carstens,
Vasily Gorbik
In-Reply-To: <024b7bb4-731e-4da4-8480-4789f5912977@linux.ibm.com>
Hello!
On Tue, Dec 16, 2025 at 10:29:28AM +0100, Jens Remus wrote:
> Hello Namhyung!
>
> On 12/16/2025 5:48 AM, Namhyung Kim wrote:
> > On Fri, Dec 12, 2025 at 01:11:38PM +0100, Jens Remus wrote:
>
> >> following is an observation from my attempt to enable unwind user fp on
> >> s390 using s390 back chain instead of frame pointer and relaxing the
> >> s390-specific IP validation check.
> >>
> >> When capturing call graphs of a Java application the list of "unwound"
> >> user space IPs may contain invalid entries, such as 0x0, 0xdeaddeaf,
> >> and 0xffffffffffffff. IPs that exceed PERF_CONTEXT_MAX, such as the
> >> latter, cause perf not to display any deferred (or merged) call chain.
> >> Note that this is not caused by your patch series.
> >
> > Right, it's not a real IP so perf ABI treats them as a magic context.
> >
> >>
> >> While re-adding the s390-specific IP checks would "hide" those, I found
> >> that the call graphs look good otherwise. That is the back chain seems
> >> to be intact. It is just the user space application (e.g. Java JRE) not
> >> correctly adhering to the ABI and saving the return address to the
> >> specified location on the stack, causing bogus IPs to be reported.
> >>
> >> Could perf be improved to handle those user space IPs that exceed
> >> PERF_CONTEXT_MAX?
> >
> > Ideally we should not have them in the first place. Is it a JRE issue
> > or your s390 unwinder issue? Is it possible to ignore them in the
> > unwinder?
>
> Stack tracing using frame pointer is virtually impossible on s390, as
> the ABI does not designate a specific register as FP register, does not
> specify a fixed register save area layout, nor does mandate a FP to be
> setup early. Compilers usually setup a FP late, that is after static
> stack allocation.
>
> An alternative is the s390-specific back chain, which is basically a
> frame pointer on stack. The ABI specifics that *(SP+0) has the pointer
> to the previous frame and *(BC-48) has the return address (RA), if a
> back chain is used (e.g. compiler option -mbackchain is used). This is
> why I implemented unwind user fp on s390 using back chain. Note that
> the back chain can be correctly followed, even if the saved RAs are
> bogus. That is what can be observed in case of this specific Java JRE.
> Apparently it correctly maintains the back chain stack slot, but does
> not correctly maintain the RA stack slot. So the RA stack save slot may
> contain any random value.
Thanks for the explanation.
>
> The s390-implementation of unwind user fp could check whether the return
> address is a valid IP. This is how it is implemented in the existing
> stack tracer in arch/s390/kernel/stacktrace.c:
>
> static inline bool ip_invalid(unsigned long ip)
> {
> /* ABI requires IPs to be 2-byte aligned */
> if (ip & 1)
> return true;
> if (ip < mmap_min_addr)
> return true;
> if (ip >= current->mm->context.asce_limit)
> return true;
> return false;
> }
>
> It could then either stop or return some magic value
> (e.g. PERF_CONTEXT_MAX - 1) to indicate that the IP is invalid and
> continue. Actually I would prefer to continue so that a user an see
> that there is something odd with the stack trace.
Agreed.
>
> Alternatively such a check could possibly also be implemented in the
> common undwind user, if the address space limits are known in common
> code, or as an architecture-specific hook. In general I tend to at
> least add a check whether the IP is zero, as this is used on several
> architectures as indication for outermost frames (usually in
> combination with a FP of zero).
Not sure about the address space limits across archs. It'd be easier if
the kernel returns any invalid value like PERF_CONTEXT_MAX - 1 or simply
0. :)
>
> >>
> >> Is there otherwise guidance how unwind user and/or the s390
> >> implementation should deal with such IPs? Should it stop taking the
> >> deferred calltrace? Should it substitute those with e.g 0, so that
> >> perf can display them?
>
>
> >> Sample for IP == ffffffffffffff (perf does not display any call chain):
> ...
> >> # perf report -D
> >> ...
> >> 44004346257 0x17718 [0x40]: PERF_RECORD_SAMPLE(IP, 0x2): 1082/1084: 0x3ffa3e413aa period: 1001001 addr: 0
> >> ... FP chain: nr:2
> >> ..... 0: fffffffffffffd80
> >> ..... 1: 0000000400000079
> >> ...... (deferred)
> >> ... thread: java:1084
> >> ...... dso: /tmp/perf-1082.map
> >>
> >> 0x17758@perf.data [0xd0]: event: 22
> >> .
> >> . ... raw event: size 208 bytes
> ...
> >>
> >> 44004348864 0x17758 [0xd0]: PERF_RECORD_CALLCHAIN_DEFERRED(IP, 0x2): 1082/1084: 0x400000079
> >> ... FP chain: nr:21
> >> ..... 0: 000003ffa3e413aa
> >> ..... 1: 000003ff3809e2d0
> >> ..... 2: 000003ff3809e130
> >> ..... 3: 000003ffb95fdf68
> >> ..... 4: 0000000000000000
> >> ..... 5: 000003ffb95fe128
> >> ..... 6: 000003ffb95fe1d0
> >> ..... 7: 005780888e7647a5
> >> ..... 8: 000003ffa3e437f2
> >> ..... 9: ffffffffffffffff <-- !
> >> ..... 10: 000003ffa3e4a1fc
> >> ..... 11: 0000000000000000
> >> ..... 12: 000003ffa3e37900
> >> ..... 13: 000003ffa3e41080
> >> ..... 14: 000003ffb9dd11de
> >> ..... 15: 000003ffb9e8df92
> >> ..... 16: 000003ffb9e90e86
> >> ..... 17: 000003ffbab8b07e
> >> ..... 18: 000003ffbab8e040
> >> ..... 19: 000003ffba8abbd8
> >> ..... 20: 000003ffba92b950
> >> : unhandled!
> >>
> >> ...
> >> [next entry]
> >>
> >>
> >> On 11/21/2025 12:48 AM, Namhyung Kim wrote:
>
> >>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> >>
> >>> +static int process_deferred_sample_event(const struct perf_tool *tool,
> >>> + union perf_event *event,
> >>> + struct perf_sample *sample,
> >>> + struct evsel *evsel,
> >>> + struct machine *machine)
> >>> +{
> >>
> >>> + perf_sample__fprintf_start(scr, sample, al.thread, evsel,
> >>> + PERF_RECORD_CALLCHAIN_DEFERRED, fp);
> >>> + fprintf(fp, "DEFERRED CALLCHAIN [cookie: %llx]",
> >>> + (unsigned long long)event->callchain_deferred.cookie);
> >>> +
> >>> + if (PRINT_FIELD(IP)) {
> >>> + struct callchain_cursor *cursor = NULL;
> >>> +
> >>> + if (symbol_conf.use_callchain && sample->callchain) {
> >>> + cursor = get_tls_callchain_cursor();
> >>> + if (thread__resolve_callchain(al.thread, cursor, evsel,
> >>> + sample, NULL, NULL,
> >>> + scripting_max_stack)) {
> >>
> >> thread__resolve_callchain()
> >> calls __thread__resolve_callchain()
> >> calls thread__resolve_callchain_sample():
> >>
> >> for (i = first_call, nr_entries = 0;
> >> i < chain_nr && nr_entries < max_stack; i++) {
> >> ...
> >> ip = chain->ips[j];
> >> if (ip < PERF_CONTEXT_MAX) <-- IP=ff..ff is greater than PERF_CONTEXT_MAX
> >> ++nr_entries;
> >
> > Right.
> >
> >> ...
> >> err = add_callchain_ip(thread, cursor, parent,
> >> root_al, &cpumode, ip,
> >> false, NULL, NULL, 0, symbols);
> >>
> >> if (err)
> >> return (err < 0) ? err : 0;
> >>
> >> calls add_callchain_ip:
> >>
> >> if (ip >= PERF_CONTEXT_MAX) {
> >> switch (ip) {
> >> case PERF_CONTEXT_HV:
> >> *cpumode = PERF_RECORD_MISC_HYPERVISOR;
> >> break;
> >> case PERF_CONTEXT_KERNEL:
> >> *cpumode = PERF_RECORD_MISC_KERNEL;
> >> break;
> >> case PERF_CONTEXT_USER:
> >> case PERF_CONTEXT_USER_DEFERRED:
> >> *cpumode = PERF_RECORD_MISC_USER;
> >> break;
> >> default:
> >> pr_debug("invalid callchain context: " <-- IP=ff..ff reaches default case
> >> "%"PRId64"\n", (s64) ip);
> >
> > We may skip -1 if it's Java and *cpumode is already USER and it's s390.
> > But I'd like to understand the situation first.
>
> Let's better not add any weird architecture-specific handling. This is
> also not limited to -1 (and 0), as Java may have used the stack save
> area in any way, so it may be any random value.
I see. Thanks again for your explanation.
Namhyung
>
> >> /*
> >> * It seems the callchain is corrupted.
> >> * Discard all.
> >> */
> >> callchain_cursor_reset(cursor);
> >> err = 1;
> >> goto out;
> >> }
> >>
> >>> + pr_info("cannot resolve deferred callchains\n");
> >>> + cursor = NULL;
> >>> + }
> >>> + }
> >>> +
> >>> + fputc(cursor ? '\n' : ' ', fp);
> >>> + sample__fprintf_sym(sample, &al, 0, output[type].print_ip_opts,
> >>> + cursor, symbol_conf.bt_stop_list, fp);
> >>> + }
>
> Thanks and regards,
> Jens
> --
> Jens Remus
> Linux on Z Development (D3303)
> +49-7031-16-1128 Office
> jremus@de.ibm.com
>
> IBM
>
> IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
> IBM Data Privacy Statement: https://www.ibm.com/privacy/
>
^ permalink raw reply
* Re: [PATCH v7] dma-buf: add some tracepoints to debug.
From: Steven Rostedt @ 2025-12-17 15:52 UTC (permalink / raw)
To: Xiang Gao
Cc: sumit.semwal, christian.koenig, mhiramat, linux-media, dri-devel,
linux-kernel, mathieu.desnoyers, dhowells, kuba, brauner, akpm,
linux-trace-kernel, gaoxiang17
In-Reply-To: <20251217105132.643300-1-gxxa03070307@gmail.com>
On Wed, 17 Dec 2025 18:51:32 +0800
Xiang Gao <gxxa03070307@gmail.com> wrote:
> From: gaoxiang17 <gaoxiang17@xiaomi.com>
>
> Since we can only inspect dmabuf by iterating over process FDs or the
> dmabuf_list, we need to add our own tracepoints to track its status in
> real time in production.
>
> For example:
> binder:3016_1-3102 [006] ...1. 255.126521: dma_buf_export: exp_name=qcom,system size=12685312 ino=2738
> binder:3016_1-3102 [006] ...1. 255.126528: dma_buf_fd: exp_name=qcom,system size=12685312 ino=2738 fd=8
> binder:3016_1-3102 [006] ...1. 255.126642: dma_buf_mmap_internal: exp_name=qcom,system size=28672 ino=2739
> kworker/6:1-86 [006] ...1. 255.127194: dma_buf_put: exp_name=qcom,system size=12685312 ino=2738
> RenderThread-9293 [006] ...1. 316.618179: dma_buf_get: exp_name=qcom,system size=12771328 ino=2762 fd=176
> RenderThread-9293 [006] ...1. 316.618195: dma_buf_dynamic_attach: exp_name=qcom,system size=12771328 ino=2762 attachment:ffffff880a18dd00 is_dynamic=0 dev_name=kgsl-3d0
> RenderThread-9293 [006] ...1. 318.878220: dma_buf_detach: exp_name=qcom,system size=12771328 ino=2762 attachment:ffffff880a18dd00 is_dynamic=0 dev_name=kgsl-3d0
>
> Signed-off-by: Xiang Gao <gaoxiang17@xiaomi.com>
> ---
> drivers/dma-buf/dma-buf.c | 42 ++++++++-
> include/trace/events/dma_buf.h | 157 +++++++++++++++++++++++++++++++++
> 2 files changed, 198 insertions(+), 1 deletion(-)
> create mode 100644 include/trace/events/dma_buf.h
>
> diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
> index 2bcf9ceca997..ce39bc19e13f 100644
> --- a/drivers/dma-buf/dma-buf.c
> +++ b/drivers/dma-buf/dma-buf.c
> @@ -35,6 +35,25 @@
>
> #include "dma-buf-sysfs-stats.h"
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/dma_buf.h>
> +
> +/*
> + * dmabuf->name must be accessed with holding dmabuf->name_lock.
> + * we need to take the lock around the tracepoint call itself where
> + * it is called in the code.
> + *
> + * Note: FUNC##_enabled() is a static branch that will only
> + * be set when the trace event is enabled.
> + */
Much better.
> +#define DMA_BUF_TRACE(FUNC, ...) \
> + do { \
> + if (FUNC##_enabled()) { \
> + guard(spinlock)(&dmabuf->name_lock); \
> + FUNC(__VA_ARGS__); \
> + } \
Hmm, I wonder if we should also add:
} else if (IS_ENABLED(CONFIG_LOCKDEP)) { \
/* Expose this lock when lockdep is enabled */ \
guard(spinlock)(&dmabuf->name_lock); \
}
This way, if there is any issue taking the lock, lockdep will flag it
without having to enable the tracepoint.
When LOCKDEP is not configured, the compiler should remove that block.
> + } while (0)
> +
> static inline int is_dma_buf_file(struct file *);
>
>
> --- /dev/null
> +++ b/include/trace/events/dma_buf.h
> @@ -0,0 +1,157 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM dma_buf
> +
> +#if !defined(_TRACE_DMA_BUF_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_DMA_BUF_H
> +
> +#include <linux/dma-buf.h>
> +#include <linux/tracepoint.h>
> +
> +DECLARE_EVENT_CLASS(dma_buf,
> +
> + TP_PROTO(struct dma_buf *dmabuf),
> +
> + TP_ARGS(dmabuf),
> +
> + TP_STRUCT__entry(
> + __string(exp_name, dmabuf->exp_name)
> + __field(size_t, size)
> + __field(ino_t, ino)
> + ),
> +
> + TP_fast_assign(
> + __assign_str(exp_name);
> + __entry->size = dmabuf->size;
> + __entry->ino = dmabuf->file->f_inode->i_ino;
> + ),
> +
> + TP_printk("exp_name=%s size=%zu ino=%lu",
> + __get_str(exp_name),
> + __entry->size,
> + __entry->ino)
> +);
For the rest of the patch, from a tracing point of view:
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
-- Steve
^ permalink raw reply
* [PATCH v7] dma-buf: add some tracepoints to debug.
From: Xiang Gao @ 2025-12-17 10:51 UTC (permalink / raw)
To: sumit.semwal, christian.koenig, rostedt, mhiramat
Cc: linux-media, dri-devel, linux-kernel, mathieu.desnoyers, dhowells,
kuba, brauner, akpm, linux-trace-kernel, gaoxiang17
From: gaoxiang17 <gaoxiang17@xiaomi.com>
Since we can only inspect dmabuf by iterating over process FDs or the
dmabuf_list, we need to add our own tracepoints to track its status in
real time in production.
For example:
binder:3016_1-3102 [006] ...1. 255.126521: dma_buf_export: exp_name=qcom,system size=12685312 ino=2738
binder:3016_1-3102 [006] ...1. 255.126528: dma_buf_fd: exp_name=qcom,system size=12685312 ino=2738 fd=8
binder:3016_1-3102 [006] ...1. 255.126642: dma_buf_mmap_internal: exp_name=qcom,system size=28672 ino=2739
kworker/6:1-86 [006] ...1. 255.127194: dma_buf_put: exp_name=qcom,system size=12685312 ino=2738
RenderThread-9293 [006] ...1. 316.618179: dma_buf_get: exp_name=qcom,system size=12771328 ino=2762 fd=176
RenderThread-9293 [006] ...1. 316.618195: dma_buf_dynamic_attach: exp_name=qcom,system size=12771328 ino=2762 attachment:ffffff880a18dd00 is_dynamic=0 dev_name=kgsl-3d0
RenderThread-9293 [006] ...1. 318.878220: dma_buf_detach: exp_name=qcom,system size=12771328 ino=2762 attachment:ffffff880a18dd00 is_dynamic=0 dev_name=kgsl-3d0
Signed-off-by: Xiang Gao <gaoxiang17@xiaomi.com>
---
drivers/dma-buf/dma-buf.c | 42 ++++++++-
include/trace/events/dma_buf.h | 157 +++++++++++++++++++++++++++++++++
2 files changed, 198 insertions(+), 1 deletion(-)
create mode 100644 include/trace/events/dma_buf.h
diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c
index 2bcf9ceca997..ce39bc19e13f 100644
--- a/drivers/dma-buf/dma-buf.c
+++ b/drivers/dma-buf/dma-buf.c
@@ -35,6 +35,25 @@
#include "dma-buf-sysfs-stats.h"
+#define CREATE_TRACE_POINTS
+#include <trace/events/dma_buf.h>
+
+/*
+ * dmabuf->name must be accessed with holding dmabuf->name_lock.
+ * we need to take the lock around the tracepoint call itself where
+ * it is called in the code.
+ *
+ * Note: FUNC##_enabled() is a static branch that will only
+ * be set when the trace event is enabled.
+ */
+#define DMA_BUF_TRACE(FUNC, ...) \
+ do { \
+ if (FUNC##_enabled()) { \
+ guard(spinlock)(&dmabuf->name_lock); \
+ FUNC(__VA_ARGS__); \
+ } \
+ } while (0)
+
static inline int is_dma_buf_file(struct file *);
static DEFINE_MUTEX(dmabuf_list_mutex);
@@ -220,6 +239,8 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma)
dmabuf->size >> PAGE_SHIFT)
return -EINVAL;
+ DMA_BUF_TRACE(trace_dma_buf_mmap_internal, dmabuf);
+
return dmabuf->ops->mmap(dmabuf, vma);
}
@@ -745,6 +766,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
__dma_buf_list_add(dmabuf);
+ DMA_BUF_TRACE(trace_dma_buf_export, dmabuf);
+
return dmabuf;
err_dmabuf:
@@ -779,6 +802,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
fd_install(fd, dmabuf->file);
+ DMA_BUF_TRACE(trace_dma_buf_fd, dmabuf, fd);
+
return fd;
}
EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
@@ -794,6 +819,7 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
struct dma_buf *dma_buf_get(int fd)
{
struct file *file;
+ struct dma_buf *dmabuf;
file = fget(fd);
@@ -805,7 +831,11 @@ struct dma_buf *dma_buf_get(int fd)
return ERR_PTR(-EINVAL);
}
- return file->private_data;
+ dmabuf = file->private_data;
+
+ DMA_BUF_TRACE(trace_dma_buf_get, dmabuf, fd);
+
+ return dmabuf;
}
EXPORT_SYMBOL_NS_GPL(dma_buf_get, "DMA_BUF");
@@ -825,6 +855,8 @@ void dma_buf_put(struct dma_buf *dmabuf)
return;
fput(dmabuf->file);
+
+ DMA_BUF_TRACE(trace_dma_buf_put, dmabuf);
}
EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF");
@@ -979,6 +1011,9 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,
list_add(&attach->node, &dmabuf->attachments);
dma_resv_unlock(dmabuf->resv);
+ DMA_BUF_TRACE(trace_dma_buf_dynamic_attach, dmabuf, attach,
+ dma_buf_attachment_is_dynamic(attach), dev);
+
return attach;
err_attach:
@@ -1023,6 +1058,9 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
if (dmabuf->ops->detach)
dmabuf->ops->detach(dmabuf, attach);
+ DMA_BUF_TRACE(trace_dma_buf_detach, dmabuf, attach,
+ dma_buf_attachment_is_dynamic(attach), attach->dev);
+
kfree(attach);
}
EXPORT_SYMBOL_NS_GPL(dma_buf_detach, "DMA_BUF");
@@ -1488,6 +1526,8 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
vma_set_file(vma, dmabuf->file);
vma->vm_pgoff = pgoff;
+ DMA_BUF_TRACE(trace_dma_buf_mmap, dmabuf);
+
return dmabuf->ops->mmap(dmabuf, vma);
}
EXPORT_SYMBOL_NS_GPL(dma_buf_mmap, "DMA_BUF");
diff --git a/include/trace/events/dma_buf.h b/include/trace/events/dma_buf.h
new file mode 100644
index 000000000000..35f8140095f4
--- /dev/null
+++ b/include/trace/events/dma_buf.h
@@ -0,0 +1,157 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM dma_buf
+
+#if !defined(_TRACE_DMA_BUF_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_DMA_BUF_H
+
+#include <linux/dma-buf.h>
+#include <linux/tracepoint.h>
+
+DECLARE_EVENT_CLASS(dma_buf,
+
+ TP_PROTO(struct dma_buf *dmabuf),
+
+ TP_ARGS(dmabuf),
+
+ TP_STRUCT__entry(
+ __string(exp_name, dmabuf->exp_name)
+ __field(size_t, size)
+ __field(ino_t, ino)
+ ),
+
+ TP_fast_assign(
+ __assign_str(exp_name);
+ __entry->size = dmabuf->size;
+ __entry->ino = dmabuf->file->f_inode->i_ino;
+ ),
+
+ TP_printk("exp_name=%s size=%zu ino=%lu",
+ __get_str(exp_name),
+ __entry->size,
+ __entry->ino)
+);
+
+DECLARE_EVENT_CLASS(dma_buf_attach_dev,
+
+ TP_PROTO(struct dma_buf *dmabuf, struct dma_buf_attachment *attach,
+ bool is_dynamic, struct device *dev),
+
+ TP_ARGS(dmabuf, attach, is_dynamic, dev),
+
+ TP_STRUCT__entry(
+ __string(dev_name, dev_name(dev))
+ __string(exp_name, dmabuf->exp_name)
+ __field(size_t, size)
+ __field(ino_t, ino)
+ __field(struct dma_buf_attachment *, attach)
+ __field(bool, is_dynamic)
+ ),
+
+ TP_fast_assign(
+ __assign_str(dev_name);
+ __assign_str(exp_name);
+ __entry->size = dmabuf->size;
+ __entry->ino = dmabuf->file->f_inode->i_ino;
+ __entry->is_dynamic = is_dynamic;
+ __entry->attach = attach;
+ ),
+
+ TP_printk("exp_name=%s size=%zu ino=%lu attachment:%p is_dynamic=%d dev_name=%s",
+ __get_str(exp_name),
+ __entry->size,
+ __entry->ino,
+ __entry->attach,
+ __entry->is_dynamic,
+ __get_str(dev_name))
+);
+
+DECLARE_EVENT_CLASS(dma_buf_fd,
+
+ TP_PROTO(struct dma_buf *dmabuf, int fd),
+
+ TP_ARGS(dmabuf, fd),
+
+ TP_STRUCT__entry(
+ __string(exp_name, dmabuf->exp_name)
+ __field(size_t, size)
+ __field(ino_t, ino)
+ __field(int, fd)
+ ),
+
+ TP_fast_assign(
+ __assign_str(exp_name);
+ __entry->size = dmabuf->size;
+ __entry->ino = dmabuf->file->f_inode->i_ino;
+ __entry->fd = fd;
+ ),
+
+ TP_printk("exp_name=%s size=%zu ino=%lu fd=%d",
+ __get_str(exp_name),
+ __entry->size,
+ __entry->ino,
+ __entry->fd)
+);
+
+DEFINE_EVENT(dma_buf, dma_buf_export,
+
+ TP_PROTO(struct dma_buf *dmabuf),
+
+ TP_ARGS(dmabuf)
+);
+
+DEFINE_EVENT(dma_buf, dma_buf_mmap_internal,
+
+ TP_PROTO(struct dma_buf *dmabuf),
+
+ TP_ARGS(dmabuf)
+);
+
+DEFINE_EVENT(dma_buf, dma_buf_mmap,
+
+ TP_PROTO(struct dma_buf *dmabuf),
+
+ TP_ARGS(dmabuf)
+);
+
+DEFINE_EVENT(dma_buf, dma_buf_put,
+
+ TP_PROTO(struct dma_buf *dmabuf),
+
+ TP_ARGS(dmabuf)
+);
+
+DEFINE_EVENT(dma_buf_attach_dev, dma_buf_dynamic_attach,
+
+ TP_PROTO(struct dma_buf *dmabuf, struct dma_buf_attachment *attach,
+ bool is_dynamic, struct device *dev),
+
+ TP_ARGS(dmabuf, attach, is_dynamic, dev)
+);
+
+DEFINE_EVENT(dma_buf_attach_dev, dma_buf_detach,
+
+ TP_PROTO(struct dma_buf *dmabuf, struct dma_buf_attachment *attach,
+ bool is_dynamic, struct device *dev),
+
+ TP_ARGS(dmabuf, attach, is_dynamic, dev)
+);
+
+DEFINE_EVENT(dma_buf_fd, dma_buf_fd,
+
+ TP_PROTO(struct dma_buf *dmabuf, int fd),
+
+ TP_ARGS(dmabuf, fd)
+);
+
+DEFINE_EVENT(dma_buf_fd, dma_buf_get,
+
+ TP_PROTO(struct dma_buf *dmabuf, int fd),
+
+ TP_ARGS(dmabuf, fd)
+);
+
+#endif /* _TRACE_DMA_BUF_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v1 1/4] tools/rtla: Consolidate nr_cpus usage across all tools
From: Tomas Glozar @ 2025-12-17 9:06 UTC (permalink / raw)
To: Costa Shulyupin
Cc: Steven Rostedt, Crystal Wood, Wander Lairson Costa, Ivan Pravdin,
John Kacur, Tiezhu Yang, linux-trace-kernel, linux-kernel, bpf
In-Reply-To: <CAP4=nvS9fTtNCtDCt254-ukTePD7hW3HoKExOPNPDOdppUig9g@mail.gmail.com>
út 16. 12. 2025 v 15:41 odesílatel Tomas Glozar <tglozar@redhat.com> napsal:
> Since commit 2f3172f9dd58c ("tools/rtla: Consolidate code between
> osnoise/timerlat and hist/top") that was merged into 6.18, common.h
> includes timerlat_u.h. Your change thus causes a double include of
> timerlat_u.h, leading to a build error:
>
> In file included from src/timerlat_u.c:20:
> src/timerlat_u.h:6:8: error: redefinition of ‘struct timerlat_u_params’
> 6 | struct timerlat_u_params {
> | ^~~~~~~~~~~~~~~~~
> In file included from src/common.h:5,
> from src/timerlat_u.c:19:
> src/timerlat_u.h:6:8: note: originally defined here
> 6 | struct timerlat_u_params {
> | ^~~~~~~~~~~~~~~~~
>
> Please rebase your patchset and fix this so that timerlat_u.h is only
> included once.
>
Correction: the base of the patchset has nothing to do with this. It
is the C standard, from C23 (default in GCC 15), redefinition of
structs is allowed [1], so this error doesn't exist. In earlier
standards, this is not allowed.
[1] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2863.pdf
Tomas
^ permalink raw reply
* Re: [PATCHv5 bpf-next 1/9] ftrace,bpf: Remove FTRACE_OPS_FL_JMP ftrace_ops flag
From: Jiri Olsa @ 2025-12-17 8:40 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: <95c13c0b793604796037ca67c191e1bfce22770dd020107c7179a698c5d49b91@mail.kernel.org>
On Mon, Dec 15, 2025 at 09:31:51PM +0000, bot+bpf-ci@kernel.org wrote:
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index 015dd1049bea..505b7d3f5641 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
>
> [ ... ]
>
> > diff --git a/kernel/bpf/trampoline.c b/kernel/bpf/trampoline.c
> > index 976d89011b15..b9a358d7a78f 100644
> > --- a/kernel/bpf/trampoline.c
> > +++ b/kernel/bpf/trampoline.c
> > @@ -214,10 +214,15 @@ static int modify_fentry(struct bpf_trampoline *tr, u32 orig_flags,
> > int ret;
> >
> > if (tr->func.ftrace_managed) {
> > + unsigned long addr = (unsigned long) new_addr;
> > +
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > +
> > if (lock_direct_mutex)
> > - ret = modify_ftrace_direct(tr->fops, (long)new_addr);
> > + ret = modify_ftrace_direct(tr->fops, addr);
> > else
> > - ret = modify_ftrace_direct_nolock(tr->fops, (long)new_addr);
> > + ret = modify_ftrace_direct_nolock(tr->fops, addr);
> > } else {
> > ret = bpf_trampoline_update_fentry(tr, orig_flags, old_addr,
> > new_addr);
> > @@ -240,10 +245,15 @@ static int register_fentry(struct bpf_trampoline *tr, void *new_addr)
> > }
> >
> > if (tr->func.ftrace_managed) {
> > + unsigned long addr = (unsigned long) new_addr;
> > +
> > + if (bpf_trampoline_use_jmp(tr->flags))
> > + addr = ftrace_jmp_set(addr);
> > +
> > ret = ftrace_set_filter_ip(tr->fops, (unsigned long)ip, 0, 1);
> > if (ret)
> > return ret;
> > - ret = register_ftrace_direct(tr->fops, (long)new_addr);
> > + ret = register_ftrace_direct(tr->fops, addr);
> > } else {
> > ret = bpf_trampoline_update_fentry(tr, 0, NULL, new_addr);
> > }
>
> After this change, register_ftrace_direct() receives an address with
> the LSB bit already set (via ftrace_jmp_set) and stores it as-is in
> entry->direct. This address then flows through to ftrace_find_rec_direct()
> which returns entry->direct unchanged.
>
> In available_filter_functions_show():
hum, there's no such function.. AFAICS it's t_show called through
ftrace_avail_fops->ftrace_avail_open->show_ftrace_seq_ops
>
> direct = ftrace_find_rec_direct(rec->ip);
> if (direct)
> seq_printf(m, "\n\tdirect-->%pS", (void *)direct);
>
but yep, it's related problem, that Menglong said he will address in
separate patch
thanks,
jirka
> Can the debug output show misleading symbol offsets? When the LSB is
> set, %pS will resolve addresses like "function+0x1" instead of just
> "function". This makes ftrace debug output harder to read for users
> trying to understand direct call attachments.
>
> The fix would be to strip the LSB bit before printing:
> seq_printf(m, "\n\tdirect-->%pS", (void *)ftrace_jmp_get(direct));
>
> [ ... ]
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index bbb37c0f8c6c..b0dc911411f1 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6017,15 +6017,8 @@ int register_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
> > if (ftrace_hash_empty(hash))
> > return -EINVAL;
> >
> > - /* This is a "raw" address, and this should never happen. */
> > - if (WARN_ON_ONCE(ftrace_is_jmp(addr)))
> > - return -EINVAL;
> > -
> > mutex_lock(&direct_mutex);
> >
> > - if (ops->flags & FTRACE_OPS_FL_JMP)
> > - addr = ftrace_jmp_set(addr);
> > -
> > /* Make sure requested entries are not already registered.. */
> > size = 1 << hash->size_bits;
> > for (i = 0; i < size; i++) {
>
> [ ... ]
>
> > @@ -6146,13 +6139,6 @@ __modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
> >
> > lockdep_assert_held_once(&direct_mutex);
> >
> > - /* This is a "raw" address, and this should never happen. */
> > - if (WARN_ON_ONCE(ftrace_is_jmp(addr)))
> > - return -EINVAL;
> > -
> > - if (ops->flags & FTRACE_OPS_FL_JMP)
> > - addr = ftrace_jmp_set(addr);
> > -
> > /* 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;
>
> [ ... ]
>
>
> ---
> 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/20247816064
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox