linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tom Zanussi <zanussi@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Namhyung Kim <namhyung@kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Tzvetomir Stoyanov <tz.stoyanov@gmail.com>
Subject: Re: [PATCH] tracing: Do no reference char * as a string in histograms
Date: Thu, 15 Jul 2021 15:39:33 -0500	[thread overview]
Message-ID: <ff91bab7a874ab134033d840100edd855e9c304e.camel@kernel.org> (raw)
In-Reply-To: <20210715155400.28f0dfd0@oasis.local.home>

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>



  reply	other threads:[~2021-07-15 20:39 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-15  4:02 Steven Rostedt
2021-07-15 16:42 ` Namhyung Kim
2021-07-15 17:04 ` Tom Zanussi
2021-07-15 19:54   ` Steven Rostedt
2021-07-15 20:39     ` Tom Zanussi [this message]
2021-07-15 21:09       ` Steven Rostedt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ff91bab7a874ab134033d840100edd855e9c304e.camel@kernel.org \
    --to=zanussi@kernel.org \
    --cc=akpm@linux-foundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhiramat@kernel.org \
    --cc=mingo@kernel.org \
    --cc=namhyung@kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=tz.stoyanov@gmail.com \
    --subject='Re: [PATCH] tracing: Do no reference char * as a string in histograms' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox