* [PATCH] tracing: expose current->comm to kprobe events @ 2016-06-01 6:17 Omar Sandoval 2016-06-02 15:40 ` Namhyung Kim 0 siblings, 1 reply; 5+ messages in thread From: Omar Sandoval @ 2016-06-01 6:17 UTC (permalink / raw) To: Steven Rostedt, Ingo Molnar, Masami Hiramatsu Cc: linux-kernel, kernel-team, Omar Sandoval 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 event variable $comm which provides exactly that. Signed-off-by: Omar Sandoval <osandov@fb.com> --- This is a problem I ran into while trying to debug an unrelated issue. I was considering trying to make ftrace try harder to save the command line, but that would most likely be at the expense at most events which don't care about the task comm. This is much less work. Documentation/trace/kprobetrace.txt | 2 ++ kernel/trace/trace_kprobe.c | 13 +++++++++++++ kernel/trace/trace_probe.c | 5 +++++ kernel/trace/trace_probe.h | 2 ++ kernel/trace/trace_uprobe.c | 7 +++++++ 5 files changed, 29 insertions(+) diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt index d68ea5fc812b..90914cbc1e51 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 ----- diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 5546eec0505f..e3a087552d49 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -214,6 +214,18 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, } NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string_size)); +#define DEFINE_FETCH_comm(type) \ +void FETCH_FUNC_NAME(comm, type)(struct pt_regs *regs, \ + void *data, void *dest) \ +{ \ + fetch_memory_##type(regs, current->comm, dest); \ +} \ +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, type)); + +DEFINE_BASIC_FETCH_FUNCS(comm) +DEFINE_FETCH_comm(string) +DEFINE_FETCH_comm(string_size) + #define DEFINE_FETCH_symbol(type) \ void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs, void *data, void *dest)\ { \ @@ -587,6 +599,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..918775fcacd3 100644 --- a/kernel/trace/trace_probe.c +++ b/kernel/trace/trace_probe.c @@ -348,6 +348,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, } } else ret = -EINVAL; + } else if (strcmp(arg, "comm") == 0) { + if (is_kprobe) + f->fn = t->fetch[FETCH_MTD_comm]; + else + ret = -EINVAL; } else ret = -EINVAL; diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h index f6398db09114..3f1a987e6dc1 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, @@ -213,6 +214,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), \ diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c index c53485441c88..f7eedb191d1f 100644 --- a/kernel/trace/trace_uprobe.c +++ b/kernel/trace/trace_uprobe.c @@ -172,6 +172,13 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, *(u32 *)dest = len; } +#define fetch_comm_u8 NULL +#define fetch_comm_u16 NULL +#define fetch_comm_u32 NULL +#define fetch_comm_u64 NULL +#define fetch_comm_string NULL +#define fetch_comm_string_size NULL + static unsigned long translate_user_vaddr(void *file_offset) { unsigned long base_addr; -- 2.8.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: expose current->comm to kprobe events 2016-06-01 6:17 [PATCH] tracing: expose current->comm to kprobe events Omar Sandoval @ 2016-06-02 15:40 ` Namhyung Kim 2016-06-02 18:21 ` Omar Sandoval 2016-06-03 8:59 ` Masami Hiramatsu 0 siblings, 2 replies; 5+ messages in thread From: Namhyung Kim @ 2016-06-02 15:40 UTC (permalink / raw) To: Omar Sandoval Cc: Steven Rostedt, Ingo Molnar, Masami Hiramatsu, LKML, kernel-team, Omar Sandoval Hello, On Wed, Jun 1, 2016 at 3:17 PM, 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 event > variable $comm which provides exactly that. > > Signed-off-by: Omar Sandoval <osandov@fb.com> > --- > This is a problem I ran into while trying to debug an unrelated issue. I > was considering trying to make ftrace try harder to save the command > line, but that would most likely be at the expense at most events which > don't care about the task comm. This is much less work. > > Documentation/trace/kprobetrace.txt | 2 ++ > kernel/trace/trace_kprobe.c | 13 +++++++++++++ > kernel/trace/trace_probe.c | 5 +++++ > kernel/trace/trace_probe.h | 2 ++ > kernel/trace/trace_uprobe.c | 7 +++++++ > 5 files changed, 29 insertions(+) > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > index d68ea5fc812b..90914cbc1e51 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 > ----- > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5546eec0505f..e3a087552d49 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -214,6 +214,18 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > } > NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string_size)); > > +#define DEFINE_FETCH_comm(type) \ > +void FETCH_FUNC_NAME(comm, type)(struct pt_regs *regs, \ > + void *data, void *dest) \ > +{ \ > + fetch_memory_##type(regs, current->comm, dest); \ > +} \ > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, type)); > + > +DEFINE_BASIC_FETCH_FUNCS(comm) > +DEFINE_FETCH_comm(string) > +DEFINE_FETCH_comm(string_size) I think you can define default type fetch functions (such as u8, ...) to NULL to make sure it's used as string type only. > + > #define DEFINE_FETCH_symbol(type) \ > void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs, void *data, void *dest)\ > { \ > @@ -587,6 +599,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..918775fcacd3 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -348,6 +348,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, > } > } else > ret = -EINVAL; > + } else if (strcmp(arg, "comm") == 0) { > + if (is_kprobe) > + f->fn = t->fetch[FETCH_MTD_comm]; > + else > + ret = -EINVAL; I think it should work with uprobes too. Why not enabling it to uprobes? Thanks, Namhyung > } else > ret = -EINVAL; > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > index f6398db09114..3f1a987e6dc1 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, > @@ -213,6 +214,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), \ > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > index c53485441c88..f7eedb191d1f 100644 > --- a/kernel/trace/trace_uprobe.c > +++ b/kernel/trace/trace_uprobe.c > @@ -172,6 +172,13 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > *(u32 *)dest = len; > } > > +#define fetch_comm_u8 NULL > +#define fetch_comm_u16 NULL > +#define fetch_comm_u32 NULL > +#define fetch_comm_u64 NULL > +#define fetch_comm_string NULL > +#define fetch_comm_string_size NULL > + > static unsigned long translate_user_vaddr(void *file_offset) > { > unsigned long base_addr; > -- > 2.8.3 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: expose current->comm to kprobe events 2016-06-02 15:40 ` Namhyung Kim @ 2016-06-02 18:21 ` Omar Sandoval 2016-06-03 8:59 ` Masami Hiramatsu 1 sibling, 0 replies; 5+ messages in thread From: Omar Sandoval @ 2016-06-02 18:21 UTC (permalink / raw) To: Namhyung Kim Cc: Steven Rostedt, Ingo Molnar, Masami Hiramatsu, LKML, kernel-team, Omar Sandoval Hi, On Fri, Jun 03, 2016 at 12:40:02AM +0900, Namhyung Kim wrote: > Hello, > > On Wed, Jun 1, 2016 at 3:17 PM, 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 event > > variable $comm which provides exactly that. > > > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > --- > > This is a problem I ran into while trying to debug an unrelated issue. I > > was considering trying to make ftrace try harder to save the command > > line, but that would most likely be at the expense at most events which > > don't care about the task comm. This is much less work. > > > > Documentation/trace/kprobetrace.txt | 2 ++ > > kernel/trace/trace_kprobe.c | 13 +++++++++++++ > > kernel/trace/trace_probe.c | 5 +++++ > > kernel/trace/trace_probe.h | 2 ++ > > kernel/trace/trace_uprobe.c | 7 +++++++ > > 5 files changed, 29 insertions(+) > > > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > > index d68ea5fc812b..90914cbc1e51 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 > > ----- > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 5546eec0505f..e3a087552d49 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -214,6 +214,18 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > } > > NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string_size)); > > > > +#define DEFINE_FETCH_comm(type) \ > > +void FETCH_FUNC_NAME(comm, type)(struct pt_regs *regs, \ > > + void *data, void *dest) \ > > +{ \ > > + fetch_memory_##type(regs, current->comm, dest); \ > > +} \ > > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, type)); > > + > > +DEFINE_BASIC_FETCH_FUNCS(comm) > > +DEFINE_FETCH_comm(string) > > +DEFINE_FETCH_comm(string_size) > > I think you can define default type fetch functions (such as u8, ...) > to NULL to make sure it's used as string type only. Yeah, my original thinking was to not make $comm a special case, but it really is kind of useless to get it as anything other than a string. I'll also try to make the type default to string just for $comm. > > + > > #define DEFINE_FETCH_symbol(type) \ > > void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs, void *data, void *dest)\ > > { \ > > @@ -587,6 +599,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..918775fcacd3 100644 > > --- a/kernel/trace/trace_probe.c > > +++ b/kernel/trace/trace_probe.c > > @@ -348,6 +348,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, > > } > > } else > > ret = -EINVAL; > > + } else if (strcmp(arg, "comm") == 0) { > > + if (is_kprobe) > > + f->fn = t->fetch[FETCH_MTD_comm]; > > + else > > + ret = -EINVAL; > > I think it should work with uprobes too. Why not enabling it to uprobes? You're right, it should work for uprobes, too, I'll add that. Thanks for taking a look! > Thanks, > Namhyung > > > > } else > > ret = -EINVAL; > > > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > index f6398db09114..3f1a987e6dc1 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, > > @@ -213,6 +214,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), \ > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index c53485441c88..f7eedb191d1f 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -172,6 +172,13 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > *(u32 *)dest = len; > > } > > > > +#define fetch_comm_u8 NULL > > +#define fetch_comm_u16 NULL > > +#define fetch_comm_u32 NULL > > +#define fetch_comm_u64 NULL > > +#define fetch_comm_string NULL > > +#define fetch_comm_string_size NULL > > + > > static unsigned long translate_user_vaddr(void *file_offset) > > { > > unsigned long base_addr; > > -- > > 2.8.3 > > -- Omar ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: expose current->comm to kprobe events 2016-06-02 15:40 ` Namhyung Kim 2016-06-02 18:21 ` Omar Sandoval @ 2016-06-03 8:59 ` Masami Hiramatsu 2016-06-03 16:41 ` Omar Sandoval 1 sibling, 1 reply; 5+ messages in thread From: Masami Hiramatsu @ 2016-06-03 8:59 UTC (permalink / raw) To: Namhyung Kim Cc: Omar Sandoval, Steven Rostedt, Ingo Molnar, Masami Hiramatsu, LKML, kernel-team, Omar Sandoval On Fri, 3 Jun 2016 00:40:02 +0900 Namhyung Kim <namhyung@gmail.com> wrote: > Hello, > > On Wed, Jun 1, 2016 at 3:17 PM, 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 event > > variable $comm which provides exactly that. Hmm, would you already tried to trace task/task_newtask and task/task_rename events? which also gives you task's comm. Of course there are no way to get running task's comm... Anyway, I also think more generic idea, e.g. enable to get "current" address. With the "current" address, we can do similar thing (by using perf-probe) The code looks good to me. As Namhyung said, it should support uprobes too. BTW, now I'm considering integrate kprobes and uprobes ftrace interface. It is easy to classify given event definition is for kprobes or uprobes. So, we don't need to have 2 different interfaces for it. Thank you, > > > > Signed-off-by: Omar Sandoval <osandov@fb.com> > > --- > > This is a problem I ran into while trying to debug an unrelated issue. I > > was considering trying to make ftrace try harder to save the command > > line, but that would most likely be at the expense at most events which > > don't care about the task comm. This is much less work. > > > > Documentation/trace/kprobetrace.txt | 2 ++ > > kernel/trace/trace_kprobe.c | 13 +++++++++++++ > > kernel/trace/trace_probe.c | 5 +++++ > > kernel/trace/trace_probe.h | 2 ++ > > kernel/trace/trace_uprobe.c | 7 +++++++ > > 5 files changed, 29 insertions(+) > > > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > > index d68ea5fc812b..90914cbc1e51 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 > > ----- > > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > > index 5546eec0505f..e3a087552d49 100644 > > --- a/kernel/trace/trace_kprobe.c > > +++ b/kernel/trace/trace_kprobe.c > > @@ -214,6 +214,18 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > } > > NOKPROBE_SYMBOL(FETCH_FUNC_NAME(memory, string_size)); > > > > +#define DEFINE_FETCH_comm(type) \ > > +void FETCH_FUNC_NAME(comm, type)(struct pt_regs *regs, \ > > + void *data, void *dest) \ > > +{ \ > > + fetch_memory_##type(regs, current->comm, dest); \ > > +} \ > > +NOKPROBE_SYMBOL(FETCH_FUNC_NAME(comm, type)); > > + > > +DEFINE_BASIC_FETCH_FUNCS(comm) > > +DEFINE_FETCH_comm(string) > > +DEFINE_FETCH_comm(string_size) > > I think you can define default type fetch functions (such as u8, ...) > to NULL to make sure it's used as string type only. > > > > + > > #define DEFINE_FETCH_symbol(type) \ > > void FETCH_FUNC_NAME(symbol, type)(struct pt_regs *regs, void *data, void *dest)\ > > { \ > > @@ -587,6 +599,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..918775fcacd3 100644 > > --- a/kernel/trace/trace_probe.c > > +++ b/kernel/trace/trace_probe.c > > @@ -348,6 +348,11 @@ static int parse_probe_vars(char *arg, const struct fetch_type *t, > > } > > } else > > ret = -EINVAL; > > + } else if (strcmp(arg, "comm") == 0) { > > + if (is_kprobe) > > + f->fn = t->fetch[FETCH_MTD_comm]; > > + else > > + ret = -EINVAL; > > I think it should work with uprobes too. Why not enabling it to uprobes? > > Thanks, > Namhyung > > > > } else > > ret = -EINVAL; > > > > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > > index f6398db09114..3f1a987e6dc1 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, > > @@ -213,6 +214,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), \ > > diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c > > index c53485441c88..f7eedb191d1f 100644 > > --- a/kernel/trace/trace_uprobe.c > > +++ b/kernel/trace/trace_uprobe.c > > @@ -172,6 +172,13 @@ static void FETCH_FUNC_NAME(memory, string_size)(struct pt_regs *regs, > > *(u32 *)dest = len; > > } > > > > +#define fetch_comm_u8 NULL > > +#define fetch_comm_u16 NULL > > +#define fetch_comm_u32 NULL > > +#define fetch_comm_u64 NULL > > +#define fetch_comm_string NULL > > +#define fetch_comm_string_size NULL > > + > > static unsigned long translate_user_vaddr(void *file_offset) > > { > > unsigned long base_addr; > > -- > > 2.8.3 > > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] tracing: expose current->comm to kprobe events 2016-06-03 8:59 ` Masami Hiramatsu @ 2016-06-03 16:41 ` Omar Sandoval 0 siblings, 0 replies; 5+ messages in thread From: Omar Sandoval @ 2016-06-03 16:41 UTC (permalink / raw) To: Masami Hiramatsu Cc: Namhyung Kim, Steven Rostedt, Ingo Molnar, LKML, kernel-team, Omar Sandoval On Fri, Jun 03, 2016 at 05:59:43PM +0900, Masami Hiramatsu wrote: > On Fri, 3 Jun 2016 00:40:02 +0900 > Namhyung Kim <namhyung@gmail.com> wrote: > > > Hello, > > > > On Wed, Jun 1, 2016 at 3:17 PM, 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 event > > > variable $comm which provides exactly that. > > Hmm, would you already tried to trace task/task_newtask and task/task_rename > events? which also gives you task's comm. > Of course there are no way to get running task's comm... Right, I was trying to figure out what task was hitting a specific code path, so I don't think that would work. > Anyway, I also think more generic idea, e.g. enable to get "current" address. > With the "current" address, we can do similar thing (by using perf-probe) Yeah, that would also be useful in general. I think having this is still convenient for quick debugging. > The code looks good to me. As Namhyung said, it should support uprobes > too. Yesterday I sent out v2 which also supports uprobes, let me know if you didn't receive it. > BTW, now I'm considering integrate kprobes and uprobes ftrace interface. > It is easy to classify given event definition is for kprobes or uprobes. > So, we don't need to have 2 different interfaces for it. > > Thank you, Thanks for taking a look. -- Omar ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-06-03 16:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-06-01 6:17 [PATCH] tracing: expose current->comm to kprobe events Omar Sandoval 2016-06-02 15:40 ` Namhyung Kim 2016-06-02 18:21 ` Omar Sandoval 2016-06-03 8:59 ` Masami Hiramatsu 2016-06-03 16:41 ` Omar Sandoval
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox