Re: [PATCH 1/5] tracing: probe: Allocate traceprobe_parse_context from heap

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 18 Jul 2025 12:58:20 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Fri, 18 Jul 2025 20:34:08 +0900
> "Masami Hiramatsu (Google)" <mhiramat@kernel.org> wrote:
> 
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index 854e5668f5ee..7bc4c84464e4 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -10,6 +10,7 @@
> >   * Author:     Srikar Dronamraju
> >   */
> >  
> > +#include <linux/cleanup.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/slab.h>
> >  #include <linux/smp.h>
> 
> Nit, but let's keep the "upside-down x-mas tree" format:
> 
> #include <linux/seq_file.h>
> #include <linux/cleanup.h>
> #include <linux/slab.h>
> #include <linux/smp.h>

Isn't it for variable rules?
I saw some examples of sorting headers by A-Z.

> 
> 
> > @@ -438,6 +439,14 @@ extern void traceprobe_free_probe_arg(struct probe_arg *arg);
> >   * this MUST be called for clean up the context and return a resource.
> >   */
> >  void traceprobe_finish_parse(struct traceprobe_parse_context *ctx);
> > +static inline void traceprobe_free_parse_ctx(struct traceprobe_parse_context *ctx)
> > +{
> > +	traceprobe_finish_parse(ctx);
> > +	kfree(ctx);
> > +}
> > +
> > +DEFINE_FREE(traceprobe_parse_context, struct traceprobe_parse_context *,
> > +		if (!IS_ERR_OR_NULL(_T)) traceprobe_free_parse_ctx(_T))
> 
> ctx will either be allocated or NULL, I think the above could be:
> 
> 		if (_T) traceprobe_free_parse_ctx(_T))

OK.

> 
> 
> >  
> >  extern int traceprobe_split_symbol_offset(char *symbol, long *offset);
> >  int traceprobe_parse_event_name(const char **pevent, const char **pgroup,
> > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
> > index f95a2c3d5b1b..1fd479718d03 100644
> > --- a/kernel/trace/trace_uprobe.c
> > +++ b/kernel/trace/trace_uprobe.c
> > @@ -537,6 +537,7 @@ static int register_trace_uprobe(struct trace_uprobe *tu)
> >   */
> >  static int __trace_uprobe_create(int argc, const char **argv)
> >  {
> > +	struct traceprobe_parse_context *ctx __free(traceprobe_parse_context) = NULL;
> >  	struct trace_uprobe *tu;
> >  	const char *event = NULL, *group = UPROBE_EVENT_SYSTEM;
> >  	char *arg, *filename, *rctr, *rctr_end, *tmp;
> > @@ -693,15 +694,17 @@ static int __trace_uprobe_create(int argc, const char **argv)
> >  	tu->path = path;
> >  	tu->filename = filename;
> >  
> > +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > +	if (!ctx) {
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +	ctx->flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER;
> > +
> >  	/* parse arguments */
> >  	for (i = 0; i < argc; i++) {
> > -		struct traceprobe_parse_context ctx = {
> > -			.flags = (is_return ? TPARG_FL_RETURN : 0) | TPARG_FL_USER,
> > -		};
> > -
> >  		trace_probe_log_set_index(i + 2);
> > -		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], &ctx);
> > -		traceprobe_finish_parse(&ctx);
> > +		ret = traceprobe_parse_probe_arg(&tu->tp, i, argv[i], ctx);
> 
> Doesn't this change the semantics a bit?

Yes, and we don't need to allocate ctx each time because probe target
point is always same (not different for each field). In this case,
we don't need to allocate/free each time.

> 
> Before this change, traceprobe_finish_parse(&ctx) is called for every
> iteration of the loop. Now we only do it when it exits the function.

Yes, but that is not a good way to use the ctx. As same as kprobe and
fprobe events, it is designed to be the same through parsing one probe,
not each field.

For the uprobe case, this is just passing ctx->flags, others are mostly
unused or temporarily used in field parsing. So allocating from stack
frame, it is OK. But allocating from heap, it involves slab allocation
and free each time. I think it is just inefficient. 

Hmm, but eprobe seems doing the same mistake. Let me split that part
to fix to keep using the same ctx through parsing one probe.


Thank you,

> 
> -- Steve
> 
> 
> >  		if (ret)
> >  			goto error;
> >  	}
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>