Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: takahiro.akashi@linaro.org (AKASHI Takahiro)
To: linux-arm-kernel@lists.infradead.org
Subject: [RFC 2/3] arm64: refactor save_stack_trace()
Date: Wed, 15 Jul 2015 20:41:34 +0900
Message-ID: <55A646EE.6030402@linaro.org> (raw)
In-Reply-To: <20150714225105.6c1e4f15@gandalf.local.home>

Steve,

On 07/15/2015 11:51 AM, Steven Rostedt wrote:
> On Wed, 15 Jul 2015 09:20:42 +0900
> AKASHI Takahiro <takahiro.akashi@linaro.org> wrote:
>
>> On 07/14/2015 10:31 PM, Steven Rostedt wrote:
>>> On Tue, 14 Jul 2015 21:47:10 +0900
>>> Jungseok Lee <jungseoklee85@gmail.com> wrote:
>>>
>>>> Is the below example an unexpected result?
>>>> Entry 17 and 18 are ftrace_call and ftrace_ops_no_ops, respectively.
>>
>> [snip]
>>
>>> Note, function tracing does not disable interrupts. This looks to be
>>> that an interrupt came in while __aloc_skb() was being traced.
>>
>> Yeah, I think so, too. But if my insight is correct, it's not __alloc_skb()
>> but one of functions that it calls. As I said in the commit log message
>> of patch[1/3], the exact traced function will not be listed by

not patch[1/3], but patch[3/3]

>> save_stack_trace() because we don't create a stack frame at mcount().
>> I think this is a flaw in the current implementation (on x86).
>>
>> what do you think, Steve?
>>
>
> mcount (well ftrace_call actually) does indeed create a stack frame for
> itself *and* for what called it. At least on x86_64. See mcount_64.S.
>
> With -pg -mfentry, it creates a stack frame. Without -mfentry, mcount
> is called after the current function's frame is made so we don't need
> to do much.

Thank you for the explanation. But what I don't really understand here
is why we need to add the "current function" to the stack dump list
returned by save_stack_trace():

In check_stack(),
 >        /*
 >         * Add the passed in ip from the function tracer.
 >         * Searching for this on the stack will skip over
 >         * most of the overhead from the stack tracer itself.
 >         */
 >        stack_dump_trace[0] = ip;
 >        max_stack_trace.nr_entries++;

I think that "ip" here is the "return address for func" in your
ascii art, and it should be already in the list if a frame is made
by mcount (or func_call).

In fact, stack tracer on arm64 works OK even without the patch[3/3]
if the code quoted above is commented out.
Even on x86, the code is conditional and not activated if the kernel
is compiled without -mfentry before the following commit:
     commit 4df297129f62 ("tracing: Remove most or all of stack tracer stack size from stack_max_size")

So what do I misunderstand here?

Thanks,
-Takahiro AKASHI

> Here's what the -mfentry version does:
>
> 	pushq %rbp
> 	pushq 8*2(%rsp)  /* this is the parent pointer */
> 	pushq %rbp
> 	movq %rsp, %rbp
> 	pushq 8*3(%rsp)   /* Return address to ftrace_call */
> 	pushq %rbp
> 	movq %rsp, %rbp
>
>
> Thus the stack looks like this:
>
>                                           <---+
> 	|                              |     |
> 	+------------------------------+     |
> 	| return address for func      |     |
> 	| return address for func_call |     |
> 	| original %rbp                |     |
> 	+------------------------------+     |
> 	| return address for func      |     |
> 	| ptr to parent frame (%rbp)   | ----+
>   	+------------------------------| <-----+
> 	| return address for func_call |       |
>          | ptr to next frame (%rbp)     | ------+
> 	+------------------------------+ <---+
>                                               |
>                                               |
>   Current %rbp points to func_call frame -----+
>
>   The first box isn't used as a frame, but is used by ftrace_call to save
>   information to restore everything properly.
>
> Thus, __alloc_skb() is what is currently being traced.
>
>
> -- Steve
>

  reply index

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-13  5:29 [RFC 0/3] arm64: ftrace: fix incorrect output from stack tracer takahiro.akashi
2015-07-13  5:29 ` [RFC 1/3] ftrace: adjust a function's pc to search for in check_stack() for arm64 takahiro.akashi
2015-07-13 15:24   ` rostedt
2015-07-15  0:22     ` takahiro.akashi
2015-07-13  5:29 ` [RFC 2/3] arm64: refactor save_stack_trace() takahiro.akashi
2015-07-14 12:47   ` jungseoklee85
2015-07-14 13:31     ` rostedt
2015-07-15  0:20       ` takahiro.akashi
2015-07-15  2:51         ` rostedt
2015-07-15 11:41           ` takahiro.akashi [this message]
2015-07-15 14:55             ` rostedt
2015-07-15 16:13               ` rostedt
2015-07-16  0:27                 ` takahiro.akashi
2015-07-16  1:08                   ` takahiro.akashi
2015-07-16  1:38                     ` rostedt
2015-07-17 10:46                       ` will.deacon
2015-07-16 13:29                     ` jungseoklee85
2015-07-16 13:54                       ` jungseoklee85
2015-07-16 14:24                       ` rostedt
2015-07-16 15:01                         ` jungseoklee85
2015-07-16 15:31                           ` rostedt
2015-07-16 15:52                             ` jungseoklee85
2015-07-16 20:22                               ` rostedt
2015-07-17  2:49                                 ` takahiro.akashi
2015-07-17  3:21                                   ` rostedt
2015-07-16 16:16                             ` rostedt
2015-07-17 12:40                               ` mark.rutland
2015-07-17 12:51                                 ` rostedt
2015-07-17 13:00                                 ` rostedt
2015-07-17 14:28                                   ` jungseoklee85
2015-07-17 14:41                                     ` rostedt
2015-07-17 14:59                                       ` jungseoklee85
2015-07-17 15:34                                         ` jungseoklee85
2015-07-17 16:01                                           ` rostedt
2015-07-20 16:20                                           ` will.deacon
2015-07-20 23:53                                             ` takahiro.akashi
2015-07-21 10:26                                               ` takahiro.akashi
2015-07-21 14:34                                                 ` jungseoklee85
2015-08-03  9:09                                             ` will.deacon
2015-08-03 14:01                                               ` rostedt
2015-08-03 14:04                                                 ` will.deacon
2015-08-03 16:30                                               ` jungseoklee85
2015-08-03 16:57                                                 ` rostedt
2015-08-03 17:22                                                   ` jungseoklee85
2015-08-03 17:32                                                     ` rostedt
2015-08-04  7:41                                                       ` takahiro.akashi
2015-07-17  2:04                       ` takahiro.akashi
2015-07-17 14:38                         ` jungseoklee85
2015-07-16 14:28                     ` mark.rutland
2015-07-16 14:34                       ` rostedt
2015-07-17  2:09                         ` takahiro.akashi
2015-07-13  5:29 ` [RFC 3/3] arm64: ftrace: mcount() should not create a stack frame takahiro.akashi
2015-07-13 15:01 ` [RFC 0/3] arm64: ftrace: fix incorrect output from stack tracer jungseoklee85

Reply instructions:

You may reply publically 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=55A646EE.6030402@linaro.org \
    --to=takahiro.akashi@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.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

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git