From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 935A8C433F5 for ; Tue, 30 Nov 2021 18:29:37 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S245487AbhK3Scz (ORCPT ); Tue, 30 Nov 2021 13:32:55 -0500 Received: from foss.arm.com ([217.140.110.172]:45364 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233978AbhK3Scx (ORCPT ); Tue, 30 Nov 2021 13:32:53 -0500 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2516B106F; Tue, 30 Nov 2021 10:29:34 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.65.89]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 469C23F694; Tue, 30 Nov 2021 10:29:32 -0800 (PST) Date: Tue, 30 Nov 2021 18:29:27 +0000 From: Mark Rutland To: "Madhavan T. Venkataraman" Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org, nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com, catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() Message-ID: References: <8b861784d85a21a9bf08598938c11aff1b1249b9> <20211123193723.12112-1-madvenka@linux.microsoft.com> <20211123193723.12112-2-madvenka@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Nov 30, 2021 at 11:13:28AM -0600, Madhavan T. Venkataraman wrote: > On 11/30/21 9:05 AM, Mark Rutland wrote: > > On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" > >> > >> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe() > >> separately. There is no need to do that. Instead, call start_backtrace() > >> from within walk_stackframe(). In other words, walk_stackframe() is the only > >> unwind function a consumer needs to call. > >> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk, > >> NOKPROBE_SYMBOL(unwind_frame); > >> > >> static void notrace walk_stackframe(struct task_struct *tsk, > >> - struct stackframe *frame, > >> + unsigned long fp, unsigned long pc, > >> bool (*fn)(void *, unsigned long), void *data) > >> { > >> + struct stackframe frame; > >> + > >> + start_backtrace(&frame, fp, pc); > >> + > >> while (1) { > >> int ret; > >> > >> - if (!fn(data, frame->pc)) > >> + if (!fn(data, frame.pc)) > >> break; > >> - ret = unwind_frame(tsk, frame); > >> + ret = unwind_frame(tsk, &frame); > >> if (ret < 0) > >> break; > >> } > >> @@ -195,17 +196,19 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > >> void *cookie, struct task_struct *task, > >> struct pt_regs *regs) > >> { > >> - struct stackframe frame; > >> - > >> - if (regs) > >> - start_backtrace(&frame, regs->regs[29], regs->pc); > >> - else if (task == current) > >> - start_backtrace(&frame, > >> - (unsigned long)__builtin_frame_address(1), > >> - (unsigned long)__builtin_return_address(0)); > >> - else > >> - start_backtrace(&frame, thread_saved_fp(task), > >> - thread_saved_pc(task)); > >> - > >> - walk_stackframe(task, &frame, consume_entry, cookie); > >> + unsigned long fp, pc; > >> + > >> + if (regs) { > >> + fp = regs->regs[29]; > >> + pc = regs->pc; > >> + } else if (task == current) { > >> + /* Skip arch_stack_walk() in the stack trace. */ > >> + fp = (unsigned long)__builtin_frame_address(1); > >> + pc = (unsigned long)__builtin_return_address(0); > >> + } else { > >> + /* Caller guarantees that the task is not running. */ > >> + fp = thread_saved_fp(task); > >> + pc = thread_saved_pc(task); > >> + } > >> + walk_stackframe(task, fp, pc, consume_entry, cookie); > > > > I'd prefer to leave this as-is. The new and old structure are largely > > equivalent, so we haven't made this any simpler, but we have added more > > arguments to walk_stackframe(). > > > > This is just to simplify things when we eventually add arch_stack_walk_reliable(). > That is all. All of the unwinding is done by a single unwinding function and > there are two consumers of that unwinding function - arch_stack_walk() and > arch_stack_walk_reliable(). I understand the theory, but I don't think that moving the start_backtrace() call actually simplifies this in a meaningful way, and I think it'll make it harder for us to make more meaningful simplifications later on. As of patch 4 of this series, we'll have: | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | unsigned long fp, pc; | | if (regs) { | fp = regs->regs[29]; | pc = regs->pc; | } else if (task == current) { | /* Skip arch_stack_walk() in the stack trace. */ | fp = (unsigned long)__builtin_frame_address(1); | pc = (unsigned long)__builtin_return_address(0); | } else { | /* Caller guarantees that the task is not running. */ | fp = thread_saved_fp(task); | pc = thread_saved_pc(task); | } | walk_stackframe(task, fp, pc, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | unsigned long fp, pc; | | if (task == current) { | /* Skip arch_stack_walk_reliable() in the stack trace. */ | fp = (unsigned long)__builtin_frame_address(1); | pc = (unsigned long)__builtin_return_address(0); | } else { | /* Caller guarantees that the task is not running. */ | fp = thread_saved_fp(task); | pc = thread_saved_pc(task); | } | if (unwind(task, fp, pc, consume_fn, cookie)) | return 0; | return -EINVAL; | } Which I do not think is substantially simpler than the naive extrapolation from what we currently have, e.g. | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | struct stackframe frame; | | if (regs) { | unwind_init(&frame, regs->regs[29], regs->pc) | } else if (task == current) { | unwind_init(&frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } else { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task); | } | walk_stackframe(task, &frame, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | struct stackframe frame; | | if (task == current) { | unwind_init(&frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } else { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task); | } | if (unwind(task, &frame, consume_fn, cookie)) | return 0; | return -EINVAL; | } Further, I think we can factor this in a different way to reduce the duplication: | /* | * TODO: document requirements here | */ | static inline void unwind_init_from_current_regs(struct stackframe *frame, | struct pt_regs *regs) | { | unwind_init(frame, regs->regs[29], regs->pc); | } | | /* | * TODO: document requirements here | */ | static inline void unwind_init_from_blocked_task(struct stackframe *frame, | struct task_struct *tsk) | { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task)); | } | | /* | * TODO: document requirements here | * | * Note: this is always inlined, and we expect our caller to be a noinline | * function, such that this starts from our caller's caller. | */ | static __always_inline void unwind_init_from_caller(struct stackframe *frame) | { | unwind_init(frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } | | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | struct stackframe frame; | | if (regs) | unwind_init_current_regs(&frame, regs); | else if (task == current) | unwind_init_from_caller(&frame); | else | unwind_init_blocked_task(&frame, task); | | unwind(task, &frame, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | struct stackframe frame; | | if (task == current) | unwind_init_from_caller(&frame); | else | unwind_init_from_blocked_task(&frame, task); | | if (unwind(task, &frame, consume_fn, cookie)) | return 0; | return -EINVAL; | } ... which minimizes the duplication and allows us to add specialized initialization for each case if necessary, which I believe we will need in future to make unwinding across exception boundaries (such as when starting with regs) more useful. Thanks, Mark. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2A732C433EF for ; Tue, 30 Nov 2021 18:31:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=EEtIMxhtUNhoqEbkkDmdn1mM38UBlfM8pvSvsR/pkVQ=; b=bf03/gB6S0m6CE 9Wk4+K9BkzfiUpyFw6zwzcM5HmmnlBsSg7zKaIajg7VPIhP52EFsnNirpEy0wndDFKEsytwwLA+Ae CeVBZGb5RsHApVKMwV9ymRbZea+ranCrp6o4BlN0efOq5EJTmZ4nZaEVImQdXMGqKkfyeGcjyYgMw wc9T4Tv6THr0Zkbyuc6e+UAVy5i+m3QFnRBvW7uxFbtMV4pe232+hB4EC6S7uNHrbTM4wlf6gnzS6 7ggP1XR/KuF6fuQ8OBU4/W5KkHVbljh/MGFxxttodASaVDVS5YOJl1eUMKMrBPU6SnPVD+sH78I73 5V9cus4wxisXhZV3Li9g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ms7sq-006V45-43; Tue, 30 Nov 2021 18:29:40 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ms7sl-006V2n-JI for linux-arm-kernel@lists.infradead.org; Tue, 30 Nov 2021 18:29:37 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2516B106F; Tue, 30 Nov 2021 10:29:34 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.65.89]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 469C23F694; Tue, 30 Nov 2021 10:29:32 -0800 (PST) Date: Tue, 30 Nov 2021 18:29:27 +0000 From: Mark Rutland To: "Madhavan T. Venkataraman" Cc: broonie@kernel.org, jpoimboe@redhat.com, ardb@kernel.org, nobuta.keiya@fujitsu.com, sjitindarsingh@gmail.com, catalin.marinas@arm.com, will@kernel.org, jmorris@namei.org, linux-arm-kernel@lists.infradead.org, live-patching@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 1/5] arm64: Call stack_backtrace() only from within walk_stackframe() Message-ID: References: <8b861784d85a21a9bf08598938c11aff1b1249b9> <20211123193723.12112-1-madvenka@linux.microsoft.com> <20211123193723.12112-2-madvenka@linux.microsoft.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211130_102935_759016_F572E8E9 X-CRM114-Status: GOOD ( 26.03 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Nov 30, 2021 at 11:13:28AM -0600, Madhavan T. Venkataraman wrote: > On 11/30/21 9:05 AM, Mark Rutland wrote: > > On Tue, Nov 23, 2021 at 01:37:19PM -0600, madvenka@linux.microsoft.com wrote: > >> From: "Madhavan T. Venkataraman" > >> > >> Currently, arch_stack_walk() calls start_backtrace() and walk_stackframe() > >> separately. There is no need to do that. Instead, call start_backtrace() > >> from within walk_stackframe(). In other words, walk_stackframe() is the only > >> unwind function a consumer needs to call. > >> @@ -143,15 +140,19 @@ static int notrace unwind_frame(struct task_struct *tsk, > >> NOKPROBE_SYMBOL(unwind_frame); > >> > >> static void notrace walk_stackframe(struct task_struct *tsk, > >> - struct stackframe *frame, > >> + unsigned long fp, unsigned long pc, > >> bool (*fn)(void *, unsigned long), void *data) > >> { > >> + struct stackframe frame; > >> + > >> + start_backtrace(&frame, fp, pc); > >> + > >> while (1) { > >> int ret; > >> > >> - if (!fn(data, frame->pc)) > >> + if (!fn(data, frame.pc)) > >> break; > >> - ret = unwind_frame(tsk, frame); > >> + ret = unwind_frame(tsk, &frame); > >> if (ret < 0) > >> break; > >> } > >> @@ -195,17 +196,19 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > >> void *cookie, struct task_struct *task, > >> struct pt_regs *regs) > >> { > >> - struct stackframe frame; > >> - > >> - if (regs) > >> - start_backtrace(&frame, regs->regs[29], regs->pc); > >> - else if (task == current) > >> - start_backtrace(&frame, > >> - (unsigned long)__builtin_frame_address(1), > >> - (unsigned long)__builtin_return_address(0)); > >> - else > >> - start_backtrace(&frame, thread_saved_fp(task), > >> - thread_saved_pc(task)); > >> - > >> - walk_stackframe(task, &frame, consume_entry, cookie); > >> + unsigned long fp, pc; > >> + > >> + if (regs) { > >> + fp = regs->regs[29]; > >> + pc = regs->pc; > >> + } else if (task == current) { > >> + /* Skip arch_stack_walk() in the stack trace. */ > >> + fp = (unsigned long)__builtin_frame_address(1); > >> + pc = (unsigned long)__builtin_return_address(0); > >> + } else { > >> + /* Caller guarantees that the task is not running. */ > >> + fp = thread_saved_fp(task); > >> + pc = thread_saved_pc(task); > >> + } > >> + walk_stackframe(task, fp, pc, consume_entry, cookie); > > > > I'd prefer to leave this as-is. The new and old structure are largely > > equivalent, so we haven't made this any simpler, but we have added more > > arguments to walk_stackframe(). > > > > This is just to simplify things when we eventually add arch_stack_walk_reliable(). > That is all. All of the unwinding is done by a single unwinding function and > there are two consumers of that unwinding function - arch_stack_walk() and > arch_stack_walk_reliable(). I understand the theory, but I don't think that moving the start_backtrace() call actually simplifies this in a meaningful way, and I think it'll make it harder for us to make more meaningful simplifications later on. As of patch 4 of this series, we'll have: | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | unsigned long fp, pc; | | if (regs) { | fp = regs->regs[29]; | pc = regs->pc; | } else if (task == current) { | /* Skip arch_stack_walk() in the stack trace. */ | fp = (unsigned long)__builtin_frame_address(1); | pc = (unsigned long)__builtin_return_address(0); | } else { | /* Caller guarantees that the task is not running. */ | fp = thread_saved_fp(task); | pc = thread_saved_pc(task); | } | walk_stackframe(task, fp, pc, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | unsigned long fp, pc; | | if (task == current) { | /* Skip arch_stack_walk_reliable() in the stack trace. */ | fp = (unsigned long)__builtin_frame_address(1); | pc = (unsigned long)__builtin_return_address(0); | } else { | /* Caller guarantees that the task is not running. */ | fp = thread_saved_fp(task); | pc = thread_saved_pc(task); | } | if (unwind(task, fp, pc, consume_fn, cookie)) | return 0; | return -EINVAL; | } Which I do not think is substantially simpler than the naive extrapolation from what we currently have, e.g. | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | struct stackframe frame; | | if (regs) { | unwind_init(&frame, regs->regs[29], regs->pc) | } else if (task == current) { | unwind_init(&frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } else { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task); | } | walk_stackframe(task, &frame, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | struct stackframe frame; | | if (task == current) { | unwind_init(&frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } else { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task); | } | if (unwind(task, &frame, consume_fn, cookie)) | return 0; | return -EINVAL; | } Further, I think we can factor this in a different way to reduce the duplication: | /* | * TODO: document requirements here | */ | static inline void unwind_init_from_current_regs(struct stackframe *frame, | struct pt_regs *regs) | { | unwind_init(frame, regs->regs[29], regs->pc); | } | | /* | * TODO: document requirements here | */ | static inline void unwind_init_from_blocked_task(struct stackframe *frame, | struct task_struct *tsk) | { | unwind_init(&frame, thread_saved_fp(task), | thread_saved_pc(task)); | } | | /* | * TODO: document requirements here | * | * Note: this is always inlined, and we expect our caller to be a noinline | * function, such that this starts from our caller's caller. | */ | static __always_inline void unwind_init_from_caller(struct stackframe *frame) | { | unwind_init(frame, __builtin_frame_address(1), | __builtin_return_address(0)); | } | | noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, | void *cookie, struct task_struct *task, | struct pt_regs *regs) | { | struct stackframe frame; | | if (regs) | unwind_init_current_regs(&frame, regs); | else if (task == current) | unwind_init_from_caller(&frame); | else | unwind_init_blocked_task(&frame, task); | | unwind(task, &frame, consume_entry, cookie); | } | | noinline int notrace arch_stack_walk_reliable(stack_trace_consume_fn consume_fn, | void *cookie, | struct task_struct *task) | { | struct stackframe frame; | | if (task == current) | unwind_init_from_caller(&frame); | else | unwind_init_from_blocked_task(&frame, task); | | if (unwind(task, &frame, consume_fn, cookie)) | return 0; | return -EINVAL; | } ... which minimizes the duplication and allows us to add specialized initialization for each case if necessary, which I believe we will need in future to make unwinding across exception boundaries (such as when starting with regs) more useful. Thanks, Mark. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel