From: Masami Hiramatsu <mhiramat@kernel.org>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
linux-kernel@vger.kernel.org, Ingo Molnar <mingo@kernel.org>,
Andrew Morton <akpm@linux-foundation.org>,
Thomas Gleixner <tglx@linutronix.de>,
Peter Zijlstra <peterz@infradead.org>,
Masami Hiramatsu <mhiramat@kernel.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Frederic Weisbecker <frederic@kernel.org>,
Andy Lutomirski <luto@kernel.org>,
Mark Rutland <mark.rutland@arm.com>
Subject: Re: [RFC][PATCH 11/14] function_graph: Convert ret_stack to a series of longs
Date: Tue, 27 Nov 2018 01:07:55 +0900 [thread overview]
Message-ID: <20181127010755.0f897c13a57315a3859d225b@kernel.org> (raw)
In-Reply-To: <20181124053138.GA242510@google.com>
On Fri, 23 Nov 2018 21:31:38 -0800
Joel Fernandes <joel@joelfernandes.org> wrote:
> On Wed, Nov 21, 2018 at 08:27:19PM -0500, Steven Rostedt wrote:
> > From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> >
> > In order to make it possible to have multiple callbacks registered with the
> > function_graph tracer, the retstack needs to be converted from an array of
> > ftrace_ret_stack structures to an array of longs. This will allow to store
> > the list of callbacks on the stack for the return side of the functions.
> >
> > [ Note, this currently breaks architectures that access the ret_stack of a
> > task to handle unwinding when 'return_to_handler' is on the stack ]
> >
> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> > ---
> > include/linux/sched.h | 2 +-
> > kernel/trace/fgraph.c | 123 +++++++++++++++++++++++-------------------
> > 2 files changed, 70 insertions(+), 55 deletions(-)
> >
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index d6183a55e8eb..71a084a300da 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -1119,7 +1119,7 @@ struct task_struct {
> > int curr_ret_depth;
> >
> > /* Stack of return addresses for return function tracing: */
> > - struct ftrace_ret_stack *ret_stack;
> > + unsigned long *ret_stack;
> >
> > /* Timestamp for last schedule: */
> > unsigned long long ftrace_timestamp;
> > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > index 9b85638ecded..1389fe39f64c 100644
> > --- a/kernel/trace/fgraph.c
> > +++ b/kernel/trace/fgraph.c
> > @@ -23,6 +23,17 @@
> > #define ASSIGN_OPS_HASH(opsname, val)
> > #endif
> >
> > +#define FGRAPH_RET_SIZE (sizeof(struct ftrace_ret_stack))
> > +#define FGRAPH_RET_INDEX (ALIGN(FGRAPH_RET_SIZE, sizeof(long)) / sizeof(long))
> > +#define SHADOW_STACK_SIZE (FTRACE_RETFUNC_DEPTH * FGRAPH_RET_SIZE)
> > +#define SHADOW_STACK_INDEX \
> > + (ALIGN(SHADOW_STACK_SIZE, sizeof(long)) / sizeof(long))
> > +#define SHADOW_STACK_MAX_INDEX (SHADOW_STACK_INDEX - FGRAPH_RET_INDEX)
> > +
> > +#define RET_STACK(t, index) ((struct ftrace_ret_stack *)(&(t)->ret_stack[index]))
> > +#define RET_STACK_INC(c) ({ c += FGRAPH_RET_INDEX; })
> > +#define RET_STACK_DEC(c) ({ c -= FGRAPH_RET_INDEX; })
> > +
> [...]
> > @@ -514,7 +531,7 @@ void ftrace_graph_init_task(struct task_struct *t)
> >
> > void ftrace_graph_exit_task(struct task_struct *t)
> > {
> > - struct ftrace_ret_stack *ret_stack = t->ret_stack;
> > + unsigned long *ret_stack = t->ret_stack;
> >
> > t->ret_stack = NULL;
> > /* NULL must become visible to IRQs before we free it: */
> > @@ -526,12 +543,10 @@ void ftrace_graph_exit_task(struct task_struct *t)
> > /* Allocate a return stack for each task */
> > static int start_graph_tracing(void)
> > {
> > - struct ftrace_ret_stack **ret_stack_list;
> > + unsigned long **ret_stack_list;
> > int ret, cpu;
> >
> > - ret_stack_list = kmalloc_array(FTRACE_RETSTACK_ALLOC_SIZE,
> > - sizeof(struct ftrace_ret_stack *),
> > - GFP_KERNEL);
> > + ret_stack_list = kmalloc(SHADOW_STACK_SIZE, GFP_KERNEL);
> >
>
> I had dumped the fgraph size related macros to understand the patch better, I
> got:
> [ 0.909528] val of FGRAPH_RET_SIZE is 40
> [ 0.910250] val of FGRAPH_RET_INDEX is 5
> [ 0.910866] val of FGRAPH_ARRAY_SIZE is 16
> [ 0.911488] val of FGRAPH_ARRAY_MASK is 255
> [ 0.912134] val of FGRAPH_MAX_INDEX is 16
> [ 0.912751] val of FGRAPH_INDEX_SHIFT is 8
> [ 0.913382] val of FGRAPH_FRAME_SIZE is 168
> [ 0.914033] val of FGRAPH_FRAME_INDEX is 21
> FTRACE_RETFUNC_DEPTH is 50
> [ 0.914686] val of SHADOW_STACK_SIZE is 8400
>
> I had a concern about memory overhead per-task. It seems the total memory
> needed per task for the stack is 8400 bytes (with my configuration with
> FUNCTION_PROFILE
> turned off).
>
> Where as before it would be 32 * 40 = 1280 bytes. That looks like ~7 times
> more than before.
Hmm, this seems too big... I thought the shadow-stack size should be
smaller than 1 page (4kB). Steve, can we give a 4k page for shadow stack
and define FTRACE_RETFUNC_DEPTH = 4096 / FGRAPH_RET_SIZE ?
> On my system with ~4000 threads, that becomes ~32MB which seems a bit
> wasteful especially if there was only one or 2 function graph callbacks
> registered and most of the callback array in the stack isn't used.
>
> Could we make the array size configurable at compile time and start it with a
> small number like 4 or 6?
Or, we can introduce online setting :)
Thank you,
>
> Also for patches 1 through 10:
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> thanks,
>
> - Joel
>
--
Masami Hiramatsu <mhiramat@kernel.org>
next prev parent reply other threads:[~2018-11-26 16:08 UTC|newest]
Thread overview: 54+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-22 1:27 [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 01/14] fgraph: Create a fgraph.c file to store function graph infrastructure Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 02/14] fgraph: Have set_graph_notrace only affect function_graph tracer Steven Rostedt
2018-11-23 0:01 ` Namhyung Kim
2018-11-23 17:37 ` Steven Rostedt
2018-11-24 5:49 ` Namhyung Kim
2018-11-24 18:41 ` Steven Rostedt
2018-11-26 4:54 ` Namhyung Kim
2018-11-22 1:27 ` [RFC][PATCH 03/14] arm64: function_graph: Remove use of FTRACE_NOTRACE_DEPTH Steven Rostedt
2018-11-27 19:31 ` Will Deacon
2018-11-27 19:50 ` Steven Rostedt
2018-12-05 21:50 ` [PATCH 03/14 v2] " Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 04/14] function_graph: Remove the " Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 05/14] ftrace: Create new ftrace-internal.h header Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 06/14] fgraph: Move function graph specific code into fgraph.c Steven Rostedt
2018-11-23 6:11 ` Joel Fernandes
2018-11-23 17:58 ` Steven Rostedt
2018-11-23 18:11 ` Steven Rostedt
2018-11-23 22:13 ` Joel Fernandes
2018-11-26 7:25 ` Masami Hiramatsu
2018-11-22 1:27 ` [RFC][PATCH 07/14] fgraph: Add new fgraph_ops structure to enable function graph hooks Steven Rostedt
2018-11-23 2:59 ` Joel Fernandes
2018-11-23 18:25 ` Steven Rostedt
2018-11-26 11:30 ` Masami Hiramatsu
2018-11-26 21:06 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 08/14] function_graph: Remove unused task_curr_ret_stack() Steven Rostedt
2018-11-26 7:40 ` Masami Hiramatsu
2018-11-26 21:26 ` Steven Rostedt
2018-11-26 10:02 ` Joey Pabalinas
2018-11-26 21:27 ` Steven Rostedt
2018-11-26 21:37 ` Joey Pabalinas
2018-11-22 1:27 ` [RFC][PATCH 09/14] function_graph: Move ftrace_graph_get_addr() to fgraph.c Steven Rostedt
2018-11-23 3:13 ` Joel Fernandes
2018-11-23 19:25 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 10/14] function_graph: Have profiler use new helper ftrace_graph_get_ret_stack() Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 11/14] function_graph: Convert ret_stack to a series of longs Steven Rostedt
2018-11-24 5:31 ` Joel Fernandes
2018-11-26 16:07 ` Masami Hiramatsu [this message]
2018-11-26 16:26 ` Steven Rostedt
2018-11-28 1:38 ` Joel Fernandes
2018-11-26 21:31 ` Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 12/14] function_graph: Add an array structure that will allow multiple callbacks Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 13/14] function_graph: Allow multiple users to attach to function graph Steven Rostedt
2018-11-22 1:27 ` [RFC][PATCH 14/14] function_graph: Allow for more than one callback to be registered Steven Rostedt
2018-11-22 10:08 ` [RFC][PATCH 00/14] function_graph: Rewrite to allow multiple users Peter Zijlstra
2018-11-22 12:46 ` Steven Rostedt
2018-11-22 13:42 ` Peter Zijlstra
2018-11-26 9:21 ` Masami Hiramatsu
2018-11-26 16:32 ` Steven Rostedt
2018-11-29 14:29 ` Masami Hiramatsu
2018-11-29 16:46 ` Steven Rostedt
2018-11-30 2:26 ` Masami Hiramatsu
2018-11-30 3:24 ` Steven Rostedt
2018-11-30 14:11 ` Masami Hiramatsu
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=20181127010755.0f897c13a57315a3859d225b@kernel.org \
--to=mhiramat@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=frederic@kernel.org \
--cc=joel@joelfernandes.org \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@kernel.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=tglx@linutronix.de \
/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).