On Tue, Sep 02, 2025 at 11:21:47AM +0900, Masami Hiramatsu wrote: Hi Masami,
Thank you for your comments. > Hi Ryan, > > Thanks for update. > > On Fri, 29 Aug 2025 19:20:50 +0900 > Ryan Chung <[email protected]> wrote: > > > This v2 addresses the TODO in trace_fprobe to handle comma-separated > > symbol lists and the '!' prefix. Tokens starting with '!' are collected > > as "nofilter", and the others as "filter", then passed to > > register_fprobe() accordingly. Empty tokens are rejected and errors are > > reported with trace_probe_log_err(). > > OK, can you describe how it changes the syntax. You may find more things > to write it down. > > For example, fprobe is not only a function entry, but also it supports > function return. How it is specified? Current "%return" suffix is introduced > for single symbol (function), like schedule%return. If we introduce a list > of symbols and filters, it looks more complicated. > I see your concern and where my confusion came from. > For example, "!funcAB,funcC,funcA*%return" seems like the exit of funcA*, > the entry of funcC, but not covers funcAB. It is naturally misleading > users. We have to check "funcA*%return,!funcAB,funcC" pattern. > > Thus, I think we should use another suffix, like ":exit" (I think the colon > does strongly separate than comma, what do you think?), or just > prohibit to use "%return" but user needs to specify "$retval" in fetcharg > to specify it is the fprobe on function exit. (this maybe more natural) > I agree with you here. Using another suffix will make it more clearer for the user. So in that sense, I am guessing you are suggesting: :exit (return) :entry (explicit entry) So this applies to the entire list. If a comma or wildcard is present and %return appears, the parser will reject it with -EINVAL and a precise trace_probe_log() index. For a single symbol, we use the legacy foo%return. Ex) funcA*,!funcAB,funcC # entry (default) funcA*,!funcAB,funcC:exit # exit (spec-level) schedule%return # single-symbol legacy > The reason why I talked about how to specify the exit point of a function > so far, is because the variables that can be accessed at the entrance > and exit are different. > Ah I see. > Also, fprobe supports event-name autogeneration from the symbol name, > e.g. 'symbol__entry' or 'symbol__exit'. Recently I found the symbol > already supports wildcards, so sanitize it from the event name [1] > > [1] > https://lore.kernel.org/all/175535345114.282990.12294108192847938710.stgit@devnote2/ > > To use this list-style filters, we may need to reject if there is no > event name. Of cause we can generate event-name from the symbol list > but you need to sanitize non alphabet-number characters. > > Ah, here is another one, symbol is used for ctx->funcname, and this is > used for querying BTF. But obviously BTF context is unusable for this > case. So we should set the ctx->funcname = NULL with listed filter. > So it seems like this TODO is actually a bit larger scope than the patch anticipated. In that sense, maybe we could: - for Single, literaly symbol, keep autogen symbol__entry/symbol__exit and sanitize wildcards. - for List or wildcard, require an explicit [GROUP/]EVENT; reject if ommitted. No autogen. I don't completely understand ctx->funcname you mentioned for the usecase for querying BTF. I will research it more. > > > > Questions for maintainers (to confirm my understanding): > > * Parsing location: Masami suggested doing the parsing in the parse > > stage (e.g., via parse_symbol_and_return()). v2 keeps the logic in > > __register_trace_fprobe(), but I can move the call into the parsing > > path in v3 if that is the preferred place. Is that correct? > > Most of above processes have been done in parse_symbol_and_return(), > thus the parsing it should be done around there. > Thank you. > > * Documentation: I plan to update the user-facing docs for fprobe > > syntax. Is Documentation/trace/ the right place (e.g., > > Documentation/trace/fprobetrace.rst)? > > Yes, please explain it with examples. > > Also, can you add a testcase (in a sparate patch) for this syntax?) > Yes. I will add selftests under tools/testing/selftests/ftrace/: Accept when list entry/exit; ! exclusions; whitespace variants. Reject when empty tokens, leading/trailing commas, %return with lists or wildcards, mixed forms. Naming: autogen only for single literal; list/wildcard requires explicit event name. BTF: ctx->fullname == NULL when multi/wildcard. I will add the following: # entry across a list echo 'f:net/tx_entry tcp_sendmsg*,!tcp_sendmsg_locked,tcp_sendpage' \ > /sys/kernel/tracing/dynamic_events # exit across the same list (spec-level) echo 'f:net/tx_exit tcp_sendmsg*,!tcp_sendmsg_locked,tcp_sendpage:exit' \ > /sys/kernel/tracing/dynamic_events echo 1 > /sys/kernel/tracing/events/net/tx_entry/enable echo 1 > /sys/kernel/tracing/events/net/tx_exit/enable > Thank you, > > > > > Link: > > https://lore.kernel.org/linux-trace-kernel/[email protected]/ > > Signed-off-by: Ryan Chung <[email protected]> > > > --- > > > > Changes in v2: > > * Classify '!' tokens as nofilter, others as filter; pass both to > > register_fprobe(). > > * Reject empty tokens; log errors with trace_probe_log_*(). > > * Use __free(kfree) for temporary buffers. > > * Keep subject and style per "Submitting patches" (tabs, wrapping). > > * No manual dedup (leave to ftrace). > > > > kernel/trace/trace_fprobe.c | 48 +++++++++++++++++++++++++++++++++++-- > > 1 file changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/kernel/trace/trace_fprobe.c b/kernel/trace/trace_fprobe.c > > index b36ade43d4b3..d731d9754a39 100644 > > --- a/kernel/trace/trace_fprobe.c > > +++ b/kernel/trace/trace_fprobe.c > > @@ -815,6 +815,11 @@ static int trace_fprobe_verify_target(struct > > trace_fprobe *tf) > > static int __register_trace_fprobe(struct trace_fprobe *tf) > > This is not a good place to add anymore, because this change turned out > not to meet the expected prerequisites. (when I commented TODO here, > I didn't expected too.) > > Anyway, this is a good opportunity to review this TODO deeper. > > Thank you, > I see. My question is whether or not all the symbol and filter should be done in a separate location or possibly separate function (i.e., parse_symbol_and_return()). Unless you prefer dropping %return entirely now, I’ll keep it for single-symbol compatibility and mark it legacy in Documentation/trace/fprobetrace.rst. I’ll send v3 with the parser move, the spec-level suffix, explicit-name rule for list/wildcard, BTF guard, docs, and selftests. > > { > > int i, ret; > > + const char *p, *q; > > + size_t spec_len, flen = 0, nflen = 0, tlen; > > + bool have_f = false, have_nf = false; > > + char *filter __free(kfree) = NULL; > > + char *nofilter __free(kfree) = NULL; > > > > /* Should we need new LOCKDOWN flag for fprobe? */ > > ret = security_locked_down(LOCKDOWN_KPROBES); > > @@ -835,8 +840,47 @@ static int __register_trace_fprobe(struct trace_fprobe > > *tf) > > if (trace_fprobe_is_tracepoint(tf)) > > return __regsiter_tracepoint_fprobe(tf); > > > > - /* TODO: handle filter, nofilter or symbol list */ > > - return register_fprobe(&tf->fp, tf->symbol, NULL); > > + spec_len = strlen(tf->symbol); > > + filter = kzalloc(spec_len + 1, GFP_KERNEL); > > + nofilter = kzalloc(spec_len + 1, GFP_KERNEL); > > + if (!filter || !nofilter) > > + return -ENOMEM; > > + > > + p = tf->symbol; > > + for (p = tf->symbol; p; p = q ? q + 1 : NULL) { > > + q = strchr(p, ','); > > + tlen = q ? (size_t)(q-p) : strlen(p); > > + > > + /* reject empty token */ > > + if (!tlen) { > > + trace_probe_log_set_index(1); > > + trace_probe_log_err(0, BAD_TP_NAME); > > + return -EINVAL; > > + } > > + > > + if (*p == '!') { > > + if (tlen == 1) { > > + trace_probe_log_set_index(1); > > + trace_probe_log_err(0, BAD_TP_NAME); > > + return -EINVAL; > > + } > > + if (have_nf) > > + nofilter[nflen++] = ','; > > + memcpy(nofilter + nflen, p + 1, tlen - 1); > > + nflen += tlen - 1; > > + have_nf = true; > > + } else { > > + if (have_f) > > + filter[flen++] = ','; > > + memcpy(filter + flen, p, tlen); > > + flen += tlen; > > + have_f = true; > > + } > > + } > > + > > + return register_fprobe(&tf->fp, > > + have_f ? filter : NULL, > > + have_nf ? nofilter : NULL); > > } > > > > /* Internal unregister function - just handle fprobe and flags */ > > -- > > 2.43.0 > > > > > -- > Masami Hiramatsu (Google) <[email protected]> Best regards, Ryan Chung
