Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753061AbbKKW5J (ORCPT ); Wed, 11 Nov 2015 17:57:09 -0500 Received: from mail-pa0-f52.google.com ([209.85.220.52]:33508 "EHLO mail-pa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752910AbbKKW5F convert rfc822-to-8bit (ORCPT ); Wed, 11 Nov 2015 17:57:05 -0500 Subject: Re: [PATCH v5 5/6] arm64: ftrace: add arch-specific stack tracer Mime-Version: 1.0 (Apple Message framework v1283) Content-Type: text/plain; charset=windows-1252 From: Jungseok Lee In-Reply-To: <5642CC2E.7010405@linaro.org> Date: Thu, 12 Nov 2015 07:56:59 +0900 Cc: catalin.marinas@arm.com, will.deacon@arm.com, rostedt@goodmis.org, broonie@kernel.org, david.griego@linaro.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8BIT Message-Id: References: <1446792285-1154-1-git-send-email-takahiro.akashi@linaro.org> <1446792285-1154-6-git-send-email-takahiro.akashi@linaro.org> <823AA540-A025-4D44-852A-10F4A99A5760@gmail.com> <5642CC2E.7010405@linaro.org> To: AKASHI Takahiro X-Mailer: Apple Mail (2.1283) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10279 Lines: 308 On Nov 11, 2015, at 2:03 PM, AKASHI Takahiro wrote: > Jungseok, > > On 11/10/2015 11:05 PM, Jungseok Lee wrote: >> On Nov 6, 2015, at 3:44 PM, AKASHI Takahiro wrote: >> >> Hi Akashi, >> >>> Stack tracer on arm64, check_stack(), is uniqeue in the following >>> points: >>> * analyze a function prologue of a traced function to estimate a more >>> accurate stack pointer value, replacing naive ' + 0x10.' >>> * use walk_stackframe(), instead of slurping stack contents as orignal >>> check_stack() does, to identify a stack frame and a stack index (height) >>> for every callsite. >>> >>> Regarding a function prologue analyzer, there is no guarantee that we can >>> handle all the possible patterns of function prologue as gcc does not use >>> any fixed templates to generate them. 'Instruction scheduling' is another >>> issue here. >>> Nevertheless, the current version will surely cover almost all the cases >>> in the kernel image and give us useful information on stack pointers. >>> >>> So this patch utilizes a function prologue only for stack tracer, and >>> does not affect the behaviors of existing stacktrace functions. >>> >>> Signed-off-by: AKASHI Takahiro >>> --- >>> arch/arm64/include/asm/stacktrace.h | 4 + >>> arch/arm64/kernel/ftrace.c | 64 ++++++++++++ >>> arch/arm64/kernel/stacktrace.c | 185 ++++++++++++++++++++++++++++++++++- >>> 3 files changed, 250 insertions(+), 3 deletions(-) >>> >>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h >>> index 7318f6d..47b4832 100644 >>> --- a/arch/arm64/include/asm/stacktrace.h >>> +++ b/arch/arm64/include/asm/stacktrace.h >>> @@ -25,5 +25,9 @@ struct stackframe { >>> extern int unwind_frame(struct stackframe *frame); >>> extern void walk_stackframe(struct stackframe *frame, >>> int (*fn)(struct stackframe *, void *), void *data); >>> +#ifdef CONFIG_STACK_TRACER >>> +struct stack_trace; >>> +extern void save_stack_trace_sp(struct stack_trace *trace, unsigned long *sp); >>> +#endif >>> >>> #endif /* __ASM_STACKTRACE_H */ >>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c >>> index 314f82d..46bfe37 100644 >>> --- a/arch/arm64/kernel/ftrace.c >>> +++ b/arch/arm64/kernel/ftrace.c >>> @@ -9,6 +9,7 @@ >>> * published by the Free Software Foundation. >>> */ >>> >>> +#include >>> #include >>> #include >>> #include >>> @@ -16,6 +17,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #ifdef CONFIG_DYNAMIC_FTRACE >>> /* >>> @@ -173,3 +175,65 @@ int ftrace_disable_ftrace_graph_caller(void) >>> } >>> #endif /* CONFIG_DYNAMIC_FTRACE */ >>> #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ >>> + >>> +#ifdef CONFIG_STACK_TRACER >>> +static unsigned long stack_trace_sp[STACK_TRACE_ENTRIES]; >>> +static unsigned long raw_stack_trace_max_size; >>> + >>> +void check_stack(unsigned long ip, unsigned long *stack) >>> +{ >>> + unsigned long this_size, flags; >>> + unsigned long top; >>> + int i, j; >>> + >>> + this_size = ((unsigned long)stack) & (THREAD_SIZE-1); >>> + this_size = THREAD_SIZE - this_size; >>> + >>> + if (this_size <= raw_stack_trace_max_size) >>> + return; >>> + >>> + /* we do not handle an interrupt stack yet */ >>> + if (!object_is_on_stack(stack)) >>> + return; >>> + >>> + local_irq_save(flags); >>> + arch_spin_lock(&max_stack_lock); >>> + >>> + /* check again */ >>> + if (this_size <= raw_stack_trace_max_size) >>> + goto out; >>> + >>> + /* find out stack frames */ >>> + stack_trace_max.nr_entries = 0; >>> + stack_trace_max.skip = 0; >>> + save_stack_trace_sp(&stack_trace_max, stack_trace_sp); >>> + stack_trace_max.nr_entries--; /* for the last entry ('-1') */ >>> + >>> + /* calculate a stack index for each function */ >>> + top = ((unsigned long)stack & ~(THREAD_SIZE-1)) + THREAD_SIZE; >>> + for (i = 0; i < stack_trace_max.nr_entries; i++) >>> + stack_trace_index[i] = top - stack_trace_sp[i]; >>> + raw_stack_trace_max_size = this_size; >>> + >>> + /* Skip over the overhead of the stack tracer itself */ >>> + for (i = 0; i < stack_trace_max.nr_entries; i++) >>> + if (stack_trace_max.entries[i] == ip) >>> + break; >>> + >>> + stack_trace_max.nr_entries -= i; >>> + for (j = 0; j < stack_trace_max.nr_entries; j++) { >>> + stack_trace_index[j] = stack_trace_index[j + i]; >>> + stack_trace_max.entries[j] = stack_trace_max.entries[j + i]; >>> + } >>> + stack_trace_max_size = stack_trace_index[0]; >>> + >>> + if (task_stack_end_corrupted(current)) { >>> + WARN(1, "task stack is corrupted.\n"); >>> + stack_trace_print(); >>> + } >>> + >>> + out: >>> + arch_spin_unlock(&max_stack_lock); >>> + local_irq_restore(flags); >>> +} >>> +#endif /* CONFIG_STACK_TRACER */ >>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c >>> index 5fd3477..4ee052a 100644 >>> --- a/arch/arm64/kernel/stacktrace.c >>> +++ b/arch/arm64/kernel/stacktrace.c >>> @@ -23,6 +23,144 @@ >>> >>> #include >>> >>> +#ifdef CONFIG_STACK_TRACER >>> +/* >>> + * This function parses a function prologue of a traced function and >>> + * determines its stack size. >>> + * A return value indicates a location of @pc in a function prologue. >>> + * @return value: >>> + * >>> + * 1: >>> + * sub sp, sp, #XX sub sp, sp, #XX >>> + * 2: >>> + * stp x29, x30, [sp, #YY] stp x29, x30, [sp, #--ZZ]! >>> + * 3: >>> + * add x29, sp, #YY mov x29, sp >>> + * 0: >>> + * >>> + * >>> + * 1: >>> + * stp x29, x30, [sp, #-XX]! >>> + * 3: >>> + * mov x29, sp >>> + * 0: >>> + * >>> + * @size: sp offset from calller's sp (XX or XX + ZZ) >>> + * @size2: fp offset from new sp (YY or 0) >>> + */ >>> +static int analyze_function_prologue(unsigned long pc, >>> + unsigned long *size, unsigned long *size2) >>> +{ >>> + unsigned long offset; >>> + u32 *addr, insn; >>> + int pos = -1; >>> + enum aarch64_insn_register src, dst, reg1, reg2, base; >>> + int imm; >>> + enum aarch64_insn_variant variant; >>> + enum aarch64_insn_adsb_type adsb_type; >>> + enum aarch64_insn_ldst_type ldst_type; >>> + >>> + *size = *size2 = 0; >>> + >>> + if (!pc) >>> + goto out; >>> + >>> + if (unlikely(!kallsyms_lookup_size_offset(pc, NULL, &offset))) >>> + goto out; >>> + >>> + addr = (u32 *)(pc - offset); >>> +#ifdef CONFIG_DYNAMIC_FTRACE >>> + if (addr == (u32 *)ftrace_call) >>> + addr = (u32 *)ftrace_caller; >>> +#ifdef CONFIG_FUNCTION_GRAPH_TRACER >>> + else if (addr == (u32 *)ftrace_graph_caller) >>> +#ifdef CONFIG_DYNAMIC_FTRACE >>> + addr = (u32 *)ftrace_caller; >>> +#else >>> + addr = (u32 *)_mcount; >>> +#endif >>> +#endif >>> +#endif >>> + >>> + insn = *addr; >>> + pos = 1; >>> + >>> + /* analyze a function prologue */ >>> + while ((unsigned long)addr < pc) { >>> + if (aarch64_insn_is_branch_imm(insn) || >>> + aarch64_insn_is_br(insn) || >>> + aarch64_insn_is_blr(insn) || >>> + aarch64_insn_is_ret(insn) || >>> + aarch64_insn_is_eret(insn)) >>> + /* exiting a basic block */ >>> + goto out; >>> + >>> + if (aarch64_insn_decode_add_sub_imm(insn, &dst, &src, >>> + &imm, &variant, &adsb_type)) { >>> + if ((adsb_type == AARCH64_INSN_ADSB_SUB) && >>> + (dst == AARCH64_INSN_REG_SP) && >>> + (src == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * Starting the following sequence: >>> + * sub sp, sp, #xx >>> + * stp x29, x30, [sp, #yy] >>> + * add x29, sp, #yy >>> + */ >>> + WARN_ON(pos != 1); >>> + pos = 2; >>> + *size += imm; >>> + } else if ((adsb_type == AARCH64_INSN_ADSB_ADD) && >>> + (dst == AARCH64_INSN_REG_29) && >>> + (src == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * add x29, sp, #yy >>> + * or >>> + * mov x29, sp >>> + */ >>> + WARN_ON(pos != 3); >>> + pos = 0; >>> + *size2 = imm; >>> + >>> + break; >>> + } >>> + } else if (aarch64_insn_decode_load_store_pair(insn, >>> + ®1, ®2, &base, &imm, >>> + &variant, &ldst_type)) { >>> + if ((ldst_type == >>> + AARCH64_INSN_LDST_STORE_PAIR_PRE_INDEX) && >>> + (reg1 == AARCH64_INSN_REG_29) && >>> + (reg2 == AARCH64_INSN_REG_30) && >>> + (base == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * Starting the following sequence: >>> + * stp x29, x30, [sp, #-xx]! >>> + * mov x29, sp >>> + */ >>> + WARN_ON(!((pos == 1) || (pos == 2))); >>> + pos = 3; >>> + *size += -imm; >>> + } else if ((ldst_type == >>> + AARCH64_INSN_LDST_STORE_PAIR) && >>> + (reg1 == AARCH64_INSN_REG_29) && >>> + (reg2 == AARCH64_INSN_REG_30) && >>> + (base == AARCH64_INSN_REG_SP)) { >>> + /* >>> + * stp x29, x30, [sp, #yy] >>> + */ >>> + WARN_ON(pos != 2); >>> + pos = 3; >>> + } >>> + } >>> + >>> + addr++; >>> + insn = *addr; >>> + } >>> + >>> +out: >>> + return pos; >>> +} >>> +#endif >>> + >> >> A small instruction decoder in software! >> >> From a high level point of view, it makes sense to deal with only three main >> cases. Since it is tough to deal with all function prologues in perspective >> of computational overhead and implementation complexity, the above change looks >> like a pretty good tradeoff. This is what I can say now. I'm also interested >> in feedbacks from other folks. > > I really appreciate your reviews and comments on the whole series of my patches > (#2 to #5). > May I add Reviewed-by (and Tested-by?) with your name if you like? Sure. Tested-by is also okay. Regarding patch6, the option would be useful if this analyzer is accepted. So, like this patch5, I'm waiting for other folk's response. How about adding your document [1] to this series? I think it explains all backgrounds, such as motivation, issue, approach, and TODO, on this series. [1] http://www.spinics.net/lists/arm-kernel/msg444300.html Best Regards Jungseok Lee-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/