Messages in this thread | | | Subject | Re: [PATCH] tracing: Do no reference char * as a string in histograms | From | Tom Zanussi <> | Date | Thu, 15 Jul 2021 15:39:33 -0500 |
| |
Hi Steve,
On Thu, 2021-07-15 at 15:54 -0400, Steven Rostedt wrote: > On Thu, 15 Jul 2021 12:04:13 -0500 > Tom Zanussi <zanussi@kernel.org> wrote: > > > Not sure what the correct fix is at this point, but it would seem > > that > > if the original intent was honored, the histogram code should still > > be > > allowed to use them. > > Note, it can in a patch I plan on sending in the next merge window > that > Tzvetomir and I are working on. We succeeded in getting the "eprobe" > working which allows you to do much more with event fields than the > current histogram code does. > > Otherwise, I need a way to get the pointer and not a string. As I > discovered, the string may not have any content in it, and you can't > fault within a tracepoint, and you are left with just passing in > "(fault)", or something. Which is not useful at all! > > What we have as a working prototype is this: > > # cd /sys/kernel/tracing > # echo 'e:myopen syscalls.sys_enter_openat file=$filename:ustring' > > kprobe_events > # trace-cmd start -e myopen -F ~/bin/openat > # cat trace > openat-1928 [000] ...2 1540.638692: myopen: (0) > file="/etc/ld.so.cache" > openat-1928 [000] ...2 1540.638692: myopen: (0) > file="/lib64/libc.so.6" > openat-1928 [000] ...2 1540.638694: myopen: (0) > file=(fault) > > > And if we were to enforce this on a histogram, all we will see is > "fault" because the filename is first referenced inside the system > call > and at the beginning of the call, it is not mapped in, and we can not > map it in inside a tracepoint. You are left with just putting in some > stub string (which kprobes uses "(fault)"). > > But, because now filename is a pointer, that the histograms can pass > around, it is much more useful, as I can use this to pass the > filename > pointer to the exit of the system call! > > # echo 'hist:keys=common_pid:f=filename' > > events/syscalls/sys_enter_openat/trigger > # echo 'openret unsigned long file; long ret' > synthetic_events > # echo > 'hist:keys=common_pid:file=$f:onmatch(syscalls.sys_enter_openat).trac > e(openret,$file,ret)' > events/syscalls/sys_exit_openat/trigger > # echo 'e:myopen synthetic.openret file=$file:ustring ret=$ret:s64' > > kprobe_events > # trace-cmd start -e myopen -F ~/bin/openat > # cat trace > openat-1994 [002] ...3 2052.403323: myopen: (0) > file="/etc/ld.so.cache" ret=3 > openat-1994 [002] ...3 2052.403350: myopen: (0) > file="/lib64/libc.so.6" ret=3 > openat-1994 [002] ...3 2052.403555: myopen: (0) > file="/etc/passwd" ret=3 > > Now, not only can we see the filename, because we passed the pointer > from the sys_enter to the sys_exit, we also can show the return of > the > system call. >
Very nice! Really looking forward to playing around with this.
> This allows for a much more flexibility than the histograms by > themselves have. If you want the file names via pointers, you can > then > create these event_probes and use them in the histograms as well. > Which > gives you the full flexibility of kprobes but on the fields of > events. > > Note, without this patch, because the histograms thinks 'char *' is a > string, you can't pass the pointer, but only the string. Hence, you > only get "(fault)", which is pretty darn useless. > > This is another rationale for why I want this patch. >
Yeah, if it's needed to enable the eprobes and since it also fixes a crashing bug, I'm all for it.
But should it also have an additional 'Fixes:' line?:
Fixes: 5967bd5c4239 ("tracing: Let filter_assign_type() detect FILTER_PTR_STRING")
Acked-by: Tom Zanussi <zanussi@kernel.org>
| |