From: Namhyung Kim <namhyung@kernel.org>
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Omar Sandoval <osandov@osandov.com>,
Steven Rostedt <rostedt@goodmis.org>,
Ingo Molnar <mingo@redhat.com>,
linux-kernel@vger.kernel.org, kernel-team@fb.com,
Omar Sandoval <osandov@fb.com>
Subject: Re: [PATCH v2 2/3] tracing: expose current->comm to [ku]probe events
Date: Tue, 7 Jun 2016 22:40:27 +0900 [thread overview]
Message-ID: <20160607134027.GA24922@danjae.aot.lge.com> (raw)
In-Reply-To: <20160607162423.415a5ee3dd504153e611b3ae@kernel.org>
Hello,
On Tue, Jun 07, 2016 at 04:24:23PM +0900, Masami Hiramatsu wrote:
> On Thu, 2 Jun 2016 18:11:02 -0700
> Omar Sandoval <osandov@osandov.com> wrote:
>
> > From: Omar Sandoval <osandov@fb.com>
> >
> > ftrace is very quick to give up on saving the task command line (see
> > `trace_save_cmdline()`). The workaround for events which really care
> > about the command line is to explicitly assign it as part of the entry.
> > However, this doesn't work for kprobe events, as there's no
> > straightforward way to get access to current->comm. Add a kprobe/uprobe
> > event variable $comm which provides exactly that.
> >
Same opinion with Masami; The string_size itself seems not useful IMHO.
Also you can increase 'saved_cmdlines_size' in the tracefs.
Thanks,
Namhyung
> > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > ---
> > Documentation/trace/kprobetrace.txt | 4 ++++
> > Documentation/trace/uprobetracer.txt | 4 ++++
> > kernel/trace/trace_kprobe.c | 1 +
> > kernel/trace/trace_probe.c | 27 +++++++++++++++++++++++++++
> > kernel/trace/trace_probe.h | 10 ++++++++++
> > 5 files changed, 46 insertions(+)
> >
> > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt
> > index 4b3f5293b7bf..0bf746656d06 100644
> > --- a/Documentation/trace/kprobetrace.txt
> > +++ b/Documentation/trace/kprobetrace.txt
> > @@ -40,6 +40,7 @@ Synopsis of kprobe_events
> > $stackN : Fetch Nth entry of stack (N >= 0)
> > $stack : Fetch stack address.
> > $retval : Fetch return value.(*)
> > + $comm : Fetch current task comm.(***)
> > +|-offs(FETCHARG) : Fetch memory at FETCHARG +|- offs address.(**)
> > NAME=FETCHARG : Set NAME as the argument name of FETCHARG.
> > FETCHARG:TYPE : Set TYPE as the type of FETCHARG. Currently, basic types
> > @@ -48,6 +49,7 @@ Synopsis of kprobe_events
> >
> > (*) only for return probe.
> > (**) this is useful for fetching a field of data structures.
> > + (***) you probably want this as a string, i.e., $comm:string
> >
> > Types
> > -----
> > @@ -63,6 +65,8 @@ offset, and container-size (usually 32). The syntax is;
> >
> > b<bit-width>@<bit-offset>/<container-size>
> >
> > +For $comm, the type must be either "string" or "string_size".
>
> Please remove string_size, which is an internal type.
>
> > +
> >
> > Per-Probe Event Filtering
> > -------------------------
> > diff --git a/Documentation/trace/uprobetracer.txt b/Documentation/trace/uprobetracer.txt
> > index 7e0480263c2f..34754da46860 100644
> > --- a/Documentation/trace/uprobetracer.txt
> > +++ b/Documentation/trace/uprobetracer.txt
> > @@ -36,6 +36,7 @@ Synopsis of uprobe_tracer
> > $stackN : Fetch Nth entry of stack (N >= 0)
> > $stack : Fetch stack address.
> > $retval : Fetch return value.(*)
> > + $comm : Fetch current task comm.(***)
> > +|-offs(FETCHARG) : Fetch memory at FETCHARG +|- offs address.(**)
> > NAME=FETCHARG : Set NAME as the argument name of FETCHARG.
> > FETCHARG:TYPE : Set TYPE as the type of FETCHARG. Currently, basic types
> > @@ -44,6 +45,7 @@ Synopsis of uprobe_tracer
> >
> > (*) only for return probe.
> > (**) this is useful for fetching a field of data structures.
> > + (***) you probably want this as a string, i.e., $comm:string
>
> Ditto.
>
> Other part looks OK for me :)
>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>
>
> >
> > Types
> > -----
> > @@ -58,6 +60,8 @@ offset, and container-size (usually 32). The syntax is;
> >
> > b<bit-width>@<bit-offset>/<container-size>
> >
> > +For $comm, the type must be either "string" or "string_size".
> > +
> >
> > Event Profiling
> > ---------------
> > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> > index 5546eec0505f..9aedb0b06683 100644
> > --- a/kernel/trace/trace_kprobe.c
> > +++ b/kernel/trace/trace_kprobe.c
> > @@ -587,6 +587,7 @@ static int create_trace_kprobe(int argc, char **argv)
> > * $retval : fetch return value
> > * $stack : fetch stack address
> > * $stackN : fetch Nth of stack (N:0-)
> > + * $comm : fetch current task comm
> > * @ADDR : fetch memory at ADDR (ADDR should be in kernel)
> > * @SYM[+|-offs] : fetch memory at SYM +|- offs (SYM is a data symbol)
> > * %REG : fetch register REG
> > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> > index 1d372fa6fefb..3900b6e4a05d 100644
> > --- a/kernel/trace/trace_probe.c
> > +++ b/kernel/trace/trace_probe.c
> > @@ -218,6 +218,28 @@ free_bitfield_fetch_param(struct bitfield_fetch_param *data)
> > kfree(data);
> > }
> >
> > +void FETCH_FUNC_NAME(comm, string)(struct pt_regs *regs,
> > + void *data, void *dest)
> > +{
> > + int maxlen = get_rloc_len(*(u32 *)dest);
> > + u8 *dst = get_rloc_data(dest);
> > + long ret;
> > +
> > + if (!maxlen)
> > + return;
> > +
> > + ret = strlcpy(dst, current->comm, maxlen);
> > + *(u32 *)dest = make_data_rloc(ret, get_rloc_offs(*(u32 *)dest));
> > +}
> > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, string));
> > +
> > +void FETCH_FUNC_NAME(comm, string_size)(struct pt_regs *regs,
> > + void *data, void *dest)
> > +{
> > + *(u32 *)dest = strlen(current->comm) + 1;
> > +}
> > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, string_size));
> > +
> > static const struct fetch_type *find_fetch_type(const char *type,
> > const struct fetch_type *ftbl)
> > {
> > @@ -348,6 +370,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t,
> > }
> > } else
> > ret = -EINVAL;
> > + } else if (strcmp(arg, "comm") == 0) {
> > + if (strcmp(t->name, "string") != 0 &&
> > + strcmp(t->name, "string_size") != 0)
> > + return -EINVAL;
> > + f->fn = t->fetch[FETCH_MTD_comm];
> > } else
> > ret = -EINVAL;
> >
> > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h
> > index f6398db09114..45400ca5ded1 100644
> > --- a/kernel/trace/trace_probe.h
> > +++ b/kernel/trace/trace_probe.h
> > @@ -102,6 +102,7 @@ enum {
> > FETCH_MTD_reg = 0,
> > FETCH_MTD_stack,
> > FETCH_MTD_retval,
> > + FETCH_MTD_comm,
> > FETCH_MTD_memory,
> > FETCH_MTD_symbol,
> > FETCH_MTD_deref,
> > @@ -183,6 +184,14 @@ DECLARE_BASIC_FETCH_FUNCS(bitfield);
> > #define fetch_bitfield_string NULL
> > #define fetch_bitfield_string_size NULL
> >
> > +/* comm only makes sense as a string */
> > +#define fetch_comm_u8 NULL
> > +#define fetch_comm_u16 NULL
> > +#define fetch_comm_u32 NULL
> > +#define fetch_comm_u64 NULL
> > +DECLARE_FETCH_FUNC(comm, string);
> > +DECLARE_FETCH_FUNC(comm, string_size);
> > +
> > /*
> > * Define macro for basic types - we don't need to define s* types, because
> > * we have to care only about bitwidth at recording time.
> > @@ -213,6 +222,7 @@ DEFINE_FETCH_##method(u64)
> > ASSIGN_FETCH_FUNC(reg, ftype), \
> > ASSIGN_FETCH_FUNC(stack, ftype), \
> > ASSIGN_FETCH_FUNC(retval, ftype), \
> > +ASSIGN_FETCH_FUNC(comm, ftype), \
> > ASSIGN_FETCH_FUNC(memory, ftype), \
> > ASSIGN_FETCH_FUNC(symbol, ftype), \
> > ASSIGN_FETCH_FUNC(deref, ftype), \
> > --
> > 2.8.3
> >
>
>
> --
> Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2016-06-07 13:41 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-06-03 1:11 [PATCH v2 0/3] tracing: expose current->comm to [ku]probe events Omar Sandoval
2016-06-03 1:11 ` [PATCH v2 1/3] tracing: document "string_size" [ku]probe type Omar Sandoval
2016-06-07 6:29 ` Masami Hiramatsu
2016-06-03 1:11 ` [PATCH v2 2/3] tracing: expose current->comm to [ku]probe events Omar Sandoval
2016-06-07 7:24 ` Masami Hiramatsu
2016-06-07 13:40 ` Namhyung Kim [this message]
2016-06-03 1:11 ` [PATCH v2 3/3] tracing: make "string" the default type for [ku]probe event $comm Omar Sandoval
2016-06-07 8:02 ` Masami Hiramatsu
2016-06-07 13:41 ` Namhyung Kim
2016-06-08 13:43 ` 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=20160607134027.GA24922@danjae.aot.lge.com \
--to=namhyung@kernel.org \
--cc=kernel-team@fb.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mingo@redhat.com \
--cc=osandov@fb.com \
--cc=osandov@osandov.com \
--cc=rostedt@goodmis.org \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).