* [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events @ 2017-03-28 13:52 Alban Crequy 2017-03-28 15:11 ` Steven Rostedt 2017-03-28 15:23 ` Masami Hiramatsu 0 siblings, 2 replies; 7+ messages in thread From: Alban Crequy @ 2017-03-28 13:52 UTC (permalink / raw) To: Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Steven Rostedt, Ingo Molnar Cc: Masami Hiramatsu, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, iago, michael When a kretprobe is installed on a kernel function, there is a maximum limit of how many calls in parallel it can catch (aka "maxactive"). A kernel module could call register_kretprobe() and initialize maxactive (see example in samples/kprobes/kretprobe_example.c). But that is not exposed to userspace and it is currently not possible to choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events The default maxactive can be as low as 1 on single-core with a non-preemptive kernel. This is too low and we need to increase it not only for recursive functions, but for functions that sleep or resched. This patch updates the format of the command that can be written to kprobe_events so that maxactive can be optionally specified. I need this for a bpf program attached to the kretprobe of inet_csk_accept, which can sleep for a long time. BugLink: https://github.com/iovisor/bcc/issues/1072 Signed-off-by: Alban Crequy <alban@kinvolk.io> --- Documentation/trace/kprobetrace.txt | 4 +++- kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++----- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt index 41ef9d8..655ca7e 100644 --- a/Documentation/trace/kprobetrace.txt +++ b/Documentation/trace/kprobetrace.txt @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via Synopsis of kprobe_events ------------------------- p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe -:[GRP/]EVENT : Clear a probe GRP : Group name. If omitted, use "kprobes" for it. @@ -32,6 +32,8 @@ Synopsis of kprobe_events MOD : Module name which has given SYM. SYM[+offs] : Symbol+offset where the probe is inserted. MEMADDR : Address where the probe is inserted. + MAXACTIVE : Maximum number of instances of the specified function that + can be probed simultaneously, or 0 for the default.(*) FETCHARGS : Arguments. Each probe can have up to 128 args. %REG : Fetch register REG diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c index 5f688cc..807e01c 100644 --- a/kernel/trace/trace_kprobe.c +++ b/kernel/trace/trace_kprobe.c @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, void *addr, const char *symbol, unsigned long offs, + int maxactive, int nargs, bool is_return) { struct trace_kprobe *tk; @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, else tk->rp.kp.pre_handler = kprobe_dispatcher; + tk->rp.maxactive = maxactive; + if (!event || !is_good_name(event)) { ret = -EINVAL; goto error; @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) { /* * Argument syntax: - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] + * - Add kprobe: + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] + * - Add kretprobe: + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] * Fetch args: * $retval : fetch return value * $stack : fetch stack address @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) int i, ret = 0; bool is_return = false, is_delete = false; char *symbol = NULL, *event = NULL, *group = NULL; + int maxactive = 0; char *arg; unsigned long offset = 0; void *addr = NULL; @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) return -EINVAL; } - if (argv[0][1] == ':') { + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { + event = strchr(&argv[0][1], ':') + 1; + event[-1] = '\0'; + ret = kstrtouint(&argv[0][1], 0, &maxactive); + if (ret) { + pr_info("Failed to parse maxactive.\n"); + return ret; + } + /* kretprobes instances are iterated over via a list. The + * maximum should stay reasonable. + */ + if (maxactive > 1024) { + pr_info("Maxactive is too big.\n"); + return -EINVAL; + } + } else if (argv[0][1] == ':') { event = &argv[0][2]; + } + + if (event) { if (strchr(event, '/')) { group = event; event = strchr(group, '/') + 1; @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) is_return ? 'r' : 'p', addr); event = buf; } - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, - is_return); + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, + argc, is_return); if (IS_ERR(tk)) { pr_info("Failed to allocate trace_probe.(%d)\n", (int)PTR_ERR(tk)); -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 13:52 [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events Alban Crequy @ 2017-03-28 15:11 ` Steven Rostedt 2017-03-28 15:23 ` Masami Hiramatsu 1 sibling, 0 replies; 7+ messages in thread From: Steven Rostedt @ 2017-03-28 15:11 UTC (permalink / raw) To: Alban Crequy Cc: Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Ingo Molnar, Masami Hiramatsu, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, iago, michael "[PATCH v1]" I like your confidence, or lack of, that there isn't going to be a v2 or v3 ;-) Masami, what do you think of this? -- Steve On Tue, 28 Mar 2017 15:52:22 +0200 Alban Crequy <alban.crequy@gmail.com> wrote: > When a kretprobe is installed on a kernel function, there is a maximum > limit of how many calls in parallel it can catch (aka "maxactive"). A > kernel module could call register_kretprobe() and initialize maxactive > (see example in samples/kprobes/kretprobe_example.c). > > But that is not exposed to userspace and it is currently not possible to > choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events > > The default maxactive can be as low as 1 on single-core with a > non-preemptive kernel. This is too low and we need to increase it not > only for recursive functions, but for functions that sleep or resched. > > This patch updates the format of the command that can be written to > kprobe_events so that maxactive can be optionally specified. > > I need this for a bpf program attached to the kretprobe of > inet_csk_accept, which can sleep for a long time. > > BugLink: https://github.com/iovisor/bcc/issues/1072 > Signed-off-by: Alban Crequy <alban@kinvolk.io> > --- > Documentation/trace/kprobetrace.txt | 4 +++- > kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++----- > 2 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > index 41ef9d8..655ca7e 100644 > --- a/Documentation/trace/kprobetrace.txt > +++ b/Documentation/trace/kprobetrace.txt > @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via > Synopsis of kprobe_events > ------------------------- > p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe > - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > -:[GRP/]EVENT : Clear a probe > > GRP : Group name. If omitted, use "kprobes" for it. > @@ -32,6 +32,8 @@ Synopsis of kprobe_events > MOD : Module name which has given SYM. > SYM[+offs] : Symbol+offset where the probe is inserted. > MEMADDR : Address where the probe is inserted. > + MAXACTIVE : Maximum number of instances of the specified function that > + can be probed simultaneously, or 0 for the default.(*) > > FETCHARGS : Arguments. Each probe can have up to 128 args. > %REG : Fetch register REG > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5f688cc..807e01c 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > void *addr, > const char *symbol, > unsigned long offs, > + int maxactive, > int nargs, bool is_return) > { > struct trace_kprobe *tk; > @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > else > tk->rp.kp.pre_handler = kprobe_dispatcher; > > + tk->rp.maxactive = maxactive; > + > if (!event || !is_good_name(event)) { > ret = -EINVAL; > goto error; > @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) > { > /* > * Argument syntax: > - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > + * - Add kprobe: > + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > + * - Add kretprobe: > + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > * Fetch args: > * $retval : fetch return value > * $stack : fetch stack address > @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) > int i, ret = 0; > bool is_return = false, is_delete = false; > char *symbol = NULL, *event = NULL, *group = NULL; > + int maxactive = 0; > char *arg; > unsigned long offset = 0; > void *addr = NULL; > @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) > return -EINVAL; > } > > - if (argv[0][1] == ':') { > + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { > + event = strchr(&argv[0][1], ':') + 1; > + event[-1] = '\0'; > + ret = kstrtouint(&argv[0][1], 0, &maxactive); > + if (ret) { > + pr_info("Failed to parse maxactive.\n"); > + return ret; > + } > + /* kretprobes instances are iterated over via a list. The > + * maximum should stay reasonable. > + */ > + if (maxactive > 1024) { > + pr_info("Maxactive is too big.\n"); > + return -EINVAL; > + } > + } else if (argv[0][1] == ':') { > event = &argv[0][2]; > + } > + > + if (event) { > if (strchr(event, '/')) { > group = event; > event = strchr(group, '/') + 1; > @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) > is_return ? 'r' : 'p', addr); > event = buf; > } > - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, > - is_return); > + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, > + argc, is_return); > if (IS_ERR(tk)) { > pr_info("Failed to allocate trace_probe.(%d)\n", > (int)PTR_ERR(tk)); ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 13:52 [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events Alban Crequy 2017-03-28 15:11 ` Steven Rostedt @ 2017-03-28 15:23 ` Masami Hiramatsu 2017-03-28 15:34 ` Steven Rostedt 2017-03-28 16:08 ` Alban Crequy 1 sibling, 2 replies; 7+ messages in thread From: Masami Hiramatsu @ 2017-03-28 15:23 UTC (permalink / raw) To: Alban Crequy Cc: Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Steven Rostedt, Ingo Molnar, Masami Hiramatsu, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, iago, michael, Dorau, Lukasz On Tue, 28 Mar 2017 15:52:22 +0200 Alban Crequy <alban.crequy@gmail.com> wrote: > When a kretprobe is installed on a kernel function, there is a maximum > limit of how many calls in parallel it can catch (aka "maxactive"). A > kernel module could call register_kretprobe() and initialize maxactive > (see example in samples/kprobes/kretprobe_example.c). > > But that is not exposed to userspace and it is currently not possible to > choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events I see, I found same issue last week... > > The default maxactive can be as low as 1 on single-core with a > non-preemptive kernel. This is too low and we need to increase it not > only for recursive functions, but for functions that sleep or resched. > > This patch updates the format of the command that can be written to > kprobe_events so that maxactive can be optionally specified. Good! this is completely same what I'm planning to add. > > I need this for a bpf program attached to the kretprobe of > inet_csk_accept, which can sleep for a long time. I'm also preparing another patch for using kmalloc(GFP_ATOMIC) in kretprobe_pre_handler(), since this manual way is sometimes hard to estimate how many instances needed. Anyway, since that may involve unwilling memory allocation, this patch also needed. > > BugLink: https://github.com/iovisor/bcc/issues/1072 Could you also add Lukasz to Cc list, since he had reported an issue related this one. https://www.spinics.net/lists/linux-trace/msg00448.html Please see my comments below. > Signed-off-by: Alban Crequy <alban@kinvolk.io> > --- > Documentation/trace/kprobetrace.txt | 4 +++- > kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++----- > 2 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > index 41ef9d8..655ca7e 100644 > --- a/Documentation/trace/kprobetrace.txt > +++ b/Documentation/trace/kprobetrace.txt > @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via > Synopsis of kprobe_events > ------------------------- > p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe > - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > -:[GRP/]EVENT : Clear a probe > > GRP : Group name. If omitted, use "kprobes" for it. > @@ -32,6 +32,8 @@ Synopsis of kprobe_events > MOD : Module name which has given SYM. > SYM[+offs] : Symbol+offset where the probe is inserted. > MEMADDR : Address where the probe is inserted. > + MAXACTIVE : Maximum number of instances of the specified function that > + can be probed simultaneously, or 0 for the default.(*) Here, yoy don't need (*), since [MAXACTIVE] is not a FETCHARGS. > > FETCHARGS : Arguments. Each probe can have up to 128 args. > %REG : Fetch register REG > diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > index 5f688cc..807e01c 100644 > --- a/kernel/trace/trace_kprobe.c > +++ b/kernel/trace/trace_kprobe.c > @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > void *addr, > const char *symbol, > unsigned long offs, > + int maxactive, > int nargs, bool is_return) > { > struct trace_kprobe *tk; > @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > else > tk->rp.kp.pre_handler = kprobe_dispatcher; > > + tk->rp.maxactive = maxactive; > + > if (!event || !is_good_name(event)) { > ret = -EINVAL; > goto error; > @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) > { > /* > * Argument syntax: > - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > + * - Add kprobe: > + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > + * - Add kretprobe: > + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > * Fetch args: > * $retval : fetch return value > * $stack : fetch stack address > @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) > int i, ret = 0; > bool is_return = false, is_delete = false; > char *symbol = NULL, *event = NULL, *group = NULL; > + int maxactive = 0; > char *arg; > unsigned long offset = 0; > void *addr = NULL; > @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) > return -EINVAL; > } > > - if (argv[0][1] == ':') { > + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name. Thank you, > + event = strchr(&argv[0][1], ':') + 1; > + event[-1] = '\0'; > + ret = kstrtouint(&argv[0][1], 0, &maxactive); > + if (ret) { > + pr_info("Failed to parse maxactive.\n"); > + return ret; > + } > + /* kretprobes instances are iterated over via a list. The > + * maximum should stay reasonable. > + */ > + if (maxactive > 1024) { > + pr_info("Maxactive is too big.\n"); > + return -EINVAL; > + } > + } else if (argv[0][1] == ':') { > event = &argv[0][2]; > + } > + > + if (event) { > if (strchr(event, '/')) { > group = event; > event = strchr(group, '/') + 1; > @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) > is_return ? 'r' : 'p', addr); > event = buf; > } > - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, > - is_return); > + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, > + argc, is_return); > if (IS_ERR(tk)) { > pr_info("Failed to allocate trace_probe.(%d)\n", > (int)PTR_ERR(tk)); > -- > 2.7.4 > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 15:23 ` Masami Hiramatsu @ 2017-03-28 15:34 ` Steven Rostedt 2017-03-28 23:44 ` Masami Hiramatsu 2017-03-28 16:08 ` Alban Crequy 1 sibling, 1 reply; 7+ messages in thread From: Steven Rostedt @ 2017-03-28 15:34 UTC (permalink / raw) To: Masami Hiramatsu Cc: Alban Crequy, Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Ingo Molnar, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, iago, michael, Dorau, Lukasz On Wed, 29 Mar 2017 00:23:35 +0900 Masami Hiramatsu <mhiramat@kernel.org> wrote: > > @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) > > { > > /* > > * Argument syntax: > > - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > > - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > > + * - Add kprobe: > > + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > > + * - Add kretprobe: > > + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > > * Fetch args: > > * $retval : fetch return value > > * $stack : fetch stack address > > @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) > > int i, ret = 0; > > bool is_return = false, is_delete = false; > > char *symbol = NULL, *event = NULL, *group = NULL; > > + int maxactive = 0; > > char *arg; > > unsigned long offset = 0; > > void *addr = NULL; > > @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) > > return -EINVAL; > > } > > > > - if (argv[0][1] == ':') { > > + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { > > This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name. You mean it doesn't support adding MAXACTIVE without the ':event'. > > Thank you, > > > + event = strchr(&argv[0][1], ':') + 1; > > + event[-1] = '\0'; > > + ret = kstrtouint(&argv[0][1], 0, &maxactive); > > + if (ret) { > > + pr_info("Failed to parse maxactive.\n"); > > + return ret; > > + } > > + /* kretprobes instances are iterated over via a list. The > > + * maximum should stay reasonable. > > + */ > > + if (maxactive > 1024) { Also, can we get rid of magic numbers within the code. There should be a const or macro defined as MAX_MAXACTIVE or something, and use that here. -- Steve > > + pr_info("Maxactive is too big.\n"); > > + return -EINVAL; > > + } > > + } else if (argv[0][1] == ':') { > > event = &argv[0][2]; > > + } > > + > > + if (event) { > > if (strchr(event, '/')) { > > group = event; > > event = strchr(group, '/') + 1; > > @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) > > is_return ? 'r' : 'p', addr); > > event = buf; > > } > > - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, > > - is_return); > > + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, > > + argc, is_return); > > if (IS_ERR(tk)) { > > pr_info("Failed to allocate trace_probe.(%d)\n", > > (int)PTR_ERR(tk)); > > -- > > 2.7.4 > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 15:34 ` Steven Rostedt @ 2017-03-28 23:44 ` Masami Hiramatsu 0 siblings, 0 replies; 7+ messages in thread From: Masami Hiramatsu @ 2017-03-28 23:44 UTC (permalink / raw) To: Steven Rostedt Cc: Alban Crequy, Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Ingo Molnar, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, iago, michael, Dorau, Lukasz On Tue, 28 Mar 2017 11:34:07 -0400 Steven Rostedt <rostedt@goodmis.org> wrote: > On Wed, 29 Mar 2017 00:23:35 +0900 > Masami Hiramatsu <mhiramat@kernel.org> wrote: > > > > @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) > > > { > > > /* > > > * Argument syntax: > > > - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > > > - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > > > + * - Add kprobe: > > > + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > > > + * - Add kretprobe: > > > + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > > > * Fetch args: > > > * $retval : fetch return value > > > * $stack : fetch stack address > > > @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) > > > int i, ret = 0; > > > bool is_return = false, is_delete = false; > > > char *symbol = NULL, *event = NULL, *group = NULL; > > > + int maxactive = 0; > > > char *arg; > > > unsigned long offset = 0; > > > void *addr = NULL; > > > @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) > > > return -EINVAL; > > > } > > > > > > - if (argv[0][1] == ':') { > > > + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { > > > > This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name. > > You mean it doesn't support adding MAXACTIVE without the ':event'. Yeah, sorry for lacking the explanation... > > > > > Thank you, > > > > > + event = strchr(&argv[0][1], ':') + 1; > > > + event[-1] = '\0'; > > > + ret = kstrtouint(&argv[0][1], 0, &maxactive); > > > + if (ret) { > > > + pr_info("Failed to parse maxactive.\n"); > > > + return ret; > > > + } > > > + /* kretprobes instances are iterated over via a list. The > > > + * maximum should stay reasonable. > > > + */ > > > + if (maxactive > 1024) { > > Also, can we get rid of magic numbers within the code. There should be a > const or macro defined as MAX_MAXACTIVE or something, and use that here. Good catch! Thanks, > > -- Steve > > > > > + pr_info("Maxactive is too big.\n"); > > > + return -EINVAL; > > > + } > > > + } else if (argv[0][1] == ':') { > > > event = &argv[0][2]; > > > + } > > > + > > > + if (event) { > > > if (strchr(event, '/')) { > > > group = event; > > > event = strchr(group, '/') + 1; > > > @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) > > > is_return ? 'r' : 'p', addr); > > > event = buf; > > > } > > > - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, > > > - is_return); > > > + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, > > > + argc, is_return); > > > if (IS_ERR(tk)) { > > > pr_info("Failed to allocate trace_probe.(%d)\n", > > > (int)PTR_ERR(tk)); > > > -- > > > 2.7.4 > > > > > > > > -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 15:23 ` Masami Hiramatsu 2017-03-28 15:34 ` Steven Rostedt @ 2017-03-28 16:08 ` Alban Crequy 2017-03-28 23:50 ` Masami Hiramatsu 1 sibling, 1 reply; 7+ messages in thread From: Alban Crequy @ 2017-03-28 16:08 UTC (permalink / raw) To: Masami Hiramatsu Cc: Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Steven Rostedt, Ingo Molnar, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, Iago López Galeiras, Michael Schubert, Dorau, Lukasz Thanks for the review, On Tue, Mar 28, 2017 at 5:23 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > On Tue, 28 Mar 2017 15:52:22 +0200 > Alban Crequy <alban.crequy@gmail.com> wrote: > >> When a kretprobe is installed on a kernel function, there is a maximum >> limit of how many calls in parallel it can catch (aka "maxactive"). A >> kernel module could call register_kretprobe() and initialize maxactive >> (see example in samples/kprobes/kretprobe_example.c). >> >> But that is not exposed to userspace and it is currently not possible to >> choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events > > I see, I found same issue last week... > >> >> The default maxactive can be as low as 1 on single-core with a >> non-preemptive kernel. This is too low and we need to increase it not >> only for recursive functions, but for functions that sleep or resched. >> >> This patch updates the format of the command that can be written to >> kprobe_events so that maxactive can be optionally specified. > > Good! this is completely same what I'm planning to add. > >> >> I need this for a bpf program attached to the kretprobe of >> inet_csk_accept, which can sleep for a long time. > > I'm also preparing another patch for using kmalloc(GFP_ATOMIC) in > kretprobe_pre_handler(), since this manual way is sometimes hard to > estimate how many instances needed. Anyway, since that may involve > unwilling memory allocation, this patch also needed. Where is that kretprobe_pre_handler()? I don't see any allocations in pre_handler_kretprobe(). >> BugLink: https://github.com/iovisor/bcc/issues/1072 > > Could you also add Lukasz to Cc list, since he had reported an issue > related this one. > > https://www.spinics.net/lists/linux-trace/msg00448.html > > Please see my comments below. > >> Signed-off-by: Alban Crequy <alban@kinvolk.io> >> --- >> Documentation/trace/kprobetrace.txt | 4 +++- >> kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++----- >> 2 files changed, 32 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt >> index 41ef9d8..655ca7e 100644 >> --- a/Documentation/trace/kprobetrace.txt >> +++ b/Documentation/trace/kprobetrace.txt >> @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via >> Synopsis of kprobe_events >> ------------------------- >> p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe >> - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe >> + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe >> -:[GRP/]EVENT : Clear a probe >> >> GRP : Group name. If omitted, use "kprobes" for it. >> @@ -32,6 +32,8 @@ Synopsis of kprobe_events >> MOD : Module name which has given SYM. >> SYM[+offs] : Symbol+offset where the probe is inserted. >> MEMADDR : Address where the probe is inserted. >> + MAXACTIVE : Maximum number of instances of the specified function that >> + can be probed simultaneously, or 0 for the default.(*) > > Here, yoy don't need (*), since [MAXACTIVE] is not a FETCHARGS. Why not? (*) refers to "(*) only for return probe." and the maxactive only makes sense for the kretprobe. >> FETCHARGS : Arguments. Each probe can have up to 128 args. >> %REG : Fetch register REG >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c >> index 5f688cc..807e01c 100644 >> --- a/kernel/trace/trace_kprobe.c >> +++ b/kernel/trace/trace_kprobe.c >> @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, >> void *addr, >> const char *symbol, >> unsigned long offs, >> + int maxactive, >> int nargs, bool is_return) >> { >> struct trace_kprobe *tk; >> @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, >> else >> tk->rp.kp.pre_handler = kprobe_dispatcher; >> >> + tk->rp.maxactive = maxactive; >> + >> if (!event || !is_good_name(event)) { >> ret = -EINVAL; >> goto error; >> @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) >> { >> /* >> * Argument syntax: >> - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] >> - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] >> + * - Add kprobe: >> + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] >> + * - Add kretprobe: >> + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] >> * Fetch args: >> * $retval : fetch return value >> * $stack : fetch stack address >> @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) >> int i, ret = 0; >> bool is_return = false, is_delete = false; >> char *symbol = NULL, *event = NULL, *group = NULL; >> + int maxactive = 0; >> char *arg; >> unsigned long offset = 0; >> void *addr = NULL; >> @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) >> return -EINVAL; >> } >> >> - if (argv[0][1] == ':') { >> + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { > > This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name. > > Thank you, Ok, I will fix this. By the way, the current code does not error out when there are extra characters: "pfuzz inet_csk_accept" is currently a valid command. I didn't change that. >> + event = strchr(&argv[0][1], ':') + 1; >> + event[-1] = '\0'; >> + ret = kstrtouint(&argv[0][1], 0, &maxactive); >> + if (ret) { >> + pr_info("Failed to parse maxactive.\n"); >> + return ret; >> + } >> + /* kretprobes instances are iterated over via a list. The >> + * maximum should stay reasonable. >> + */ >> + if (maxactive > 1024) { >> + pr_info("Maxactive is too big.\n"); >> + return -EINVAL; >> + } >> + } else if (argv[0][1] == ':') { >> event = &argv[0][2]; >> + } >> + >> + if (event) { >> if (strchr(event, '/')) { >> group = event; >> event = strchr(group, '/') + 1; >> @@ -718,8 +742,8 @@ static int create_trace_kprobe(int argc, char **argv) >> is_return ? 'r' : 'p', addr); >> event = buf; >> } >> - tk = alloc_trace_kprobe(group, event, addr, symbol, offset, argc, >> - is_return); >> + tk = alloc_trace_kprobe(group, event, addr, symbol, offset, maxactive, >> + argc, is_return); >> if (IS_ERR(tk)) { >> pr_info("Failed to allocate trace_probe.(%d)\n", >> (int)PTR_ERR(tk)); >> -- >> 2.7.4 >> > > > -- > Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events 2017-03-28 16:08 ` Alban Crequy @ 2017-03-28 23:50 ` Masami Hiramatsu 0 siblings, 0 replies; 7+ messages in thread From: Masami Hiramatsu @ 2017-03-28 23:50 UTC (permalink / raw) To: Alban Crequy Cc: Alban Crequy, Alexei Starovoitov, Jonathan Corbet, Steven Rostedt, Ingo Molnar, Arnaldo Carvalho de Melo, Omar Sandoval, linux-doc, netdev, linux-kernel, Iago López Galeiras, Michael Schubert, Dorau, Lukasz On Tue, 28 Mar 2017 18:08:16 +0200 Alban Crequy <alban@kinvolk.io> wrote: > Thanks for the review, > > On Tue, Mar 28, 2017 at 5:23 PM, Masami Hiramatsu <mhiramat@kernel.org> wrote: > > On Tue, 28 Mar 2017 15:52:22 +0200 > > Alban Crequy <alban.crequy@gmail.com> wrote: > > > >> When a kretprobe is installed on a kernel function, there is a maximum > >> limit of how many calls in parallel it can catch (aka "maxactive"). A > >> kernel module could call register_kretprobe() and initialize maxactive > >> (see example in samples/kprobes/kretprobe_example.c). > >> > >> But that is not exposed to userspace and it is currently not possible to > >> choose maxactive when writing to /sys/kernel/debug/tracing/kprobe_events > > > > I see, I found same issue last week... > > > >> > >> The default maxactive can be as low as 1 on single-core with a > >> non-preemptive kernel. This is too low and we need to increase it not > >> only for recursive functions, but for functions that sleep or resched. > >> > >> This patch updates the format of the command that can be written to > >> kprobe_events so that maxactive can be optionally specified. > > > > Good! this is completely same what I'm planning to add. > > > >> > >> I need this for a bpf program attached to the kretprobe of > >> inet_csk_accept, which can sleep for a long time. > > > > I'm also preparing another patch for using kmalloc(GFP_ATOMIC) in > > kretprobe_pre_handler(), since this manual way is sometimes hard to > > estimate how many instances needed. Anyway, since that may involve > > unwilling memory allocation, this patch also needed. > > Where is that kretprobe_pre_handler()? I don't see any allocations in > pre_handler_kretprobe(). pre_handler_kretprobe(), I'll send my patch, but note that I also considered to introduce a patch which exactly same as yours. So even with my patch, this patch should be introduced. > > >> BugLink: https://github.com/iovisor/bcc/issues/1072 > > > > Could you also add Lukasz to Cc list, since he had reported an issue > > related this one. > > > > https://www.spinics.net/lists/linux-trace/msg00448.html > > > > Please see my comments below. > > > >> Signed-off-by: Alban Crequy <alban@kinvolk.io> > >> --- > >> Documentation/trace/kprobetrace.txt | 4 +++- > >> kernel/trace/trace_kprobe.c | 34 +++++++++++++++++++++++++++++----- > >> 2 files changed, 32 insertions(+), 6 deletions(-) > >> > >> diff --git a/Documentation/trace/kprobetrace.txt b/Documentation/trace/kprobetrace.txt > >> index 41ef9d8..655ca7e 100644 > >> --- a/Documentation/trace/kprobetrace.txt > >> +++ b/Documentation/trace/kprobetrace.txt > >> @@ -23,7 +23,7 @@ current_tracer. Instead of that, add probe points via > >> Synopsis of kprobe_events > >> ------------------------- > >> p[:[GRP/]EVENT] [MOD:]SYM[+offs]|MEMADDR [FETCHARGS] : Set a probe > >> - r[:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > >> + r[MAXACTIVE][:[GRP/]EVENT] [MOD:]SYM[+0] [FETCHARGS] : Set a return probe > >> -:[GRP/]EVENT : Clear a probe > >> > >> GRP : Group name. If omitted, use "kprobes" for it. > >> @@ -32,6 +32,8 @@ Synopsis of kprobe_events > >> MOD : Module name which has given SYM. > >> SYM[+offs] : Symbol+offset where the probe is inserted. > >> MEMADDR : Address where the probe is inserted. > >> + MAXACTIVE : Maximum number of instances of the specified function that > >> + can be probed simultaneously, or 0 for the default.(*) > > > > Here, yoy don't need (*), since [MAXACTIVE] is not a FETCHARGS. > > Why not? (*) refers to "(*) only for return probe." and the maxactive > only makes sense for the kretprobe. Because simply synopsis already explained MAXACTIVE is only for 'r' :) The reason why I added the (*) note is that FETCHARGS are shown in both synopsis of 'p' and 'r'. > > >> FETCHARGS : Arguments. Each probe can have up to 128 args. > >> %REG : Fetch register REG > >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c > >> index 5f688cc..807e01c 100644 > >> --- a/kernel/trace/trace_kprobe.c > >> +++ b/kernel/trace/trace_kprobe.c > >> @@ -282,6 +282,7 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > >> void *addr, > >> const char *symbol, > >> unsigned long offs, > >> + int maxactive, > >> int nargs, bool is_return) > >> { > >> struct trace_kprobe *tk; > >> @@ -309,6 +310,8 @@ static struct trace_kprobe *alloc_trace_kprobe(const char *group, > >> else > >> tk->rp.kp.pre_handler = kprobe_dispatcher; > >> > >> + tk->rp.maxactive = maxactive; > >> + > >> if (!event || !is_good_name(event)) { > >> ret = -EINVAL; > >> goto error; > >> @@ -598,8 +601,10 @@ static int create_trace_kprobe(int argc, char **argv) > >> { > >> /* > >> * Argument syntax: > >> - * - Add kprobe: p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > >> - * - Add kretprobe: r[:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > >> + * - Add kprobe: > >> + * p[:[GRP/]EVENT] [MOD:]KSYM[+OFFS]|KADDR [FETCHARGS] > >> + * - Add kretprobe: > >> + * r[MAXACTIVE][:[GRP/]EVENT] [MOD:]KSYM[+0] [FETCHARGS] > >> * Fetch args: > >> * $retval : fetch return value > >> * $stack : fetch stack address > >> @@ -619,6 +624,7 @@ static int create_trace_kprobe(int argc, char **argv) > >> int i, ret = 0; > >> bool is_return = false, is_delete = false; > >> char *symbol = NULL, *event = NULL, *group = NULL; > >> + int maxactive = 0; > >> char *arg; > >> unsigned long offset = 0; > >> void *addr = NULL; > >> @@ -637,8 +643,26 @@ static int create_trace_kprobe(int argc, char **argv) > >> return -EINVAL; > >> } > >> > >> - if (argv[0][1] == ':') { > >> + if (is_return && isdigit(argv[0][1]) && strchr(&argv[0][1], ':')) { > > > > This only supports r[MAXACTIVE:[GRP/]EVENT]. e.g "r100" without event name. > > > > Thank you, > > Ok, I will fix this. > > By the way, the current code does not error out when there are extra characters: > "pfuzz inet_csk_accept" is currently a valid command. > I didn't change that. OK, that is another improvement. Thank you! -- Masami Hiramatsu <mhiramat@kernel.org> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-28 23:51 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-28 13:52 [PATCH v1] tracing/kprobes: expose maxactive for kretprobe in kprobe_events Alban Crequy 2017-03-28 15:11 ` Steven Rostedt 2017-03-28 15:23 ` Masami Hiramatsu 2017-03-28 15:34 ` Steven Rostedt 2017-03-28 23:44 ` Masami Hiramatsu 2017-03-28 16:08 ` Alban Crequy 2017-03-28 23:50 ` Masami Hiramatsu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.