* Re: [rtla 04/13] rtla: Replace atoi() with a robust strtoi()
From: Wander Lairson Costa @ 2025-11-25 13:49 UTC (permalink / raw)
To: Costa Shulyupin
Cc: Steven Rostedt, Tomas Glozar, Ivan Pravdin, Crystal Wood,
John Kacur, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:b|_)bpf(?:b|_)
In-Reply-To: <CADDUTFyAAAv641OfGf_U4hVdegyAVyp5rgruF=NSNd+UPkjOzQ@mail.gmail.com>
On Tue, Nov 25, 2025 at 10:35:39AM +0200, Costa Shulyupin wrote:
> On Mon, 17 Nov 2025 at 20:55, Wander Lairson Costa <wander@redhat.com> wrote:
> > To address this, introduce a new strtoi() helper function that safely
> > converts a string to an integer. This function validates the input and
> > checks for overflows, returning a boolean to indicate success or failure.
>
> Why not use sscanf() for this purpose instead of adding a new utility function?
>
The strtoi implementation properly detects:
1. Empty strings - via the !*s check
2. Conversion errors - via errno from strtol
3. Trailing garbage - via *end_ptr check ensuring entire string was consumed
4. Integer overflow/underflow - via explicit lres > INT_MAX || lres < INT_MIN
bounds checking
sscanf has the following limitations:
1. Trailing garbage is silently ignored
int val;
sscanf("123abc", "%d", &val); /* Returns 1 (success), val=123, "abc" ignored */
While you could use "%d%n" with character count checking, this becomes
cumbersome and defeats the purpose of simplification.
2. Integer overflow has undefined behavior
sscanf with %d doesn't guarantee overflow detection and may silently wrap
values (e.g., 2147483648 -> -2147483648). There's no standard way to detect
this has occurred.
3. No detailed error reporting (this is minor, though)
sscanf only returns match count, not error type. You cannot distinguish
"bad format" from "overflow" from "trailing garbage".
> Also, using a boolean to return success or failure does not conform to
> POSIX standards and is confusing in Linux/POSIX code.
>
Ok, I will change it.
> Costa
>
^ permalink raw reply
* Re: [PATCH v3 2/7] rtla/timerlat: Add --bpf-action option
From: Tomas Glozar @ 2025-11-25 13:48 UTC (permalink / raw)
To: Wander Lairson Costa
Cc: Steven Rostedt, LKML, Linux Trace Kernel, John Kacur,
Luis Goncalves, Costa Shulyupin, Crystal Wood,
Arnaldo Carvalho de Melo
In-Reply-To: <liouklnl72kulibowk33roalgtjor24u3h6hfbuzudlodasdvz@fhy2zeif43nk>
po 3. 11. 2025 v 15:45 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
> >
> > Executing additional BPF code on latency threshold overflow allows doing
> > doing low-latency and in-kernel troubleshooting of the cause of the
>
> typo: double "doing"
>
Thanks, I'll fix that :)
> > --- a/tools/tracing/rtla/src/timerlat.c
> > +++ b/tools/tracing/rtla/src/timerlat.c
> > @@ -48,6 +48,17 @@ timerlat_apply_config(struct osnoise_tool *tool, struct timerlat_params *params)
> > }
> > }
> >
> > + /* Check if BPF action program is requested but BPF is not available */
> > + if (params->bpf_action_program) {
> > + if (params->mode == TRACING_MODE_TRACEFS) {
> > + err_msg("BPF actions are not supported in tracefs-only mode\n");
>
> I would just emit a warning to the user and proceed ignoring the bpf action argument.
>
I believe if the user explicitly requests BPF actions to be used,
measurement should not proceed without the action. Imagine someone
setting --bpf-action in an automated test, expecting it to report
something. But the action never fires, because they do not notice they
are running an old kernel that does not support this.
The user can always restart/reconfigure RTLA to skip the option.
Tomas
^ permalink raw reply
* Re: [rtla 05/13] rtla: Simplify argument parsing
From: Wander Lairson Costa @ 2025-11-25 13:45 UTC (permalink / raw)
To: Crystal Wood
Cc: Steven Rostedt, Tomas Glozar, Ivan Pravdin, John Kacur,
Costa Shulyupin, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_)
In-Reply-To: <e96f06667d07fe7f207fc8769218967d22cf7634.camel@redhat.com>
On Mon, Nov 24, 2025 at 06:46:33PM -0600, Crystal Wood wrote:
> On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
>
> >
> > +/*
> > + * extract_arg - extract argument value from option token
> > + * @token: option token (e.g., "file=trace.txt")
> > + * @opt: option name to match (e.g., "file")
> > + *
> > + * Returns pointer to argument value after "=" if token matches "opt=",
> > + * otherwise returns NULL.
> > + */
> > +#define extract_arg(token, opt) ( \
> > + strlen(token) > STRING_LENGTH(opt "=") && \
> > + !strncmp_static(token, opt "=") \
> > + ? (token) + STRING_LENGTH(opt "=") : NULL )
>
> This could be implemented as a function (albeit without the
> concatenation trick)... but if it must be a macro, at least encase it
> with ({ }) so it behaves more like a function.
>
> > +
> > /*
> > * actions_parse - add an action based on text specification
> > */
> > int
> > actions_parse(struct actions *self, const char *trigger, const char *tracefn)
> > {
> > +
> > enum action_type type = ACTION_NONE;
>
> Why this blank line?
>
Must be a typo during the rebase process. I will remove it.
>
> > diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
> > index 160491f5de91c..f7ff548f7fba7 100644
> > --- a/tools/tracing/rtla/src/utils.h
> > +++ b/tools/tracing/rtla/src/utils.h
> > @@ -13,8 +13,18 @@
> > #define MAX_NICE 20
> > #define MIN_NICE -19
> >
> > -#define container_of(ptr, type, member)({ \
> > - const typeof(((type *)0)->member) *__mptr = (ptr); \
> [snip]
> > +#define container_of(ptr, type, member)({ \
> > + const typeof(((type *)0)->member) *__mptr = (ptr); \
> > (type *)((char *)__mptr - offsetof(type, member)) ; })
>
> It's easier to review patches when they don't make unrelated aesthetic
> changes...
>
It is just git messing up the diff. No actual change.
> -Crystal
>
^ permalink raw reply
* Re: [PATCH v3 1/7] rtla/timerlat: Support tail call from BPF program
From: Tomas Glozar @ 2025-11-25 13:35 UTC (permalink / raw)
To: Wander Lairson Costa
Cc: Steven Rostedt, LKML, Linux Trace Kernel, John Kacur,
Luis Goncalves, Costa Shulyupin, Crystal Wood,
Arnaldo Carvalho de Melo
In-Reply-To: <ijizcoufpxwtrutqpurumsx7zls2dixyanlcn6oshvhpon5osd@aeuzrsmkidrz>
po 3. 11. 2025 v 15:24 odesílatel Wander Lairson Costa
<wander@redhat.com> napsal:
>> +/*
>> + * timerlat_bpf_set_action - set action on threshold executed on BPF side
>> + */
>> +static int timerlat_bpf_set_action(struct bpf_program *prog)
>> +{
>> + unsigned int key = 0, value = bpf_program__fd(prog);
>> +
>> + return bpf_map__update_elem(bpf->maps.bpf_action,
>> + &key, sizeof(key),
>> + &value, sizeof(value),
>> + BPF_ANY);
>> +}
>> +
>
> I believe it makes more sense to add the definition of this function to
> the patch where it is called.
>
Maybe, but I wrote this patch first, the rest of the patchset some
time after that. That is why it is this way; either way, it doesn't
hurt to define something before using it, the other way is worse.
Tomas
^ permalink raw reply
* Re: [rtla 04/13] rtla: Replace atoi() with a robust strtoi()
From: Wander Lairson Costa @ 2025-11-25 13:34 UTC (permalink / raw)
To: Crystal Wood
Cc: Steven Rostedt, Tomas Glozar, Ivan Pravdin, John Kacur,
Costa Shulyupin, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_)
In-Reply-To: <9770045bcf400920152f0698c07090a641cc4aa1.camel@redhat.com>
On Mon, Nov 24, 2025 at 9:46 PM Crystal Wood <crwood@redhat.com> wrote:
>
> On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
>
> >
> > diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
> > index efa17290926da..e23d4f1c5a592 100644
> > --- a/tools/tracing/rtla/src/actions.c
> > +++ b/tools/tracing/rtla/src/actions.c
> > @@ -199,12 +199,14 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
> > /* Takes two arguments, num (signal) and pid */
> > while (token != NULL) {
> > if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) {
> > - signal = atoi(token + 4);
> > + if(!strtoi(token + 4, &signal))
> > + return -1;
>
> if (
>
> > } else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) {
> > if (strncmp(token + 4, "parent", 7) == 0)
> > pid = -1;
> > else
> > - pid = atoi(token + 4);
> > + if (!strtoi(token + 4, &pid))
> > + return -1;
>
> else if (
>
> Please run the patches through checkpatch.pl
>
Good catch, thanks.
> > @@ -959,3 +967,25 @@ int auto_house_keeping(cpu_set_t *monitored_cpus)
> >
> > return 1;
> > }
> > +
> > +/*
> > + * strtoi - convert string to integer with error checking
> > + *
> > + * Returns true on success, false if conversion fails or result is out of int range.
> > + */
> > +bool strtoi(const char *s, int *res)
>
> Could use __attribute__((__warn_unused_result__)) like kstrtoint().
>
Sure, I will do it in v2.
> BTW, it's pretty annoying that we need to reinvent the wheel on all this
> stuff just because it's userspace. From some of the other tools it
> looks like we can at least include basic kernel headers like compiler.h;
> maybe we should have a tools/-wide common util area as well? Even
> better if some of the code can be shared with the kernel itself.
>
> Not saying that should in any way be a blocker for these patches, just
> something to think about.
>
I thought the same thing some time ago.
>
> -Crystal
>
^ permalink raw reply
* Re: [PATCH 2/2] tracing: Merge struct event_trigger_ops into struct event_command
From: Tom Zanussi @ 2025-11-25 13:01 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251119031603.658997916@kernel.org>
Hi Steve,
On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> Now that there's pretty much a one to one mapping between the struct
> event_trigger_ops and struct event_command, there's no reason to have two
> different structures. Merge the function pointers of event_trigger_ops
> into event_command.
>
> There's one exception in trace_events_hist.c for the
> event_hist_trigger_named_ops. This has special logic for the init and free
> function pointers for "named histograms". In this case, allocate the
> cmd_ops of the event_trigger_data and set it to the proper init and free
> functions, which are used to initialize and free the event_trigger_data
> respectively. Have the free function and the init function (on failure)
> free the cmd_ops of the data element.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Very nice as well, just one tiny note below..
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
> ---
> kernel/trace/trace.h | 126 +++++++++++-----------------
> kernel/trace/trace_eprobe.c | 13 +--
> kernel/trace/trace_events_hist.c | 93 ++++++++++----------
> kernel/trace/trace_events_trigger.c | 121 +++++++++++---------------
> 4 files changed, 152 insertions(+), 201 deletions(-)
>
[...]
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index f9cc8d6a215b..1e03398b9e91 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -5694,7 +5694,7 @@ static void hist_trigger_show(struct seq_file *m,
> seq_puts(m, "\n\n");
>
> seq_puts(m, "# event histogram\n#\n# trigger info: ");
> - data->ops->print(m, data);
> + data->cmd_ops->print(m, data);
> seq_puts(m, "#\n\n");
>
> hist_data = data->private_data;
> @@ -6016,7 +6016,7 @@ static void hist_trigger_debug_show(struct seq_file *m,
> seq_puts(m, "\n\n");
>
> seq_puts(m, "# event histogram\n#\n# trigger info: ");
> - data->ops->print(m, data);
> + data->cmd_ops->print(m, data);
> seq_puts(m, "#\n\n");
>
> hist_data = data->private_data;
> @@ -6326,20 +6326,23 @@ static void event_hist_trigger_free(struct event_trigger_data *data)
> free_hist_pad();
> }
>
> -static const struct event_trigger_ops event_hist_trigger_ops = {
> - .trigger = event_hist_trigger,
> - .print = event_hist_trigger_print,
> - .init = event_hist_trigger_init,
> - .free = event_hist_trigger_free,
> -};
> +static struct event_command trigger_hist_cmd;
>
I don't think this is needed, since the same declaration already
appears just above the event_hist_trigger_parse() declaration.
Thanks,
Tom
^ permalink raw reply
* Re: [PATCH 1/2] tracing: Remove get_trigger_ops() and add count_func() from trigger ops
From: Tom Zanussi @ 2025-11-25 12:54 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel, linux-trace-kernel
Cc: Masami Hiramatsu, Mark Rutland, Mathieu Desnoyers, Andrew Morton
In-Reply-To: <20251119031603.491057010@kernel.org>
Hi Steve,
On Tue, 2025-11-18 at 22:10 -0500, Steven Rostedt wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
>
> The struct event_command has a callback function called get_trigger_ops().
> This callback returns the "trigger_ops" to use for the trigger. These ops
> define the trigger function, how to init the trigger, how to print the
> trigger and how to free it.
>
> The only reason there's a callback function to get these ops is because
> some triggers have two types of operations. One is an "always on"
> operation, and the other is a "count down" operation. If a user passes in
> a parameter to say how many times the trigger should execute. For example:
>
> echo stacktrace:5 > events/kmem/kmem_cache_alloc/trigger
>
> It will trigger the stacktrace for the first 5 times the kmem_cache_alloc
> event is hit.
>
> Instead of having two different trigger_ops since the only difference
> between them is the tigger itself (the print, init and free functions are
> all the same), just use a single ops that the event_command points to and
> add a function field to the trigger_ops to have a count_func.
>
> When a trigger is added to an event, if there's a count attached to it and
> the trigger ops has the count_func field, the data allocated to represent
> this trigger will have a new flag set called COUNT.
>
> Then when the trigger executes, it will check if the COUNT data flag is
> set, and if so, it will call the ops count_func(). If that returns false,
> it returns without executing the trigger.
>
> This removes the need for duplicate event_trigger_ops structures.
>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Very nice cleanup, thanks!
Reviewed-by: Tom Zanussi <zanussi@kernel.org>
> ---
> kernel/trace/trace.h | 26 ++-
> kernel/trace/trace_eprobe.c | 8 +-
> kernel/trace/trace_events_hist.c | 60 +------
> kernel/trace/trace_events_trigger.c | 257 ++++++++++------------------
> 4 files changed, 116 insertions(+), 235 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 58be6d741d72..036019ffc407 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -1791,6 +1791,7 @@ extern void clear_event_triggers(struct trace_array *tr);
>
> enum {
> EVENT_TRIGGER_FL_PROBE = BIT(0),
> + EVENT_TRIGGER_FL_COUNT = BIT(1),
> };
>
> struct event_trigger_data {
> @@ -1822,6 +1823,10 @@ struct enable_trigger_data {
> bool hist;
> };
>
> +bool event_trigger_count(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event);
> +
> extern int event_enable_trigger_print(struct seq_file *m,
> struct event_trigger_data *data);
> extern void event_enable_trigger_free(struct event_trigger_data *data);
> @@ -1909,6 +1914,11 @@ extern void event_file_put(struct trace_event_file *file);
> * registered the trigger (see struct event_command) along with
> * the trace record, rec.
> *
> + * @count_func: If defined and a numeric parameter is passed to the
> + * trigger, then this function will be called before @trigger
> + * is called. If this function returns false, then @trigger is not
> + * executed.
> + *
> * @init: An optional initialization function called for the trigger
> * when the trigger is registered (via the event_command reg()
> * function). This can be used to perform per-trigger
> @@ -1936,6 +1946,10 @@ struct event_trigger_ops {
> struct trace_buffer *buffer,
> void *rec,
> struct ring_buffer_event *rbe);
> + bool (*count_func)(struct event_trigger_data *data,
> + struct trace_buffer *buffer,
> + void *rec,
> + struct ring_buffer_event *rbe);
> int (*init)(struct event_trigger_data *data);
> void (*free)(struct event_trigger_data *data);
> int (*print)(struct seq_file *m,
> @@ -1962,6 +1976,9 @@ struct event_trigger_ops {
> * @name: The unique name that identifies the event command. This is
> * the name used when setting triggers via trigger files.
> *
> + * @trigger_ops: The event_trigger_ops implementation associated with
> + * the command.
> + *
> * @trigger_type: A unique id that identifies the event command
> * 'type'. This value has two purposes, the first to ensure that
> * only one trigger of the same type can be set at a given time
> @@ -2013,17 +2030,11 @@ struct event_trigger_ops {
> * event command, filters set by the user for the command will be
> * ignored. This is usually implemented by the generic utility
> * function @set_trigger_filter() (see trace_event_triggers.c).
> - *
> - * @get_trigger_ops: The callback function invoked to retrieve the
> - * event_trigger_ops implementation associated with the command.
> - * This callback function allows a single event_command to
> - * support multiple trigger implementations via different sets of
> - * event_trigger_ops, depending on the value of the @param
> - * string.
> */
> struct event_command {
> struct list_head list;
> char *name;
> + const struct event_trigger_ops *trigger_ops;
> enum event_trigger_type trigger_type;
> int flags;
> int (*parse)(struct event_command *cmd_ops,
> @@ -2040,7 +2051,6 @@ struct event_command {
> int (*set_filter)(char *filter_str,
> struct event_trigger_data *data,
> struct trace_event_file *file);
> - const struct event_trigger_ops *(*get_trigger_ops)(char *cmd, char *param);
> };
>
> /**
> diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c
> index a1d402124836..14ae896dbe75 100644
> --- a/kernel/trace/trace_eprobe.c
> +++ b/kernel/trace/trace_eprobe.c
> @@ -513,21 +513,15 @@ static void eprobe_trigger_unreg_func(char *glob,
>
> }
>
> -static const struct event_trigger_ops *eprobe_trigger_get_ops(char *cmd,
> - char *param)
> -{
> - return &eprobe_trigger_ops;
> -}
> -
> static struct event_command event_trigger_cmd = {
> .name = "eprobe",
> .trigger_type = ETT_EVENT_EPROBE,
> .flags = EVENT_CMD_FL_NEEDS_REC,
> + .trigger_ops = &eprobe_trigger_ops,
> .parse = eprobe_trigger_cmd_parse,
> .reg = eprobe_trigger_reg_func,
> .unreg = eprobe_trigger_unreg_func,
> .unreg_all = NULL,
> - .get_trigger_ops = eprobe_trigger_get_ops,
> .set_filter = NULL,
> };
>
> diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c
> index 1d536219b624..f9cc8d6a215b 100644
> --- a/kernel/trace/trace_events_hist.c
> +++ b/kernel/trace/trace_events_hist.c
> @@ -6363,12 +6363,6 @@ static const struct event_trigger_ops event_hist_trigger_named_ops = {
> .free = event_hist_trigger_named_free,
> };
>
> -static const struct event_trigger_ops *event_hist_get_trigger_ops(char *cmd,
> - char *param)
> -{
> - return &event_hist_trigger_ops;
> -}
> -
> static void hist_clear(struct event_trigger_data *data)
> {
> struct hist_trigger_data *hist_data = data->private_data;
> @@ -6908,11 +6902,11 @@ static struct event_command trigger_hist_cmd = {
> .name = "hist",
> .trigger_type = ETT_EVENT_HIST,
> .flags = EVENT_CMD_FL_NEEDS_REC,
> + .trigger_ops = &event_hist_trigger_ops,
> .parse = event_hist_trigger_parse,
> .reg = hist_register_trigger,
> .unreg = hist_unregister_trigger,
> .unreg_all = hist_unreg_all,
> - .get_trigger_ops = event_hist_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> @@ -6945,29 +6939,9 @@ hist_enable_trigger(struct event_trigger_data *data,
> }
> }
>
> -static void
> -hist_enable_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> -{
> - if (!data->count)
> - return;
> -
> - if (data->count != -1)
> - (data->count)--;
> -
> - hist_enable_trigger(data, buffer, rec, event);
> -}
> -
> static const struct event_trigger_ops hist_enable_trigger_ops = {
> .trigger = hist_enable_trigger,
> - .print = event_enable_trigger_print,
> - .init = event_trigger_init,
> - .free = event_enable_trigger_free,
> -};
> -
> -static const struct event_trigger_ops hist_enable_count_trigger_ops = {
> - .trigger = hist_enable_count_trigger,
> + .count_func = event_trigger_count,
> .print = event_enable_trigger_print,
> .init = event_trigger_init,
> .free = event_enable_trigger_free,
> @@ -6975,36 +6949,12 @@ static const struct event_trigger_ops hist_enable_count_trigger_ops = {
>
> static const struct event_trigger_ops hist_disable_trigger_ops = {
> .trigger = hist_enable_trigger,
> + .count_func = event_trigger_count,
> .print = event_enable_trigger_print,
> .init = event_trigger_init,
> .free = event_enable_trigger_free,
> };
>
> -static const struct event_trigger_ops hist_disable_count_trigger_ops = {
> - .trigger = hist_enable_count_trigger,
> - .print = event_enable_trigger_print,
> - .init = event_trigger_init,
> - .free = event_enable_trigger_free,
> -};
> -
> -static const struct event_trigger_ops *
> -hist_enable_get_trigger_ops(char *cmd, char *param)
> -{
> - const struct event_trigger_ops *ops;
> - bool enable;
> -
> - enable = (strcmp(cmd, ENABLE_HIST_STR) == 0);
> -
> - if (enable)
> - ops = param ? &hist_enable_count_trigger_ops :
> - &hist_enable_trigger_ops;
> - else
> - ops = param ? &hist_disable_count_trigger_ops :
> - &hist_disable_trigger_ops;
> -
> - return ops;
> -}
> -
> static void hist_enable_unreg_all(struct trace_event_file *file)
> {
> struct event_trigger_data *test, *n;
> @@ -7023,22 +6973,22 @@ static void hist_enable_unreg_all(struct trace_event_file *file)
> static struct event_command trigger_hist_enable_cmd = {
> .name = ENABLE_HIST_STR,
> .trigger_type = ETT_HIST_ENABLE,
> + .trigger_ops = &hist_enable_trigger_ops,
> .parse = event_enable_trigger_parse,
> .reg = event_enable_register_trigger,
> .unreg = event_enable_unregister_trigger,
> .unreg_all = hist_enable_unreg_all,
> - .get_trigger_ops = hist_enable_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> static struct event_command trigger_hist_disable_cmd = {
> .name = DISABLE_HIST_STR,
> .trigger_type = ETT_HIST_ENABLE,
> + .trigger_ops = &hist_disable_trigger_ops,
> .parse = event_enable_trigger_parse,
> .reg = event_enable_register_trigger,
> .unreg = event_enable_unregister_trigger,
> .unreg_all = hist_enable_unreg_all,
> - .get_trigger_ops = hist_enable_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> diff --git a/kernel/trace/trace_events_trigger.c b/kernel/trace/trace_events_trigger.c
> index cbfc306c0159..576bad18bcdb 100644
> --- a/kernel/trace/trace_events_trigger.c
> +++ b/kernel/trace/trace_events_trigger.c
> @@ -28,6 +28,20 @@ void trigger_data_free(struct event_trigger_data *data)
> kfree(data);
> }
>
> +static inline void data_ops_trigger(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event)
> +{
> + const struct event_trigger_ops *ops = data->ops;
> +
> + if (data->flags & EVENT_TRIGGER_FL_COUNT) {
> + if (!ops->count_func(data, buffer, rec, event))
> + return;
> + }
> +
> + ops->trigger(data, buffer, rec, event);
> +}
> +
> /**
> * event_triggers_call - Call triggers associated with a trace event
> * @file: The trace_event_file associated with the event
> @@ -70,7 +84,7 @@ event_triggers_call(struct trace_event_file *file,
> if (data->paused)
> continue;
> if (!rec) {
> - data->ops->trigger(data, buffer, rec, event);
> + data_ops_trigger(data, buffer, rec, event);
> continue;
> }
> filter = rcu_dereference_sched(data->filter);
> @@ -80,7 +94,7 @@ event_triggers_call(struct trace_event_file *file,
> tt |= data->cmd_ops->trigger_type;
> continue;
> }
> - data->ops->trigger(data, buffer, rec, event);
> + data_ops_trigger(data, buffer, rec, event);
> }
> return tt;
> }
> @@ -122,7 +136,7 @@ event_triggers_post_call(struct trace_event_file *file,
> if (data->paused)
> continue;
> if (data->cmd_ops->trigger_type & tt)
> - data->ops->trigger(data, NULL, NULL, NULL);
> + data_ops_trigger(data, NULL, NULL, NULL);
> }
> }
> EXPORT_SYMBOL_GPL(event_triggers_post_call);
> @@ -377,6 +391,36 @@ __init int unregister_event_command(struct event_command *cmd)
> return -ENODEV;
> }
>
> +/**
> + * event_trigger_count - Optional count function for event triggers
> + * @data: Trigger-specific data
> + * @buffer: The ring buffer that the event is being written to
> + * @rec: The trace entry for the event, NULL for unconditional invocation
> + * @event: The event meta data in the ring buffer
> + *
> + * For triggers that can take a count parameter that doesn't do anything
> + * special, they can use this function to assign to their .count_func
> + * field.
> + *
> + * This simply does a count down of the @data->count field.
> + *
> + * If the @data->count is greater than zero, it will decrement it.
> + *
> + * Returns false if @data->count is zero, otherwise true.
> + */
> +bool event_trigger_count(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event)
> +{
> + if (!data->count)
> + return false;
> +
> + if (data->count != -1)
> + (data->count)--;
> +
> + return true;
> +}
> +
> /**
> * event_trigger_print - Generic event_trigger_ops @print implementation
> * @name: The name of the event trigger
> @@ -807,9 +851,13 @@ int event_trigger_separate_filter(char *param_and_filter, char **param,
> * @private_data: User data to associate with the event trigger
> *
> * Allocate an event_trigger_data instance and initialize it. The
> - * @cmd_ops are used along with the @cmd and @param to get the
> - * trigger_ops to assign to the event_trigger_data. @private_data can
> - * also be passed in and associated with the event_trigger_data.
> + * @cmd_ops defines how the trigger will operate. If @param is set,
> + * and @cmd_ops->trigger_ops->count_func is non NULL, then the
> + * data->count is set to @param and before the trigger is executed, the
> + * @cmd_ops->trigger_ops->count_func() is called. If that function returns
> + * false, the @cmd_ops->trigger_ops->trigger() function will not be called.
> + * @private_data can also be passed in and associated with the
> + * event_trigger_data.
> *
> * Use trigger_data_free() to free an event_trigger_data object.
> *
> @@ -821,18 +869,17 @@ struct event_trigger_data *trigger_data_alloc(struct event_command *cmd_ops,
> void *private_data)
> {
> struct event_trigger_data *trigger_data;
> - const struct event_trigger_ops *trigger_ops;
> -
> - trigger_ops = cmd_ops->get_trigger_ops(cmd, param);
>
> trigger_data = kzalloc(sizeof(*trigger_data), GFP_KERNEL);
> if (!trigger_data)
> return NULL;
>
> trigger_data->count = -1;
> - trigger_data->ops = trigger_ops;
> + trigger_data->ops = cmd_ops->trigger_ops;
> trigger_data->cmd_ops = cmd_ops;
> trigger_data->private_data = private_data;
> + if (param && cmd_ops->trigger_ops->count_func)
> + trigger_data->flags |= EVENT_TRIGGER_FL_COUNT;
>
> INIT_LIST_HEAD(&trigger_data->list);
> INIT_LIST_HEAD(&trigger_data->named_list);
> @@ -1271,31 +1318,28 @@ traceon_trigger(struct event_trigger_data *data,
> tracing_on();
> }
>
> -static void
> -traceon_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> +static bool
> +traceon_count_func(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event)
> {
> struct trace_event_file *file = data->private_data;
>
> if (file) {
> if (tracer_tracing_is_on(file->tr))
> - return;
> + return false;
> } else {
> if (tracing_is_on())
> - return;
> + return false;
> }
>
> if (!data->count)
> - return;
> + return false;
>
> if (data->count != -1)
> (data->count)--;
>
> - if (file)
> - tracer_tracing_on(file->tr);
> - else
> - tracing_on();
> + return true;
> }
>
> static void
> @@ -1319,31 +1363,28 @@ traceoff_trigger(struct event_trigger_data *data,
> tracing_off();
> }
>
> -static void
> -traceoff_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> +static bool
> +traceoff_count_func(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event)
> {
> struct trace_event_file *file = data->private_data;
>
> if (file) {
> if (!tracer_tracing_is_on(file->tr))
> - return;
> + return false;
> } else {
> if (!tracing_is_on())
> - return;
> + return false;
> }
>
> if (!data->count)
> - return;
> + return false;
>
> if (data->count != -1)
> (data->count)--;
>
> - if (file)
> - tracer_tracing_off(file->tr);
> - else
> - tracing_off();
> + return true;
> }
>
> static int
> @@ -1362,13 +1403,7 @@ traceoff_trigger_print(struct seq_file *m, struct event_trigger_data *data)
>
> static const struct event_trigger_ops traceon_trigger_ops = {
> .trigger = traceon_trigger,
> - .print = traceon_trigger_print,
> - .init = event_trigger_init,
> - .free = event_trigger_free,
> -};
> -
> -static const struct event_trigger_ops traceon_count_trigger_ops = {
> - .trigger = traceon_count_trigger,
> + .count_func = traceon_count_func,
> .print = traceon_trigger_print,
> .init = event_trigger_init,
> .free = event_trigger_free,
> @@ -1376,41 +1411,19 @@ static const struct event_trigger_ops traceon_count_trigger_ops = {
>
> static const struct event_trigger_ops traceoff_trigger_ops = {
> .trigger = traceoff_trigger,
> + .count_func = traceoff_count_func,
> .print = traceoff_trigger_print,
> .init = event_trigger_init,
> .free = event_trigger_free,
> };
>
> -static const struct event_trigger_ops traceoff_count_trigger_ops = {
> - .trigger = traceoff_count_trigger,
> - .print = traceoff_trigger_print,
> - .init = event_trigger_init,
> - .free = event_trigger_free,
> -};
> -
> -static const struct event_trigger_ops *
> -onoff_get_trigger_ops(char *cmd, char *param)
> -{
> - const struct event_trigger_ops *ops;
> -
> - /* we register both traceon and traceoff to this callback */
> - if (strcmp(cmd, "traceon") == 0)
> - ops = param ? &traceon_count_trigger_ops :
> - &traceon_trigger_ops;
> - else
> - ops = param ? &traceoff_count_trigger_ops :
> - &traceoff_trigger_ops;
> -
> - return ops;
> -}
> -
> static struct event_command trigger_traceon_cmd = {
> .name = "traceon",
> .trigger_type = ETT_TRACE_ONOFF,
> + .trigger_ops = &traceon_trigger_ops,
> .parse = event_trigger_parse,
> .reg = register_trigger,
> .unreg = unregister_trigger,
> - .get_trigger_ops = onoff_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> @@ -1418,10 +1431,10 @@ static struct event_command trigger_traceoff_cmd = {
> .name = "traceoff",
> .trigger_type = ETT_TRACE_ONOFF,
> .flags = EVENT_CMD_FL_POST_TRIGGER,
> + .trigger_ops = &traceoff_trigger_ops,
> .parse = event_trigger_parse,
> .reg = register_trigger,
> .unreg = unregister_trigger,
> - .get_trigger_ops = onoff_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> @@ -1439,20 +1452,6 @@ snapshot_trigger(struct event_trigger_data *data,
> tracing_snapshot();
> }
>
> -static void
> -snapshot_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> -{
> - if (!data->count)
> - return;
> -
> - if (data->count != -1)
> - (data->count)--;
> -
> - snapshot_trigger(data, buffer, rec, event);
> -}
> -
> static int
> register_snapshot_trigger(char *glob,
> struct event_trigger_data *data,
> @@ -1486,31 +1485,19 @@ snapshot_trigger_print(struct seq_file *m, struct event_trigger_data *data)
>
> static const struct event_trigger_ops snapshot_trigger_ops = {
> .trigger = snapshot_trigger,
> + .count_func = event_trigger_count,
> .print = snapshot_trigger_print,
> .init = event_trigger_init,
> .free = event_trigger_free,
> };
>
> -static const struct event_trigger_ops snapshot_count_trigger_ops = {
> - .trigger = snapshot_count_trigger,
> - .print = snapshot_trigger_print,
> - .init = event_trigger_init,
> - .free = event_trigger_free,
> -};
> -
> -static const struct event_trigger_ops *
> -snapshot_get_trigger_ops(char *cmd, char *param)
> -{
> - return param ? &snapshot_count_trigger_ops : &snapshot_trigger_ops;
> -}
> -
> static struct event_command trigger_snapshot_cmd = {
> .name = "snapshot",
> .trigger_type = ETT_SNAPSHOT,
> + .trigger_ops = &snapshot_trigger_ops,
> .parse = event_trigger_parse,
> .reg = register_snapshot_trigger,
> .unreg = unregister_snapshot_trigger,
> - .get_trigger_ops = snapshot_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> @@ -1558,20 +1545,6 @@ stacktrace_trigger(struct event_trigger_data *data,
> trace_dump_stack(STACK_SKIP);
> }
>
> -static void
> -stacktrace_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> -{
> - if (!data->count)
> - return;
> -
> - if (data->count != -1)
> - (data->count)--;
> -
> - stacktrace_trigger(data, buffer, rec, event);
> -}
> -
> static int
> stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data)
> {
> @@ -1581,32 +1554,20 @@ stacktrace_trigger_print(struct seq_file *m, struct event_trigger_data *data)
>
> static const struct event_trigger_ops stacktrace_trigger_ops = {
> .trigger = stacktrace_trigger,
> + .count_func = event_trigger_count,
> .print = stacktrace_trigger_print,
> .init = event_trigger_init,
> .free = event_trigger_free,
> };
>
> -static const struct event_trigger_ops stacktrace_count_trigger_ops = {
> - .trigger = stacktrace_count_trigger,
> - .print = stacktrace_trigger_print,
> - .init = event_trigger_init,
> - .free = event_trigger_free,
> -};
> -
> -static const struct event_trigger_ops *
> -stacktrace_get_trigger_ops(char *cmd, char *param)
> -{
> - return param ? &stacktrace_count_trigger_ops : &stacktrace_trigger_ops;
> -}
> -
> static struct event_command trigger_stacktrace_cmd = {
> .name = "stacktrace",
> .trigger_type = ETT_STACKTRACE,
> + .trigger_ops = &stacktrace_trigger_ops,
> .flags = EVENT_CMD_FL_POST_TRIGGER,
> .parse = event_trigger_parse,
> .reg = register_trigger,
> .unreg = unregister_trigger,
> - .get_trigger_ops = stacktrace_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> @@ -1642,24 +1603,24 @@ event_enable_trigger(struct event_trigger_data *data,
> set_bit(EVENT_FILE_FL_SOFT_DISABLED_BIT, &enable_data->file->flags);
> }
>
> -static void
> -event_enable_count_trigger(struct event_trigger_data *data,
> - struct trace_buffer *buffer, void *rec,
> - struct ring_buffer_event *event)
> +static bool
> +event_enable_count_func(struct event_trigger_data *data,
> + struct trace_buffer *buffer, void *rec,
> + struct ring_buffer_event *event)
> {
> struct enable_trigger_data *enable_data = data->private_data;
>
> if (!data->count)
> - return;
> + return false;
>
> /* Skip if the event is in a state we want to switch to */
> if (enable_data->enable == !(enable_data->file->flags & EVENT_FILE_FL_SOFT_DISABLED))
> - return;
> + return false;
>
> if (data->count != -1)
> (data->count)--;
>
> - event_enable_trigger(data, buffer, rec, event);
> + return true;
> }
>
> int event_enable_trigger_print(struct seq_file *m,
> @@ -1706,13 +1667,7 @@ void event_enable_trigger_free(struct event_trigger_data *data)
>
> static const struct event_trigger_ops event_enable_trigger_ops = {
> .trigger = event_enable_trigger,
> - .print = event_enable_trigger_print,
> - .init = event_trigger_init,
> - .free = event_enable_trigger_free,
> -};
> -
> -static const struct event_trigger_ops event_enable_count_trigger_ops = {
> - .trigger = event_enable_count_trigger,
> + .count_func = event_enable_count_func,
> .print = event_enable_trigger_print,
> .init = event_trigger_init,
> .free = event_enable_trigger_free,
> @@ -1720,13 +1675,7 @@ static const struct event_trigger_ops event_enable_count_trigger_ops = {
>
> static const struct event_trigger_ops event_disable_trigger_ops = {
> .trigger = event_enable_trigger,
> - .print = event_enable_trigger_print,
> - .init = event_trigger_init,
> - .free = event_enable_trigger_free,
> -};
> -
> -static const struct event_trigger_ops event_disable_count_trigger_ops = {
> - .trigger = event_enable_count_trigger,
> + .count_func = event_enable_count_func,
> .print = event_enable_trigger_print,
> .init = event_trigger_init,
> .free = event_enable_trigger_free,
> @@ -1906,45 +1855,23 @@ void event_enable_unregister_trigger(char *glob,
> data->ops->free(data);
> }
>
> -static const struct event_trigger_ops *
> -event_enable_get_trigger_ops(char *cmd, char *param)
> -{
> - const struct event_trigger_ops *ops;
> - bool enable;
> -
> -#ifdef CONFIG_HIST_TRIGGERS
> - enable = ((strcmp(cmd, ENABLE_EVENT_STR) == 0) ||
> - (strcmp(cmd, ENABLE_HIST_STR) == 0));
> -#else
> - enable = strcmp(cmd, ENABLE_EVENT_STR) == 0;
> -#endif
> - if (enable)
> - ops = param ? &event_enable_count_trigger_ops :
> - &event_enable_trigger_ops;
> - else
> - ops = param ? &event_disable_count_trigger_ops :
> - &event_disable_trigger_ops;
> -
> - return ops;
> -}
> -
> static struct event_command trigger_enable_cmd = {
> .name = ENABLE_EVENT_STR,
> .trigger_type = ETT_EVENT_ENABLE,
> + .trigger_ops = &event_enable_trigger_ops,
> .parse = event_enable_trigger_parse,
> .reg = event_enable_register_trigger,
> .unreg = event_enable_unregister_trigger,
> - .get_trigger_ops = event_enable_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
> static struct event_command trigger_disable_cmd = {
> .name = DISABLE_EVENT_STR,
> .trigger_type = ETT_EVENT_ENABLE,
> + .trigger_ops = &event_disable_trigger_ops,
> .parse = event_enable_trigger_parse,
> .reg = event_enable_register_trigger,
> .unreg = event_enable_unregister_trigger,
> - .get_trigger_ops = event_enable_get_trigger_ops,
> .set_filter = set_trigger_filter,
> };
>
^ permalink raw reply
* Re: [PATCH] ring-buffer: Add helper functions for allocations
From: kernel test robot @ 2025-11-25 12:17 UTC (permalink / raw)
To: Steven Rostedt, LKML, Linux Trace Kernel
Cc: oe-kbuild-all, Masami Hiramatsu, Mathieu Desnoyers
In-Reply-To: <20251124140906.71a2abf6@gandalf.local.home>
Hi Steven,
kernel test robot noticed the following build warnings:
[auto build test WARNING on trace/for-next]
[also build test WARNING on linus/master v6.18-rc7 next-20251125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Steven-Rostedt/ring-buffer-Add-helper-functions-for-allocations/20251125-031044
base: https://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace for-next
patch link: https://lore.kernel.org/r/20251124140906.71a2abf6%40gandalf.local.home
patch subject: [PATCH] ring-buffer: Add helper functions for allocations
config: x86_64-defconfig (https://download.01.org/0day-ci/archive/20251125/202511252040.Jny2Yxxn-lkp@intel.com/config)
compiler: gcc-14 (Debian 14.2.0-19) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20251125/202511252040.Jny2Yxxn-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202511252040.Jny2Yxxn-lkp@intel.com/
All warnings (new ones prefixed by >>):
kernel/trace/ring_buffer.c: In function '__rb_allocate_pages':
>> kernel/trace/ring_buffer.c:2279:30: warning: unused variable 'page' [-Wunused-variable]
2279 | struct page *page;
| ^~~~
>> kernel/trace/ring_buffer.c:2242:15: warning: variable 'mflags' set but not used [-Wunused-but-set-variable]
2242 | gfp_t mflags;
| ^~~~~~
kernel/trace/ring_buffer.c: In function 'rb_allocate_cpu_buffer':
kernel/trace/ring_buffer.c:2362:22: warning: unused variable 'page' [-Wunused-variable]
2362 | struct page *page;
| ^~~~
kernel/trace/ring_buffer.c: In function 'ring_buffer_alloc_read_page':
kernel/trace/ring_buffer.c:6493:22: warning: unused variable 'page' [-Wunused-variable]
6493 | struct page *page;
| ^~~~
vim +/page +2279 kernel/trace/ring_buffer.c
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2234)
74e2afc6df5782 Qiujun Huang 2020-10-15 2235 static int __rb_allocate_pages(struct ring_buffer_per_cpu *cpu_buffer,
74e2afc6df5782 Qiujun Huang 2020-10-15 2236 long nr_pages, struct list_head *pages)
7a8e76a3829f10 Steven Rostedt 2008-09-29 2237 {
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2238) struct trace_buffer *buffer = cpu_buffer->buffer;
4009cc31e7813e Steven Rostedt 2025-03-05 2239 struct ring_buffer_cpu_meta *meta = NULL;
044fa782ebb947 Steven Rostedt 2008-12-02 2240 struct buffer_page *bpage, *tmp;
927e56db625322 Steven Rostedt (VMware 2018-04-04 2241) bool user_thread = current->mm != NULL;
927e56db625322 Steven Rostedt (VMware 2018-04-04 @2242) gfp_t mflags;
9b94a8fba501f3 Steven Rostedt (Red Hat 2016-05-12 2243) long i;
3adc54fa82a68b Steven Rostedt 2009-03-30 2244
927e56db625322 Steven Rostedt (VMware 2018-04-04 2245) /*
927e56db625322 Steven Rostedt (VMware 2018-04-04 2246) * Check if the available memory is there first.
927e56db625322 Steven Rostedt (VMware 2018-04-04 2247) * Note, si_mem_available() only gives us a rough estimate of available
927e56db625322 Steven Rostedt (VMware 2018-04-04 2248) * memory. It may not be accurate. But we don't care, we just want
927e56db625322 Steven Rostedt (VMware 2018-04-04 2249) * to prevent doing any allocation when it is obvious that it is
927e56db625322 Steven Rostedt (VMware 2018-04-04 2250) * not going to succeed.
927e56db625322 Steven Rostedt (VMware 2018-04-04 2251) */
2a872fa4e9c8ad Steven Rostedt (VMware 2018-04-02 2252) i = si_mem_available();
2a872fa4e9c8ad Steven Rostedt (VMware 2018-04-02 2253) if (i < nr_pages)
2a872fa4e9c8ad Steven Rostedt (VMware 2018-04-02 2254) return -ENOMEM;
2a872fa4e9c8ad Steven Rostedt (VMware 2018-04-02 2255)
d7ec4bfed6c974 Vaibhav Nagarnaik 2011-06-07 2256 /*
848618857d2535 Joel Fernandes 2017-07-12 2257 * __GFP_RETRY_MAYFAIL flag makes sure that the allocation fails
848618857d2535 Joel Fernandes 2017-07-12 2258 * gracefully without invoking oom-killer and the system is not
848618857d2535 Joel Fernandes 2017-07-12 2259 * destabilized.
d7ec4bfed6c974 Vaibhav Nagarnaik 2011-06-07 2260 */
927e56db625322 Steven Rostedt (VMware 2018-04-04 2261) mflags = GFP_KERNEL | __GFP_RETRY_MAYFAIL;
927e56db625322 Steven Rostedt (VMware 2018-04-04 2262)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2263) /*
927e56db625322 Steven Rostedt (VMware 2018-04-04 2264) * If a user thread allocates too much, and si_mem_available()
927e56db625322 Steven Rostedt (VMware 2018-04-04 2265) * reports there's enough memory, even though there is not.
927e56db625322 Steven Rostedt (VMware 2018-04-04 2266) * Make sure the OOM killer kills this thread. This can happen
927e56db625322 Steven Rostedt (VMware 2018-04-04 2267) * even with RETRY_MAYFAIL because another task may be doing
927e56db625322 Steven Rostedt (VMware 2018-04-04 2268) * an allocation after this task has taken all memory.
927e56db625322 Steven Rostedt (VMware 2018-04-04 2269) * This is the task the OOM killer needs to take out during this
927e56db625322 Steven Rostedt (VMware 2018-04-04 2270) * loop, even if it was triggered by an allocation somewhere else.
927e56db625322 Steven Rostedt (VMware 2018-04-04 2271) */
927e56db625322 Steven Rostedt (VMware 2018-04-04 2272) if (user_thread)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2273) set_current_oom_origin();
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2274)
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2275) if (buffer->range_addr_start)
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2276) meta = rb_range_meta(buffer, nr_pages, cpu_buffer->cpu);
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2277)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2278) for (i = 0; i < nr_pages; i++) {
927e56db625322 Steven Rostedt (VMware 2018-04-04 @2279) struct page *page;
927e56db625322 Steven Rostedt (VMware 2018-04-04 2280)
1e3d56c5556b8d Steven Rostedt 2025-11-24 2281 bpage = alloc_cpu_page(cpu_buffer->cpu);
044fa782ebb947 Steven Rostedt 2008-12-02 2282 if (!bpage)
e4c2ce82ca2710 Steven Rostedt 2008-10-01 2283 goto free_pages;
77ae365eca8950 Steven Rostedt 2009-03-27 2284
74e2afc6df5782 Qiujun Huang 2020-10-15 2285 rb_check_bpage(cpu_buffer, bpage);
74e2afc6df5782 Qiujun Huang 2020-10-15 2286
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2287) /*
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2288) * Append the pages as for mapped buffers we want to keep
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2289) * the order
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2290) */
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2291) list_add_tail(&bpage->list, pages);
e4c2ce82ca2710 Steven Rostedt 2008-10-01 2292
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2293) if (meta) {
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2294) /* A range was given. Use that for the buffer page */
b14d032973d4e6 Steven Rostedt (Google 2024-06-12 2295) bpage->page = rb_range_buffer(cpu_buffer, i + 1);
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2296) if (!bpage->page)
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2297) goto free_pages;
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2298) /* If this is valid from a previous boot */
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2299) if (meta->head_buffer)
c76883f18e59b7 Steven Rostedt (Google 2024-06-12 2300) rb_meta_buffer_update(cpu_buffer, bpage);
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2301) bpage->range = 1;
b14d032973d4e6 Steven Rostedt (Google 2024-06-12 2302) bpage->id = i + 1;
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2303) } else {
1e3d56c5556b8d Steven Rostedt 2025-11-24 2304 int order = cpu_buffer->buffer->subbuf_order;
1e3d56c5556b8d Steven Rostedt 2025-11-24 2305 bpage->page = alloc_cpu_data(cpu_buffer->cpu, order);
1e3d56c5556b8d Steven Rostedt 2025-11-24 2306 if (!bpage->page)
7a8e76a3829f10 Steven Rostedt 2008-09-29 2307 goto free_pages;
be68d63a139bd4 Steven Rostedt (Google 2024-06-12 2308) }
f9b94daa542a8d Tzvetomir Stoyanov (VMware 2023-12-19 2309) bpage->order = cpu_buffer->buffer->subbuf_order;
927e56db625322 Steven Rostedt (VMware 2018-04-04 2310)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2311) if (user_thread && fatal_signal_pending(current))
927e56db625322 Steven Rostedt (VMware 2018-04-04 2312) goto free_pages;
7a8e76a3829f10 Steven Rostedt 2008-09-29 2313 }
927e56db625322 Steven Rostedt (VMware 2018-04-04 2314) if (user_thread)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2315) clear_current_oom_origin();
7a8e76a3829f10 Steven Rostedt 2008-09-29 2316
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2317 return 0;
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2318
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2319 free_pages:
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2320 list_for_each_entry_safe(bpage, tmp, pages, list) {
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2321 list_del_init(&bpage->list);
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2322 free_buffer_page(bpage);
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2323 }
927e56db625322 Steven Rostedt (VMware 2018-04-04 2324) if (user_thread)
927e56db625322 Steven Rostedt (VMware 2018-04-04 2325) clear_current_oom_origin();
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2326
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2327 return -ENOMEM;
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2328 }
438ced1720b584 Vaibhav Nagarnaik 2012-02-02 2329
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH v8 21/28] KVM: arm64: Add tracing capability for the pKVM hyp
From: Vincent Donnefort @ 2025-11-25 11:22 UTC (permalink / raw)
To: Marc Zyngier
Cc: rostedt, mhiramat, mathieu.desnoyers, linux-trace-kernel,
oliver.upton, joey.gouly, suzuki.poulose, yuzenghui, kvmarm,
linux-arm-kernel, jstultz, qperret, will, aneesh.kumar,
kernel-team, linux-kernel
In-Reply-To: <86bjkyrly9.wl-maz@kernel.org>
[...]
> > diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> > index f55a9a17d38f..504c3b9caef8 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> > +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> > @@ -29,7 +29,7 @@ hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
> > ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o
> > hyp-obj-y += ../../../kernel/smccc-call.o
> > hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
> > -hyp-obj-$(CONFIG_PKVM_TRACING) += clock.o
> > +hyp-obj-$(CONFIG_PKVM_TRACING) += clock.o trace.o ../../../../../kernel/trace/simple_ring_buffer.o
>
> Can we get something less awful here? Surely there is a way to get an
> absolute path from the kbuild infrastructure? $(objtree) springs to
> mind...
Because of the path substitution below, I can't simply use $(objtree) here. I
suppose that's why all other paths above are relative.
However, I could probably simplify by including simple_ring_buffer.c into trace.c:
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
-hyp-obj-$(CONFIG_TRACING) += clock.o trace.o ../../../../../kernel/trace/simple_ring_buffer.o
+hyp-obj-$(CONFIG_TRACING) += clock.o trace.o
hyp-obj-y += $(lib-objs)
+# Path to simple_ring_buffer.c
+CFLAGS_trace.nvhe.o += -I$(objtree)/kernel/trace/
+
diff --git a/arch/arm64/kvm/hyp/nvhe/trace.c b/arch/arm64/kvm/hyp/nvhe/trace.c
-#include <linux/simple_ring_buffer.h>
+#include "simple_ring_buffer.c"
>
> > hyp-obj-y += $(lib-objs)
> >
> > ##
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
[...]
^ permalink raw reply
* Re: [PATCH] blk-trace: Fix potential buffer overflow in blk_trace_setup()
From: Christoph Hellwig @ 2025-11-25 11:12 UTC (permalink / raw)
To: Huiwen He
Cc: Jens Axboe, Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
linux-block, linux-kernel, linux-trace-kernel
In-Reply-To: <20251125082420.856030-1-hehuiwen@kylinos.cn>
On Tue, Nov 25, 2025 at 04:24:20PM +0800, Huiwen He wrote:
> The legacy struct blk_user_trace_setup has a 32-byte name field,
> while buts2->name is a 64-byte buffer (BLKTRACE_BDEV_SIZE2).
>
> Since commit 113cbd62824a ("blktrace: pass blk_user_trace2 to setup
> functions"), blk_trace_setup() copied buts2->name into buts->name
> using strcpy(). strcpy() performs no bounds checking on the destination
> buffer, which can overflow if the source string exceeds 31 characters.
>
> Replace deprecated [1] strcpy() with strscpy() to ensure proper bounds
> checking and prevent potential buffer overflow.
At this point all this has been checked as part of the setup. If you
hatr strcpy with passing, just doing a memcpy of BLKTRACE_BDEV_SIZE2
is the saner alternative.
^ permalink raw reply
* [PATCH v9] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-25 9:34 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, pengdonglin, Sven Schnelle,
Masami Hiramatsu, Xiaoqin Zhang
From: pengdonglin <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by allowing
funcgraph-retaddr to have an args array.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Xiaoqin Zhang <zhangxiaoqin@xiaomi.com>
Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
---
v9:
- Fix typo
- Link to v8: https://lore.kernel.org/linux-trace-kernel/20251125092711.2558787-1-dolinux.peng@gmail.com/
v8:
- Add the retaddr member back and reconstruct the patch
- Rebase on the trace/for-next branch
- Link to v7: https://lore.kernel.org/linux-trace-kernel/20251114025522.2115359-1-dolinux.peng@gmail.com/
v7:
- Remove nr_args to eliminate a multiplication instruction
- Link to v6: https://lore.kernel.org/all/20251114023453.2061590-1-dolinux.peng@gmail.com/
v6:
- Initialize retaddr to NULL to silence uninitialized variable warning.
- Use ARGS_OFFS(size) macro instead of args_offs variable to eliminate
"set but not used" warning
- Link to v5: https://lore.kernel.org/all/20251113072938.333657-1-dolinux.peng@gmail.com/
v5:
- Avoid unnecessary branch overhead by first verifying
CONFIG_FUNCTION_GRAPH_RETADDR is enabled prior to testing
the TRACE_GRAPH_PRINT_RETADDR flag
- Link to v4: https://lore.kernel.org/all/20251112093650.3345108-1-dolinux.peng@gmail.com/
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
- Resend to add missing add Acked-by tags
- Link to v3: https://lore.kernel.org/all/20251112034333.2901601-1-dolinux.peng@gmail.com/
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
- Link to v2: https://lore.kernel.org/all/20251111135432.2143993-1-dolinux.peng@gmail.com/
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
- Link to v1: https://lore.kernel.org/all/20251011164156.3678012-1-dolinux.peng@gmail.com/
---
include/linux/ftrace.h | 7 +--
kernel/trace/trace.h | 24 +++++++++-
kernel/trace/trace_entries.h | 15 +++---
kernel/trace/trace_functions_graph.c | 71 ++++++++++++++++++----------
4 files changed, 80 insertions(+), 37 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..6ca9c6229d93 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1126,17 +1126,14 @@ static inline void ftrace_init(void) { }
*/
struct ftrace_graph_ent {
unsigned long func; /* Current function */
- int depth;
+ unsigned long depth;
} __packed;
/*
* Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
*/
struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
+ struct ftrace_graph_ent ent;
unsigned long retaddr; /* Return address */
} __packed;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 58be6d741d72..a06d836b7d92 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -962,7 +962,8 @@ extern int __trace_graph_entry(struct trace_array *tr,
extern int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr);
+ unsigned long retaddr,
+ struct ftrace_regs *fregs);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
@@ -2287,4 +2288,25 @@ static inline int rv_init_interface(void)
*/
#define FTRACE_TRAMPOLINE_MARKER ((unsigned long) INT_MAX)
+/*
+ * This is used to get the address of the args array based on
+ * the type of the entry.
+ */
+#define FGRAPH_ENTRY_ARGS(e) \
+ ({ \
+ unsigned long *_args; \
+ struct ftrace_graph_ent_entry *_e = e; \
+ \
+ if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) && \
+ e->ent.type == TRACE_GRAPH_RETADDR_ENT) { \
+ struct fgraph_retaddr_ent_entry *_re; \
+ \
+ _re = (typeof(_re))_e; \
+ _args = _re->args; \
+ } else { \
+ _args = _e->args; \
+ } \
+ _args; \
+ })
+
#endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..f6a8d29c0d76 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -80,11 +80,11 @@ FTRACE_ENTRY(funcgraph_entry, ftrace_graph_ent_entry,
F_STRUCT(
__field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
+ __field_packed( unsigned long, graph_ent, depth )
__dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u)", (void *)__entry->func, __entry->depth)
+ F_printk("--> %ps (%lu)", (void *)__entry->func, __entry->depth)
);
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
@@ -95,13 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
- __field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __field_struct( struct fgraph_retaddr_ent, graph_rent )
+ __field_packed( unsigned long, graph_rent.ent, func )
+ __field_packed( unsigned long, graph_rent.ent, depth )
+ __field_packed( unsigned long, graph_rent, retaddr )
+ __dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
+ F_printk("--> %ps (%lu) <- %ps", (void *)__entry->func, __entry->depth,
(void *)__entry->retaddr)
);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 44d5dc5031e2..8a075d3c4563 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -36,14 +36,19 @@ struct fgraph_ent_args {
unsigned long args[FTRACE_REGS_MAX_ARGS];
};
+struct fgraph_retaddr_ent_args {
+ struct fgraph_retaddr_ent_entry ent;
+ /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
+ unsigned long args[FTRACE_REGS_MAX_ARGS];
+};
+
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
union {
struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
+ struct fgraph_retaddr_ent_args rent;
};
struct ftrace_graph_ret_entry ret;
int failed;
@@ -160,20 +165,32 @@ int __trace_graph_entry(struct trace_array *tr,
int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr)
+ unsigned long retaddr,
+ struct ftrace_regs *fregs)
{
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct fgraph_retaddr_ent_entry *entry;
+ int size;
+
+ /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
+ size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
+ size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
+ entry->graph_rent.ent = *trace;
+ entry->graph_rent.retaddr = retaddr;
+
+#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
+ if (fregs) {
+ for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
+ entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ }
+#endif
+
trace_buffer_unlock_commit_nostack(buffer, event);
return 1;
@@ -182,7 +199,8 @@ int __trace_graph_retaddr_entry(struct trace_array *tr,
int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr)
+ unsigned long retaddr,
+ struct ftrace_regs *fregs)
{
return 1;
}
@@ -267,7 +285,8 @@ static int graph_entry(struct ftrace_graph_ent *trace,
if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
tracer_flags_is_set(tr, TRACE_GRAPH_PRINT_RETADDR)) {
unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
+ ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx,
+ retaddr, fregs);
} else {
ret = __graph_entry(tr, trace, trace_ctx, fregs);
}
@@ -654,13 +673,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->rent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->rent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -838,7 +853,7 @@ static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_e
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym_offset(s, entry->graph_ent.retaddr, trace_flags);
+ seq_print_ip_sym_offset(s, entry->graph_rent.retaddr, trace_flags);
if (comment)
trace_seq_puts(s, " */");
@@ -984,7 +999,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, FGRAPH_ENTRY_ARGS(entry), ret_func);
trace_seq_putc(s, ';');
} else
trace_seq_puts(s, "();");
@@ -1036,7 +1051,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, FGRAPH_ENTRY_ARGS(entry), func);
else
trace_seq_puts(s, "()");
@@ -1218,11 +1233,14 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
/*
* print_graph_entry() may consume the current event,
* thus @field may become invalid, so we need to save it.
- * sizeof(struct ftrace_graph_ent_entry) is very small,
- * it can be safely saved at the stack.
+ * This function is shared by ftrace_graph_ent_entry and
+ * fgraph_retaddr_ent_entry, the size of the latter one
+ * is larger, but it is very small and can be safely saved
+ * at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ struct fgraph_retaddr_ent_entry *rentry;
+ u8 save_buf[sizeof(*rentry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1451,12 +1469,17 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
}
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
+ /*
+ * ftrace_graph_ent_entry and fgraph_retaddr_ent_entry have
+ * similar functions and memory layouts. The only difference
+ * is that the latter one has an extra retaddr member, so
+ * they can share most of the logic.
+ */
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((struct ftrace_graph_ent_entry *)rfield,
+ s, iter, flags);
}
#endif
case TRACE_GRAPH_RET: {
--
2.34.1
^ permalink raw reply related
* [PATCH for-next v8] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-25 9:27 UTC (permalink / raw)
To: rostedt
Cc: linux-trace-kernel, linux-kernel, pengdonglin, Sven Schnelle,
Masami Hiramatsu, Xiaoqin Zhang
From: pengdonglin <pengdonglin@xiaomi.com>
Currently, the funcgraph-args and funcgraph-retaddr features are
mutually exclusive. This patch resolves this limitation by allowing
funcgraph-retaddr to have an args array.
To verify the change, use perf to trace vfs_write with both options
enabled:
Before:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read() { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(); /* <-down_read+0x3b/0x160 */
preempt_count_sub(); /* <-down_read+0x8b/0x160 */
}
After:
# perf ftrace -G vfs_write --graph-opts args,retaddr
......
down_read(sem=0xffff8880100bea78) { /* <-n_tty_write+0xa3/0x540 */
__cond_resched(); /* <-down_read+0x12/0x160 */
preempt_count_add(val=1); /* <-down_read+0x3b/0x160 */
preempt_count_sub(val=1); /* <-down_read+0x8b/0x160 */
}
Cc: Steven Rostedt (Google) <rostedt@goodmis.org>
Cc: Sven Schnelle <svens@linux.ibm.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Xiaoqin Zhang <zhangxiaoqin@xiaomi.com>
Signed-off-by: pengdonglin <pengdonglin@xiaomi.com>
---
v8:
- Add the retaddr member back and reconstruct the patch
- Rebase on the trace/for-next branch
- Link to v7: https://lore.kernel.org/linux-trace-kernel/20251114025522.2115359-1-dolinux.peng@gmail.com/
v7:
- Remove nr_args to eliminate a multiplication instruction
- Link to v6: https://lore.kernel.org/all/20251114023453.2061590-1-dolinux.peng@gmail.com/
v6:
- Initialize retaddr to NULL to silence uninitialized variable warning.
- Use ARGS_OFFS(size) macro instead of args_offs variable to eliminate
"set but not used" warning
- Link to v5: https://lore.kernel.org/all/20251113072938.333657-1-dolinux.peng@gmail.com/
v5:
- Avoid unnecessary branch overhead by first verifying
CONFIG_FUNCTION_GRAPH_RETADDR is enabled prior to testing
the TRACE_GRAPH_PRINT_RETADDR flag
- Link to v4: https://lore.kernel.org/all/20251112093650.3345108-1-dolinux.peng@gmail.com/
v4:
- Remove redundant TRACE_GRAPH_ARGS flag check
- Eliminate unnecessary retaddr initialization
- Resend to add missing add Acked-by tags
- Link to v3: https://lore.kernel.org/all/20251112034333.2901601-1-dolinux.peng@gmail.com/
v3:
- Replace min() with min_t() to improve type safety and maintainability
- Keep only one Signed-off-by for cleaner attribution
- Code refactoring for improved readability
- Link to v2: https://lore.kernel.org/all/20251111135432.2143993-1-dolinux.peng@gmail.com/
v2:
- Preserve retaddr event functionality (suggested by Steven)
- Store the retaddr in args[0] (suggested by Steven)
- Refactor implementation logic and commit message clarity
- Link to v1: https://lore.kernel.org/all/20251011164156.3678012-1-dolinux.peng@gmail.com/
---
include/linux/ftrace.h | 7 +--
kernel/trace/trace.h | 24 +++++++++-
kernel/trace/trace_entries.h | 15 +++---
kernel/trace/trace_functions_graph.c | 71 ++++++++++++++++++----------
4 files changed, 80 insertions(+), 37 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 7ded7df6e9b5..6ca9c6229d93 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1126,17 +1126,14 @@ static inline void ftrace_init(void) { }
*/
struct ftrace_graph_ent {
unsigned long func; /* Current function */
- int depth;
+ unsigned long depth;
} __packed;
/*
* Structure that defines an entry function trace with retaddr.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
*/
struct fgraph_retaddr_ent {
- unsigned long func; /* Current function */
- int depth;
+ struct ftrace_graph_ent ent;
unsigned long retaddr; /* Return address */
} __packed;
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 58be6d741d72..a06d836b7d92 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -962,7 +962,8 @@ extern int __trace_graph_entry(struct trace_array *tr,
extern int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr);
+ unsigned long retaddr,
+ struct ftrace_regs *fregs);
extern void __trace_graph_return(struct trace_array *tr,
struct ftrace_graph_ret *trace,
unsigned int trace_ctx,
@@ -2287,4 +2288,25 @@ static inline int rv_init_interface(void)
*/
#define FTRACE_TRAMPOLINE_MARKER ((unsigned long) INT_MAX)
+/*
+ * This is used to get the address of the args array based on
+ * the type of the entry.
+ */
+#define FGRAPH_ENTRY_ARGS(e) \
+ ({ \
+ unsigned long *_args; \
+ struct ftrace_graph_ent_entry *_e = e; \
+ \
+ if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) && \
+ e->ent.type == TRACE_GRAPH_RETADDR_ENT) { \
+ struct fgraph_retaddr_ent_entry *_re; \
+ \
+ _re = (typeof(_re))_e; \
+ _args = _re->args; \
+ } else { \
+ _args = _e->args; \
+ } \
+ _args; \
+ })
+
#endif /* _LINUX_KERNEL_TRACE_H */
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index de294ae2c5c5..f6a8d29c0d76 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -80,11 +80,11 @@ FTRACE_ENTRY(funcgraph_entry, ftrace_graph_ent_entry,
F_STRUCT(
__field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
+ __field_packed( unsigned long, graph_ent, depth )
__dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u)", (void *)__entry->func, __entry->depth)
+ F_printk("--> %ps (%lu)", (void *)__entry->func, __entry->depth)
);
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
@@ -95,13 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct fgraph_retaddr_ent, graph_ent )
- __field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
- __field_packed( unsigned long, graph_ent, retaddr )
+ __field_struct( struct fgraph_retaddr_ent, graph_rent )
+ __field_packed( unsigned long, graph_rent.ent, func )
+ __field_packed( unsigned long, graph_rent.ent, depth )
+ __field_packed( unsigned long, graph_rent, retaddr )
+ __dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
+ F_printk("--> %ps (%lu) <- %ps", (void *)__entry->func, __entry->depth,
(void *)__entry->retaddr)
);
diff --git a/kernel/trace/trace_functions_graph.c b/kernel/trace/trace_functions_graph.c
index 44d5dc5031e2..b04dcbdb236f 100644
--- a/kernel/trace/trace_functions_graph.c
+++ b/kernel/trace/trace_functions_graph.c
@@ -36,14 +36,19 @@ struct fgraph_ent_args {
unsigned long args[FTRACE_REGS_MAX_ARGS];
};
+struct fgraph_retaddr_ent_args {
+ struct fgraph_retaddr_ent_entry ent;
+ /* Force the sizeof of args[] to have FTRACE_REGS_MAX_ARGS entries */
+ unsigned long args[FTRACE_REGS_MAX_ARGS];
+};
+
struct fgraph_data {
struct fgraph_cpu_data __percpu *cpu_data;
/* Place to preserve last processed entry. */
union {
struct fgraph_ent_args ent;
- /* TODO allow retaddr to have args */
- struct fgraph_retaddr_ent_entry rent;
+ struct fgraph_retaddr_ent_args rent;
};
struct ftrace_graph_ret_entry ret;
int failed;
@@ -160,20 +165,32 @@ int __trace_graph_entry(struct trace_array *tr,
int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr)
+ unsigned long retaddr,
+ struct ftrace_regs *fregs)
{
struct ring_buffer_event *event;
struct trace_buffer *buffer = tr->array_buffer.buffer;
struct fgraph_retaddr_ent_entry *entry;
+ int size;
+
+ /* If fregs is defined, add FTRACE_REGS_MAX_ARGS long size words */
+ size = sizeof(*entry) + (FTRACE_REGS_MAX_ARGS * !!fregs * sizeof(long));
event = trace_buffer_lock_reserve(buffer, TRACE_GRAPH_RETADDR_ENT,
- sizeof(*entry), trace_ctx);
+ size, trace_ctx);
if (!event)
return 0;
entry = ring_buffer_event_data(event);
- entry->graph_ent.func = trace->func;
- entry->graph_ent.depth = trace->depth;
- entry->graph_ent.retaddr = retaddr;
+ entry->graph_rent.ent = *trace;
+ entry->graph_rent.retaddr = retaddr;
+
+#ifdef CONFIG_HAVE_FUNCTION_ARG_ACCESS_API
+ if (fregs) {
+ for (int i = 0; i < FTRACE_REGS_MAX_ARGS; i++)
+ entry->args[i] = ftrace_regs_get_argument(fregs, i);
+ }
+#endif
+
trace_buffer_unlock_commit_nostack(buffer, event);
return 1;
@@ -182,7 +199,8 @@ int __trace_graph_retaddr_entry(struct trace_array *tr,
int __trace_graph_retaddr_entry(struct trace_array *tr,
struct ftrace_graph_ent *trace,
unsigned int trace_ctx,
- unsigned long retaddr)
+ unsigned long retaddr,
+ struct ftrace_regs *fregs)
{
return 1;
}
@@ -267,7 +285,8 @@ static int graph_entry(struct ftrace_graph_ent *trace,
if (IS_ENABLED(CONFIG_FUNCTION_GRAPH_RETADDR) &&
tracer_flags_is_set(tr, TRACE_GRAPH_PRINT_RETADDR)) {
unsigned long retaddr = ftrace_graph_top_ret_addr(current);
- ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx, retaddr);
+ ret = __trace_graph_retaddr_entry(tr, trace, trace_ctx,
+ retaddr, fregs);
} else {
ret = __graph_entry(tr, trace, trace_ctx, fregs);
}
@@ -654,13 +673,9 @@ get_return_for_leaf(struct trace_iterator *iter,
* Save current and next entries for later reference
* if the output fails.
*/
- if (unlikely(curr->ent.type == TRACE_GRAPH_RETADDR_ENT)) {
- data->rent = *(struct fgraph_retaddr_ent_entry *)curr;
- } else {
- int size = min((int)sizeof(data->ent), (int)iter->ent_size);
+ int size = min_t(int, sizeof(data->rent), iter->ent_size);
- memcpy(&data->ent, curr, size);
- }
+ memcpy(&data->rent, curr, size);
/*
* If the next event is not a return type, then
* we only care about what type it is. Otherwise we can
@@ -838,7 +853,7 @@ static void print_graph_retaddr(struct trace_seq *s, struct fgraph_retaddr_ent_e
trace_seq_puts(s, " /*");
trace_seq_puts(s, " <-");
- seq_print_ip_sym_offset(s, entry->graph_ent.retaddr, trace_flags);
+ seq_print_ip_sym_offset(s, entry->graph_rent.retaddr, trace_flags);
if (comment)
trace_seq_puts(s, " */");
@@ -984,7 +999,7 @@ print_graph_entry_leaf(struct trace_iterator *iter,
trace_seq_printf(s, "%ps", (void *)ret_func);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long)) {
- print_function_args(s, entry->args, ret_func);
+ print_function_args(s, FGRAPH_ENTRY_ARGS(entry), ret_func);
trace_seq_putc(s, ';');
} else
trace_seq_puts(s, "();");
@@ -1036,7 +1051,7 @@ print_graph_entry_nested(struct trace_iterator *iter,
args_size = iter->ent_size - offsetof(struct ftrace_graph_ent_entry, args);
if (args_size >= FTRACE_REGS_MAX_ARGS * sizeof(long))
- print_function_args(s, entry->args, func);
+ print_function_args(s, FGRAPH_ENTRY_ARGS(entry), func);
else
trace_seq_puts(s, "()");
@@ -1218,11 +1233,14 @@ print_graph_entry(struct ftrace_graph_ent_entry *field, struct trace_seq *s,
/*
* print_graph_entry() may consume the current event,
* thus @field may become invalid, so we need to save it.
- * sizeof(struct ftrace_graph_ent_entry) is very small,
- * it can be safely saved at the stack.
+ * This function is shared by ftrace_graph_ent_entry and
+ * fgraph_retaddr_ent_entry, the size of the latter one
+ * is larger, but it is very small and can be safely saved
+ * at the stack.
*/
struct ftrace_graph_ent_entry *entry;
- u8 save_buf[sizeof(*entry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
+ struct fgraph_retaddr_ent_entry *rentry;
+ u8 save_buf[sizeof(*rentry) + FTRACE_REGS_MAX_ARGS * sizeof(long)];
/* The ent_size is expected to be as big as the entry */
if (iter->ent_size > sizeof(save_buf))
@@ -1451,12 +1469,17 @@ print_graph_function_flags(struct trace_iterator *iter, u32 flags)
}
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
case TRACE_GRAPH_RETADDR_ENT: {
- struct fgraph_retaddr_ent_entry saved;
+ /*
+ * ftrace_graph_ent_entry and fgraph_retaddr_ent_entry have
+ * similar functions and memory layouts. The only difference
+ * is that the latter once has an extra retaddr member, so
+ * they can share most of the logic.
+ */
struct fgraph_retaddr_ent_entry *rfield;
trace_assign_type(rfield, entry);
- saved = *rfield;
- return print_graph_entry((struct ftrace_graph_ent_entry *)&saved, s, iter, flags);
+ return print_graph_entry((struct ftrace_graph_ent_entry *)rfield,
+ s, iter, flags);
}
#endif
case TRACE_GRAPH_RET: {
--
2.34.1
^ permalink raw reply related
* Re: [rtla 04/13] rtla: Replace atoi() with a robust strtoi()
From: Costa Shulyupin @ 2025-11-25 8:35 UTC (permalink / raw)
To: Wander Lairson Costa
Cc: Steven Rostedt, Tomas Glozar, Ivan Pravdin, Crystal Wood,
John Kacur, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:b|_)bpf(?:b|_)
In-Reply-To: <20251117184409.42831-5-wander@redhat.com>
On Mon, 17 Nov 2025 at 20:55, Wander Lairson Costa <wander@redhat.com> wrote:
> To address this, introduce a new strtoi() helper function that safely
> converts a string to an integer. This function validates the input and
> checks for overflows, returning a boolean to indicate success or failure.
Why not use sscanf() for this purpose instead of adding a new utility function?
Also, using a boolean to return success or failure does not conform to
POSIX standards and is confusing in Linux/POSIX code.
Costa
^ permalink raw reply
* [PATCH] blk-trace: Fix potential buffer overflow in blk_trace_setup()
From: Huiwen He @ 2025-11-25 8:24 UTC (permalink / raw)
To: Jens Axboe
Cc: Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers, linux-block,
linux-kernel, linux-trace-kernel, Huiwen He
The legacy struct blk_user_trace_setup has a 32-byte name field,
while buts2->name is a 64-byte buffer (BLKTRACE_BDEV_SIZE2).
Since commit 113cbd62824a ("blktrace: pass blk_user_trace2 to setup
functions"), blk_trace_setup() copied buts2->name into buts->name
using strcpy(). strcpy() performs no bounds checking on the destination
buffer, which can overflow if the source string exceeds 31 characters.
Replace deprecated [1] strcpy() with strscpy() to ensure proper bounds
checking and prevent potential buffer overflow.
Link: https://github.com/KSPP/linux/issues/88 [1]
Fixes: 113cbd62824a ("blktrace: pass blk_user_trace2 to setup functions")
Signed-off-by: Huiwen He <hehuiwen@kylinos.cn>
---
kernel/trace/blktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/trace/blktrace.c b/kernel/trace/blktrace.c
index d031c8d80be4..50460e2e7212 100644
--- a/kernel/trace/blktrace.c
+++ b/kernel/trace/blktrace.c
@@ -793,7 +793,7 @@ int blk_trace_setup(struct request_queue *q, char *name, dev_t dev,
return PTR_ERR(bt);
}
blk_trace_setup_finalize(q, name, 1, bt, &buts2);
- strcpy(buts.name, buts2.name);
+ strscpy(buts.name, buts2.name);
mutex_unlock(&q->debugfs_mutex);
if (copy_to_user(arg, &buts, sizeof(buts))) {
--
2.25.1
^ permalink raw reply related
* [RFC PATCH net-next] page_pool: Add page_pool_release_stalled tracepoint
From: Leon Hwang @ 2025-11-25 8:22 UTC (permalink / raw)
To: netdev
Cc: hawk, ilias.apalodimas, rostedt, mhiramat, mathieu.desnoyers,
davem, edumazet, kuba, pabeni, horms, kerneljasonxing, lance.yang,
jiayuan.chen, linux-kernel, linux-trace-kernel, Leon Hwang,
Leon Huang Fu
Introduce a new tracepoint to track stalled page pool releases,
providing better observability for page pool lifecycle issues.
Problem:
Currently, when a page pool shutdown is stalled due to inflight pages,
the kernel only logs a warning message via pr_warn(). This has several
limitations:
1. The warning floods the kernel log after the initial DEFER_WARN_INTERVAL,
making it difficult to track the progression of stalled releases
2. There's no structured way to monitor or analyze these events
3. Debugging tools cannot easily capture and correlate stalled pool
events with other network activity
Solution:
Add a new tracepoint, page_pool_release_stalled, that fires when a page
pool shutdown is stalled. The tracepoint captures:
- pool: pointer to the stalled page_pool
- inflight: number of pages still in flight
- sec: seconds since the release was deferred
The implementation also modifies the logging behavior:
- pr_warn() is only emitted during the first warning interval
(DEFER_WARN_INTERVAL to DEFER_WARN_INTERVAL*2)
- The tracepoint is fired always, reducing log noise while still
allowing monitoring tools to track the issue
This allows developers and system administrators to:
- Use tools like perf, ftrace, or eBPF to monitor stalled releases
- Correlate page pool issues with network driver behavior
- Analyze patterns without parsing kernel logs
- Track the progression of inflight page counts over time
Signed-off-by: Leon Huang Fu <leon.huangfu@shopee.com>
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
---
include/trace/events/page_pool.h | 22 ++++++++++++++++++++++
net/core/page_pool.c | 6 ++++--
2 files changed, 26 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/page_pool.h b/include/trace/events/page_pool.h
index 31825ed30032..c4205af9bf93 100644
--- a/include/trace/events/page_pool.h
+++ b/include/trace/events/page_pool.h
@@ -113,6 +113,28 @@ TRACE_EVENT(page_pool_update_nid,
__entry->pool, __entry->pool_nid, __entry->new_nid)
);
+TRACE_EVENT(page_pool_release_stalled,
+
+ TP_PROTO(const struct page_pool *pool, int inflight, int sec),
+
+ TP_ARGS(pool, inflight, sec),
+
+ TP_STRUCT__entry(
+ __field(const struct page_pool *, pool)
+ __field(int, inflight)
+ __field(int, sec)
+ ),
+
+ TP_fast_assign(
+ __entry->pool = pool;
+ __entry->inflight = inflight;
+ __entry->sec = sec;
+ ),
+
+ TP_printk("page_pool=%p id=%d inflight=%d sec=%d",
+ __entry->pool, __entry->pool->user.id, __entry->inflight, __entry->sec)
+);
+
#endif /* _TRACE_PAGE_POOL_H */
/* This part must be outside protection */
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a5edec485f1..9fd86749c705 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -1218,8 +1218,10 @@ static void page_pool_release_retry(struct work_struct *wq)
(!netdev || netdev == NET_PTR_POISON)) {
int sec = (s32)((u32)jiffies - (u32)pool->defer_start) / HZ;
- pr_warn("%s() stalled pool shutdown: id %u, %d inflight %d sec\n",
- __func__, pool->user.id, inflight, sec);
+ if (sec >= DEFER_WARN_INTERVAL / HZ && sec < DEFER_WARN_INTERVAL * 2 / HZ)
+ pr_warn("%s() stalled pool shutdown: id %u, %d inflight %d sec\n",
+ __func__, pool->user.id, inflight, sec);
+ trace_page_pool_release_stalled(pool, inflight, sec);
pool->defer_warn = jiffies + DEFER_WARN_INTERVAL;
}
--
2.52.0
^ permalink raw reply related
* Re: 答复: [External Mail]Re: [PATCH] dma-buf: add some tracepoints to debug.
From: Christian König @ 2025-11-25 7:52 UTC (permalink / raw)
To: 高翔, Xiang Gao, sumit.semwal@linaro.org,
rostedt@goodmis.org, mhiramat@kernel.org
Cc: linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org,
mathieu.desnoyers@efficios.com, dhowells@redhat.com,
kuba@kernel.org, brauner@kernel.org, akpm@linux-foundation.org,
linux-trace-kernel@vger.kernel.org
In-Reply-To: <c6aeed35a17b4fb6b30c5b58a464aba5@xiaomi.com>
On 11/25/25 03:01, 高翔 wrote:
>> In general quite nice to have, but this needs a bit more explanation why you need it.
>
> I want to track the status of dmabuf in real time in the production environment.
Please add that to the commit message.
>
>> I have strongly doubts that this is your legal name :)
>> Please google the requirements for a Signed-off-by line.
> That's my real name, gaoxiang. However, there are quite a few people with the same name in the company, and I rank 17th.
It doesn't matter what your eMail address is, but the name in a Signed-off-by must be your legal name.
In other words the same what you would use on a contract, usually including first and last name.
It could be that gaoxiang is perfectly fine in your restrication, but the number 17 certainly looks odd.
> And the previous patches all used this name.
>
>> dmabuf->name can't be accessed without holding the appropriate lock, same of all other cases.
> ok, thanks. It can be accessed with holding dmabuf->name_lock.
That should work, yes.
Regards,
Christian.
>
>
>
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *发件人:* Christian König <christian.koenig@amd.com>
> *发送时间:* 2025年11月24日 22:21:57
> *收件人:* Xiang Gao; sumit.semwal@linaro.org; rostedt@goodmis.org; mhiramat@kernel.org
> *抄送:* linux-media@vger.kernel.org; dri-devel@lists.freedesktop.org; linaro-mm-sig@lists.linaro.org; linux-kernel@vger.kernel.org; mathieu.desnoyers@efficios.com; dhowells@redhat.com; kuba@kernel.org; brauner@kernel.org; akpm@linux-foundation.org; linux-trace-kernel@vger.kernel.org; 高翔
> *主题:* [External Mail]Re: [PATCH] dma-buf: add some tracepoints to debug.
>
> [外部邮件] 此邮件来源于小米公司外部，请谨慎处理。若对邮件安全性存疑，请将邮件转发给misec@xiaomi.com进行反馈
>
> On 11/24/25 14:36, Xiang Gao wrote:
>> From: gaoxiang17 <gaoxiang17@xiaomi.com>
>>
>> With these tracepoints, we can track dmabuf in real time.
>>
>> For example:
>> binder:3025_3-10524 [000] ..... 553.310313: dma_buf_export: exp_name=qcom,system name=(null) size=12771328 ino=2799
>> binder:3025_3-10524 [000] ..... 553.310318: dma_buf_fd: exp_name=qcom,system name=(null) size=12771328 ino=2799 fd=8
>> RenderThread-9307 [000] ..... 553.310869: dma_buf_get: exp_name=qcom,system name=blastBufferQueue for scaleUpDow size=12771328 ino=2799 fd=673 f_ref=4
>> RenderThread-9307 [000] ..... 553.310871: dma_buf_attach: dev_name=kgsl-3d0 exp_name=qcom,system name=blastBufferQueue for scaleUpDow size=12771328 ino=2799
>> RenderThread-9307 [000] ..... 553.310946: dma_buf_mmap_internal: exp_name=qcom,system name=blastBufferQueue for scaleUpDow size=12771328 ino=2799
>> RenderThread-9307 [004] ..... 553.315084: dma_buf_detach: exp_name=qcom,system name=blastBufferQueue for scaleUpDow size=12771328 ino=2799
>> RenderThread-9307 [004] ..... 553.315084: dma_buf_put: exp_name=qcom,system name=blastBufferQueue for scaleUpDow size=12771328 ino=2799 f_ref=5
>
> In general quite nice to have, but this needs a bit more explanation why you need it.
>
>>
>> Signed-off-by: gaoxiang17 <gaoxiang17@xiaomi.com>
>
> I have strongly doubts that this is your legal name :)
>
> Please google the requirements for a Signed-off-by line.
>
>> ---
>> drivers/dma-buf/dma-buf.c | 19 +++
>> include/trace/events/dma_buf.h | 245 +++++++++++++++++++++++++++++++++
>> 2 files changed, 264 insertions(+)
>> 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..8b5af73f0218 100644
>> --- a/drivers/dma-buf/dma-buf.c
>> +++ b/drivers/dma-buf/dma-buf.c
>> @@ -35,6 +35,9 @@
>>
>> #include "dma-buf-sysfs-stats.h"
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/dma_buf.h>
>> +
>> static inline int is_dma_buf_file(struct file *);
>>
>> static DEFINE_MUTEX(dmabuf_list_mutex);
>> @@ -220,6 +223,8 @@ static int dma_buf_mmap_internal(struct file *file, struct vm_area_struct *vma)
>> dmabuf->size >> PAGE_SHIFT)
>> return -EINVAL;
>>
>> + trace_dma_buf_mmap_internal(dmabuf);
>> +
>> return dmabuf->ops->mmap(dmabuf, vma);
>> }
>>
>> @@ -745,6 +750,8 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info)
>>
>> __dma_buf_list_add(dmabuf);
>>
>> + trace_dma_buf_export(dmabuf);
>> +
>> return dmabuf;
>>
>> err_dmabuf:
>> @@ -779,6 +786,8 @@ int dma_buf_fd(struct dma_buf *dmabuf, int flags)
>>
>> fd_install(fd, dmabuf->file);
>>
>> + trace_dma_buf_fd(dmabuf, fd);
>> +
>> return fd;
>> }
>> EXPORT_SYMBOL_NS_GPL(dma_buf_fd, "DMA_BUF");
>> @@ -805,6 +814,8 @@ struct dma_buf *dma_buf_get(int fd)
>> return ERR_PTR(-EINVAL);
>> }
>>
>> + trace_dma_buf_get(fd, file);
>> +
>> return file->private_data;
>> }
>> EXPORT_SYMBOL_NS_GPL(dma_buf_get, "DMA_BUF");
>> @@ -825,6 +836,8 @@ void dma_buf_put(struct dma_buf *dmabuf)
>> return;
>>
>> fput(dmabuf->file);
>> +
>> + trace_dma_buf_put(dmabuf);
>> }
>> EXPORT_SYMBOL_NS_GPL(dma_buf_put, "DMA_BUF");
>>
>> @@ -998,6 +1011,8 @@ EXPORT_SYMBOL_NS_GPL(dma_buf_dynamic_attach, "DMA_BUF");
>> struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf,
>> struct device *dev)
>> {
>> + trace_dma_buf_attach(dmabuf, dev);
>> +
>> return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL);
>> }
>> EXPORT_SYMBOL_NS_GPL(dma_buf_attach, "DMA_BUF");
>> @@ -1024,6 +1039,8 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)
>> dmabuf->ops->detach(dmabuf, attach);
>>
>> kfree(attach);
>> +
>> + trace_dma_buf_detach(dmabuf);
>> }
>> EXPORT_SYMBOL_NS_GPL(dma_buf_detach, "DMA_BUF");
>>
>> @@ -1488,6 +1505,8 @@ int dma_buf_mmap(struct dma_buf *dmabuf, struct vm_area_struct *vma,
>> vma_set_file(vma, dmabuf->file);
>> vma->vm_pgoff = pgoff;
>>
>> + 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..796ae444f6ae
>> --- /dev/null
>> +++ b/include/trace/events/dma_buf.h
>> @@ -0,0 +1,245 @@
>> +/* 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>
>> +
>> +TRACE_EVENT(dma_buf_export,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf),
>> +
>> + TP_ARGS(dmabuf),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>
> dmabuf->name can't be accessed without holding the appropriate lock, same of all other cases.
>
> Regards,
> Christian.
>
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_fd,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf, int fd),
>> +
>> + TP_ARGS(dmabuf, fd),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + __field(int, fd)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + __entry->fd = fd;
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu fd=%d",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino,
>> + __entry->fd)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_mmap_internal,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf),
>> +
>> + TP_ARGS(dmabuf),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_mmap,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf),
>> +
>> + TP_ARGS(dmabuf),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_attach,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf, struct device *dev),
>> +
>> + TP_ARGS(dmabuf, dev),
>> +
>> + TP_STRUCT__entry(
>> + __string(dname, dev_name(dev))
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(dname);
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + ),
>> +
>> + TP_printk("dev_name=%s exp_name=%s name=%s size=%zu ino=%lu",
>> + __get_str(dname),
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_detach,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf),
>> +
>> + TP_ARGS(dmabuf),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_get,
>> +
>> + TP_PROTO(int fd, struct file *file),
>> +
>> + TP_ARGS(fd, file),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, ((struct dma_buf *)file->private_data)->exp_name)
>> + __string(name, ((struct dma_buf *)file->private_data)->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + __field(int, fd)
>> + __field(long, f_ref)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = ((struct dma_buf *)file->private_data)->size;
>> + __entry->ino = ((struct dma_buf *)file->private_data)->file->f_inode->i_ino;
>> + __entry->fd = fd;
>> + __entry->f_ref = file_ref_get(&file->f_ref);
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu fd=%d f_ref=%ld",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino,
>> + __entry->fd,
>> + __entry->f_ref)
>> +);
>> +
>> +TRACE_EVENT(dma_buf_put,
>> +
>> + TP_PROTO(struct dma_buf *dmabuf),
>> +
>> + TP_ARGS(dmabuf),
>> +
>> + TP_STRUCT__entry(
>> + __string(exp_name, dmabuf->exp_name)
>> + __string(name, dmabuf->name)
>> + __field(size_t, size)
>> + __field(ino_t, ino)
>> + __field(long, f_ref)
>> + ),
>> +
>> + TP_fast_assign(
>> + __assign_str(exp_name);
>> + __assign_str(name);
>> + __entry->size = dmabuf->size;
>> + __entry->ino = dmabuf->file->f_inode->i_ino;
>> + __entry->f_ref = file_ref_get(&dmabuf->file->f_ref);
>> + ),
>> +
>> + TP_printk("exp_name=%s name=%s size=%zu ino=%lu f_ref=%ld",
>> + __get_str(exp_name),
>> + __get_str(name),
>> + __entry->size,
>> + __entry->ino,
>> + __entry->f_ref)
>> +);
>> +
>> +#endif /* _TRACE_DMA_BUF_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>
^ permalink raw reply
* Re: [PATCH v7] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Donglin Peng @ 2025-11-25 2:05 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
In-Reply-To: <20251124201258.0e34ac3b@gandalf.local.home>
On Tue, Nov 25, 2025 at 9:12 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 24 Nov 2025 16:54:40 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > On Mon, 24 Nov 2025 16:47:01 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > > --- a/kernel/trace/trace_entries.h
> > > > +++ b/kernel/trace/trace_entries.h
> > > > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > > > TRACE_GRAPH_RETADDR_ENT,
> > > >
> > > > F_STRUCT(
> > > > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > > > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > > > __field_packed( unsigned long, graph_ent, func )
> > > > __field_packed( unsigned int, graph_ent, depth )
> > > > - __field_packed( unsigned long, graph_ent, retaddr )
> > >
> > > You can't delete the retaddr without breaking user space.
> > >
> > > Please keep that here.
> >
> > I added this, and user space works again:
> >
> > diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> > index 593a74663c65..4666b6179056 100644
> > --- a/kernel/trace/trace_entries.h
> > +++ b/kernel/trace/trace_entries.h
> > @@ -98,6 +98,7 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > __field_struct( struct ftrace_graph_ent, graph_ent )
> > __field_packed( unsigned long, graph_ent, func )
> > __field_packed( unsigned int, graph_ent, depth )
> > + __field_packed( unsigned long, graph_ent, retaddr )
> > __dynamic_array(unsigned long, args )
> > ),
> >
>
> Nope, this wasn't enough. I got garbage from this. The following patch
> appears to make it all work though (this is against your patch that was
> forward ported to my for-next branch):
>
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 88cb54d73bdb..ba180d19423e 100644
> --- a/include/linux/ftrace.h
> +++ b/include/linux/ftrace.h
> @@ -1121,12 +1121,10 @@ static inline void ftrace_init(void) { }
>
> /*
> * Structure that defines an entry function trace.
> - * It's already packed but the attribute "packed" is needed
> - * to remove extra padding at the end.
> */
> struct ftrace_graph_ent {
> unsigned long func; /* Current function */
> - int depth;
> + unsigned long depth;
> } __packed;
>
> /*
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 05fb6c9279e3..d509a5041d38 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -60,6 +60,13 @@ enum trace_type {
> __TRACE_LAST_TYPE,
> };
>
> +/*
> + * Structure that defines an entry function trace with retaddr.
> + */
> +struct fgraph_retaddr_ent {
> + struct ftrace_graph_ent ent;
> + unsigned long retaddr; /* Return address */
> +} __packed;
>
> #undef __field
> #define __field(type, item) type item;
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index 593a74663c65..87f4228374cd 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -80,11 +80,11 @@ FTRACE_ENTRY(funcgraph_entry, ftrace_graph_ent_entry,
> F_STRUCT(
> __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> - __field_packed( unsigned int, graph_ent, depth )
> + __field_packed( unsigned long, graph_ent, depth )
> __dynamic_array(unsigned long, args )
> ),
>
> - F_printk("--> %ps (%u)", (void *)__entry->func, __entry->depth)
> + F_printk("--> %ps (%lu)", (void *)__entry->func, __entry->depth)
> );
>
> #ifdef CONFIG_FUNCTION_GRAPH_RETADDR
> @@ -95,13 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> TRACE_GRAPH_RETADDR_ENT,
>
> F_STRUCT(
> - __field_struct( struct ftrace_graph_ent, graph_ent )
> - __field_packed( unsigned long, graph_ent, func )
> - __field_packed( unsigned int, graph_ent, depth )
> + __field_struct( struct fgraph_retaddr_ent, graph_ent )
> + __field_packed( unsigned long, graph_ent.ent, func )
> + __field_packed( unsigned long, graph_ent.ent, depth )
> + __field_packed( unsigned long, graph_ent, retaddr )
> __dynamic_array(unsigned long, args )
> ),
>
> - F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
> + F_printk("--> %ps (%lu) <- %ps", (void *)__entry->func, __entry->depth,
> (void *)__entry->args[0])
Thanks. The args[0] holds the return address in this patch, so if the retaddr
member is added back, we may need to reconstruct this patch to store the
return address in the retaddr member, with the args array storing only function
parameters. I will fix it in the next version.
Thanks
Donglin
> );
>
> -- Steve
^ permalink raw reply
* [PATCH] kprobes: avoid crash when rmmod/insmod modules after ftrace_disabled
From: Ye Bin @ 2025-11-25 2:05 UTC (permalink / raw)
To: naveen, davem, mhiramat, linux-trace-kernel; +Cc: yebin, yebin10
From: Ye Bin <yebin10@huawei.com>
There's a issue as follows when rmmod modules after ftrace disabled:
BUG: unable to handle page fault for address: fffffbfff805000d
PGD 817fcc067 P4D 817fcc067 PUD 817fc8067 PMD 101555067 PTE 0
Oops: Oops: 0000 [#1] SMP KASAN PTI
CPU: 4 UID: 0 PID: 2012 Comm: rmmod Tainted: G W OE
Tainted: [W]=WARN, [O]=OOT_MODULE, [E]=UNSIGNED_MODULE
RIP: 0010:kprobes_module_callback+0x89/0x790
RSP: 0018:ffff88812e157d30 EFLAGS: 00010a02
RAX: 1ffffffff805000d RBX: dffffc0000000000 RCX: ffffffff86a8de90
RDX: ffffed1025c2af9b RSI: 0000000000000008 RDI: ffffffffc0280068
RBP: 0000000000000000 R08: 0000000000000001 R09: ffffed1025c2af9a
R10: ffff88812e157cd7 R11: 205d323130325420 R12: 0000000000000002
R13: ffffffffc0290488 R14: 0000000000000002 R15: ffffffffc0280040
FS: 00007fbc450dd740(0000) GS:ffff888420331000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: fffffbfff805000d CR3: 000000010f624000 CR4: 00000000000006f0
Call Trace:
<TASK>
notifier_call_chain+0xc6/0x280
blocking_notifier_call_chain+0x60/0x90
__do_sys_delete_module.constprop.0+0x32a/0x4e0
do_syscall_64+0x5d/0xfa0
entry_SYSCALL_64_after_hwframe+0x76/0x7e
The above issue occurs because the kprobe was not removed from the hash
list after ftrace_disable.
To prevent the system from restarting unexpectedly after ftrace_disable,
in such cases, unregister_kprobe() ensures that the probe is removed from
the hash list, preventing subsequent access to already freed memory.
Fixes: 6f0f1dd71953 ("kprobes: Cleanup disabling and unregistering path")
Signed-off-by: Ye Bin <yebin10@huawei.com>
---
kernel/kprobes.c | 26 ++++++++++++++++++++++++--
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index ab8f9fc1f0d1..d735a608b810 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -1731,8 +1731,30 @@ static int __unregister_kprobe_top(struct kprobe *p)
/* Disable kprobe. This will disarm it if needed. */
ap = __disable_kprobe(p);
- if (IS_ERR(ap))
- return PTR_ERR(ap);
+ if (IS_ERR(ap)) {
+ int ret = PTR_ERR(ap);
+
+ /*
+ * If ftrace disabled we need to delete kprobe node from
+ * hlist or aggregation list. If nodes are not removed when
+ * modules are removed, the already released nodes will
+ * remain in the linked list. Subsequent access to the
+ * linked list may then trigger exceptions.
+ */
+ if (ret != -ENODEV)
+ return ret;
+
+ ap = __get_valid_kprobe(p);
+ if (!ap)
+ return ret;
+
+ if (ap == p)
+ hlist_del_rcu(&ap->hlist);
+ else
+ list_del_rcu(&p->list);
+
+ return ret;
+ }
WARN_ON(ap != p && !kprobe_aggrprobe(ap));
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v7] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Steven Rostedt @ 2025-11-25 1:12 UTC (permalink / raw)
To: Donglin Peng
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
In-Reply-To: <20251124165440.1418ed85@gandalf.local.home>
On Mon, 24 Nov 2025 16:54:40 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Mon, 24 Nov 2025 16:47:01 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
>
> > > --- a/kernel/trace/trace_entries.h
> > > +++ b/kernel/trace/trace_entries.h
> > > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > > TRACE_GRAPH_RETADDR_ENT,
> > >
> > > F_STRUCT(
> > > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > > __field_packed( unsigned long, graph_ent, func )
> > > __field_packed( unsigned int, graph_ent, depth )
> > > - __field_packed( unsigned long, graph_ent, retaddr )
> >
> > You can't delete the retaddr without breaking user space.
> >
> > Please keep that here.
>
> I added this, and user space works again:
>
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index 593a74663c65..4666b6179056 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -98,6 +98,7 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> __field_struct( struct ftrace_graph_ent, graph_ent )
> __field_packed( unsigned long, graph_ent, func )
> __field_packed( unsigned int, graph_ent, depth )
> + __field_packed( unsigned long, graph_ent, retaddr )
> __dynamic_array(unsigned long, args )
> ),
>
Nope, this wasn't enough. I got garbage from this. The following patch
appears to make it all work though (this is against your patch that was
forward ported to my for-next branch):
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 88cb54d73bdb..ba180d19423e 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -1121,12 +1121,10 @@ static inline void ftrace_init(void) { }
/*
* Structure that defines an entry function trace.
- * It's already packed but the attribute "packed" is needed
- * to remove extra padding at the end.
*/
struct ftrace_graph_ent {
unsigned long func; /* Current function */
- int depth;
+ unsigned long depth;
} __packed;
/*
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index 05fb6c9279e3..d509a5041d38 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -60,6 +60,13 @@ enum trace_type {
__TRACE_LAST_TYPE,
};
+/*
+ * Structure that defines an entry function trace with retaddr.
+ */
+struct fgraph_retaddr_ent {
+ struct ftrace_graph_ent ent;
+ unsigned long retaddr; /* Return address */
+} __packed;
#undef __field
#define __field(type, item) type item;
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index 593a74663c65..87f4228374cd 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -80,11 +80,11 @@ FTRACE_ENTRY(funcgraph_entry, ftrace_graph_ent_entry,
F_STRUCT(
__field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
+ __field_packed( unsigned long, graph_ent, depth )
__dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u)", (void *)__entry->func, __entry->depth)
+ F_printk("--> %ps (%lu)", (void *)__entry->func, __entry->depth)
);
#ifdef CONFIG_FUNCTION_GRAPH_RETADDR
@@ -95,13 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
TRACE_GRAPH_RETADDR_ENT,
F_STRUCT(
- __field_struct( struct ftrace_graph_ent, graph_ent )
- __field_packed( unsigned long, graph_ent, func )
- __field_packed( unsigned int, graph_ent, depth )
+ __field_struct( struct fgraph_retaddr_ent, graph_ent )
+ __field_packed( unsigned long, graph_ent.ent, func )
+ __field_packed( unsigned long, graph_ent.ent, depth )
+ __field_packed( unsigned long, graph_ent, retaddr )
__dynamic_array(unsigned long, args )
),
- F_printk("--> %ps (%u) <- %ps", (void *)__entry->func, __entry->depth,
+ F_printk("--> %ps (%lu) <- %ps", (void *)__entry->func, __entry->depth,
(void *)__entry->args[0])
);
-- Steve
^ permalink raw reply related
* Re: [PATCH v1 0/8] tools/rtla: Consolidate common command line options
From: Crystal Wood @ 2025-11-25 1:01 UTC (permalink / raw)
To: Costa Shulyupin, Steven Rostedt, Tomas Glozar,
Wander Lairson Costa, Ivan Pravdin, linux-trace-kernel,
linux-kernel
In-Reply-To: <20251121174504.519230-1-costa.shul@redhat.com>
On Fri, 2025-11-21 at 19:44 +0200, Costa Shulyupin wrote:
> Argument parsing functions are the longest functions of the project:
>
> lizard --warnings_only --sort nloc -L100 | head -n 4
> ./src/timerlat_hist.c:766: warning: timerlat_hist_parse_args has 247 NLOC, 76 CCN, 1579 token, 1 PARAM, 287 length, 0 ND
> ./src/timerlat_top.c:538: warning: timerlat_top_parse_args has 217 NLOC, 64 CCN, 1338 token, 2 PARAM, 262 length, 0 ND
> ./src/osnoise_hist.c:469: warning: osnoise_hist_parse_args has 195 NLOC, 56 CCN, 1225 token, 1 PARAM, 216 length, 0 ND
> ./src/osnoise_top.c:322: warning: osnoise_top_parse_args has 169 NLOC, 46 CCN, 1019 token, 2 PARAM, 191 length, 0 ND
>
> Moreover, they contain a lot of duplicate code and growing.
>
> Refactor these functions to reduce code duplication.
>
> Benefits:
> - Single implementation for common options
> - Easier maintenance and consistent behavior
>
> Costa Shulyupin (8):
> tools/rtla: Add common_parse_options()
> tools/rtla: Consolidate -c/--cpus option parsing
> tools/rtla: Consolidate -C/--cgroup option parsing
> tools/rtla: Consolidate -D/--debug option parsing
> tools/rtla: Consolidate -d/--duration option parsing
> tools/rtla: Consolidate -e/--event option parsing
> tools/rtla: Consolidate -P/--priority option parsing
> tools/rtla: Consolidate -H/--house-keeping option parsing
>
> tools/tracing/rtla/src/common.c | 78 ++++++++++++++++++++++++++
> tools/tracing/rtla/src/common.h | 1 +
> tools/tracing/rtla/src/osnoise_hist.c | 53 ++---------------
> tools/tracing/rtla/src/osnoise_top.c | 53 ++---------------
> tools/tracing/rtla/src/timerlat_hist.c | 53 ++---------------
> tools/tracing/rtla/src/timerlat_top.c | 52 ++---------------
> 6 files changed, 95 insertions(+), 195 deletions(-)
Reviewed-by: Crystal Wood <crwood@redhat.com>
-Crystal
^ permalink raw reply
* Re: [rtla 07/13] rtla: Introduce timerlat_restart() helper
From: Crystal Wood @ 2025-11-25 0:46 UTC (permalink / raw)
To: Wander Lairson Costa, Steven Rostedt, Tomas Glozar, Ivan Pravdin,
John Kacur, Costa Shulyupin, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_)
In-Reply-To: <20251117184409.42831-8-wander@redhat.com>
On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
> +enum restart_result
> +timerlat_restart(const struct osnoise_tool *tool, struct timerlat_params *params)
> +{
> + actions_perform(¶ms->common.threshold_actions);
> +
> + if (!params->common.threshold_actions.continue_flag)
> + /* continue flag not set, break */
> + return RESTART_STOP;
> +
> + /* continue action reached, re-enable tracing */
> + if (tool->record && trace_instance_start(&tool->record->trace))
> + goto err;
> + if (tool->aa && trace_instance_start(&tool->aa->trace))
> + goto err;
> + return RESTART_OK;
> +
> +err:
> + err_msg("Error restarting trace\n");
> + return RESTART_ERROR;
> +}
The non-BPF functions in common.c have the same logic and should also
call this. This isn't timerlat-specific.
> diff --git a/tools/tracing/rtla/src/timerlat_hist.c b/tools/tracing/rtla/src/timerlat_hist.c
> index 09a3da3f58630..f14fc56c5b4a5 100644
> --- a/tools/tracing/rtla/src/timerlat_hist.c
> +++ b/tools/tracing/rtla/src/timerlat_hist.c
> @@ -1165,18 +1165,19 @@ static int timerlat_hist_bpf_main_loop(struct osnoise_tool *tool)
>
> if (!stop_tracing) {
> /* Threshold overflow, perform actions on threshold */
> - actions_perform(¶ms->common.threshold_actions);
> + enum restart_result result;
>
> - if (!params->common.threshold_actions.continue_flag)
> - /* continue flag not set, break */
> + result = timerlat_restart(tool, params);
> + if (result == RESTART_STOP)
> break;
>
> - /* continue action reached, re-enable tracing */
> - if (tool->record)
> - trace_instance_start(&tool->record->trace);
> - if (tool->aa)
> - trace_instance_start(&tool->aa->trace);
> - timerlat_bpf_restart_tracing();
> + if (result == RESTART_ERROR)
> + return -1;
Does it matter that we're not detaching on an error here? Is this
something that gets cleaned up automatically (and if so, why do we ever
need to do it explicitly)?
> +
> + if (timerlat_bpf_restart_tracing()) {
> + err_msg("Error restarting BPF trace\n");
> + return -1;
> + }
[insert rant about not being able to use exceptions in userspace code in
the year 2025]
-Crystal
^ permalink raw reply
* Re: [rtla 05/13] rtla: Simplify argument parsing
From: Crystal Wood @ 2025-11-25 0:46 UTC (permalink / raw)
To: Wander Lairson Costa, Steven Rostedt, Tomas Glozar, Ivan Pravdin,
John Kacur, Costa Shulyupin, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_)
In-Reply-To: <20251117184409.42831-6-wander@redhat.com>
On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
>
> +/*
> + * extract_arg - extract argument value from option token
> + * @token: option token (e.g., "file=trace.txt")
> + * @opt: option name to match (e.g., "file")
> + *
> + * Returns pointer to argument value after "=" if token matches "opt=",
> + * otherwise returns NULL.
> + */
> +#define extract_arg(token, opt) ( \
> + strlen(token) > STRING_LENGTH(opt "=") && \
> + !strncmp_static(token, opt "=") \
> + ? (token) + STRING_LENGTH(opt "=") : NULL )
This could be implemented as a function (albeit without the
concatenation trick)... but if it must be a macro, at least encase it
with ({ }) so it behaves more like a function.
> +
> /*
> * actions_parse - add an action based on text specification
> */
> int
> actions_parse(struct actions *self, const char *trigger, const char *tracefn)
> {
> +
> enum action_type type = ACTION_NONE;
Why this blank line?
> diff --git a/tools/tracing/rtla/src/utils.h b/tools/tracing/rtla/src/utils.h
> index 160491f5de91c..f7ff548f7fba7 100644
> --- a/tools/tracing/rtla/src/utils.h
> +++ b/tools/tracing/rtla/src/utils.h
> @@ -13,8 +13,18 @@
> #define MAX_NICE 20
> #define MIN_NICE -19
>
> -#define container_of(ptr, type, member)({ \
> - const typeof(((type *)0)->member) *__mptr = (ptr); \
[snip]
> +#define container_of(ptr, type, member)({ \
> + const typeof(((type *)0)->member) *__mptr = (ptr); \
> (type *)((char *)__mptr - offsetof(type, member)) ; })
It's easier to review patches when they don't make unrelated aesthetic
changes...
-Crystal
^ permalink raw reply
* Re: [rtla 04/13] rtla: Replace atoi() with a robust strtoi()
From: Crystal Wood @ 2025-11-25 0:46 UTC (permalink / raw)
To: Wander Lairson Costa, Steven Rostedt, Tomas Glozar, Ivan Pravdin,
John Kacur, Costa Shulyupin, Tiezhu Yang,
open list:Real-time Linux Analysis (RTLA) tools, open list,
open list:BPF [MISC]:Keyword:(?:\b|_)bpf(?:\b|_)
In-Reply-To: <20251117184409.42831-5-wander@redhat.com>
On Mon, 2025-11-17 at 15:41 -0300, Wander Lairson Costa wrote:
>
> diff --git a/tools/tracing/rtla/src/actions.c b/tools/tracing/rtla/src/actions.c
> index efa17290926da..e23d4f1c5a592 100644
> --- a/tools/tracing/rtla/src/actions.c
> +++ b/tools/tracing/rtla/src/actions.c
> @@ -199,12 +199,14 @@ actions_parse(struct actions *self, const char *trigger, const char *tracefn)
> /* Takes two arguments, num (signal) and pid */
> while (token != NULL) {
> if (strlen(token) > 4 && strncmp(token, "num=", 4) == 0) {
> - signal = atoi(token + 4);
> + if(!strtoi(token + 4, &signal))
> + return -1;
if (
> } else if (strlen(token) > 4 && strncmp(token, "pid=", 4) == 0) {
> if (strncmp(token + 4, "parent", 7) == 0)
> pid = -1;
> else
> - pid = atoi(token + 4);
> + if (!strtoi(token + 4, &pid))
> + return -1;
else if (
Please run the patches through checkpatch.pl
> @@ -959,3 +967,25 @@ int auto_house_keeping(cpu_set_t *monitored_cpus)
>
> return 1;
> }
> +
> +/*
> + * strtoi - convert string to integer with error checking
> + *
> + * Returns true on success, false if conversion fails or result is out of int range.
> + */
> +bool strtoi(const char *s, int *res)
Could use __attribute__((__warn_unused_result__)) like kstrtoint().
BTW, it's pretty annoying that we need to reinvent the wheel on all this
stuff just because it's userspace. From some of the other tools it
looks like we can at least include basic kernel headers like compiler.h;
maybe we should have a tools/-wide common util area as well? Even
better if some of the code can be shared with the kernel itself.
Not saying that should in any way be a blocker for these patches, just
something to think about.
-Crystal
^ permalink raw reply
* Re: [PATCH] selftests: tracing: Add tprobe enable/disable testcase
From: Shuah Khan @ 2025-11-25 0:34 UTC (permalink / raw)
To: Steven Rostedt
Cc: Masami Hiramatsu (Google), Shuah Khan, Mathieu Desnoyers,
linux-kernel, linux-trace-kernel, linux-kselftest, Shuah Khan
In-Reply-To: <20251124153930.336ef162@gandalf.local.home>
On 11/24/25 13:39, Steven Rostedt wrote:
> On Thu, 20 Nov 2025 09:40:28 -0700
> Shuah Khan <skhan@linuxfoundation.org> wrote:
>
>>> Thanks Shuah! This and other regression fixes is better to go
>>> through selftests tree because those are checking existing
>>> features. Maybe better to add [PATCH -selftests] or something
>>> like that?
>>>
>>
>> Let me know which ones you would like to pick up and apply to my tree.
>
> This one and I believe this one:
>
> https://lore.kernel.org/linux-trace-kernel/176295318112.431538.11780280333728368327.stgit@devnote2
>
Both these patches are in linux-kselftest next.
thanks,
-- Shuah
^ permalink raw reply
* Re: [PATCH v7] function_graph: Enable funcgraph-args and funcgraph-retaddr to work simultaneously
From: Steven Rostedt @ 2025-11-24 21:54 UTC (permalink / raw)
To: Donglin Peng
Cc: linux-trace-kernel, linux-kernel, Donglin Peng, Sven Schnelle,
Masami Hiramatsu
In-Reply-To: <20251124164701.1216a073@gandalf.local.home>
On Mon, 24 Nov 2025 16:47:01 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:
> > --- a/kernel/trace/trace_entries.h
> > +++ b/kernel/trace/trace_entries.h
> > @@ -95,14 +95,14 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
> > TRACE_GRAPH_RETADDR_ENT,
> >
> > F_STRUCT(
> > - __field_struct( struct fgraph_retaddr_ent, graph_ent )
> > + __field_struct( struct ftrace_graph_ent, graph_ent )
> > __field_packed( unsigned long, graph_ent, func )
> > __field_packed( unsigned int, graph_ent, depth )
> > - __field_packed( unsigned long, graph_ent, retaddr )
>
> You can't delete the retaddr without breaking user space.
>
> Please keep that here.
I added this, and user space works again:
diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
index 593a74663c65..4666b6179056 100644
--- a/kernel/trace/trace_entries.h
+++ b/kernel/trace/trace_entries.h
@@ -98,6 +98,7 @@ FTRACE_ENTRY_PACKED(fgraph_retaddr_entry, fgraph_retaddr_ent_entry,
__field_struct( struct ftrace_graph_ent, graph_ent )
__field_packed( unsigned long, graph_ent, func )
__field_packed( unsigned int, graph_ent, depth )
+ __field_packed( unsigned long, graph_ent, retaddr )
__dynamic_array(unsigned long, args )
),
-- Steve
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox