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 55228C2BA18 for ; Tue, 18 Jun 2024 17:43:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=jOykAzfvwcaKHm5YVVqIvDQlDuRsTl9xJ1V2WnHp0Qk=; b=D7jMWxS2GS8PH6WDPdued+9pbc kN0rgcR1E2dbaOwFyni9fKrRTywkjQLzOHUTlkq7PRjOeuMQbAzComFBHoEFip+DRN9Ry9ceJm+Kz RWvCxM9BFUu8M/71GX4zDj77/6DkNKvbxt4QIajDSOpWUqEdAOO6WgkKFwqx5uTVQDBtrjf5v/Zra BlMs7hL5iTN/W1MqsxkgxeISUSCBXTbITEjjoJPyLxVjDoKDGGCLkkpVyf8arl2S63IKtuPgTENa0 bRRTJRKcQHa5aAIYaoXcVrGTe8RjRJRCkN885GZqP0QashIHS3RGCMi5IZaX1h/fEZ60tHhw75r52 vSQ18BeQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJcrL-0000000G4jw-2poD; Tue, 18 Jun 2024 17:43:07 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sJcrI-0000000G4j9-40U5 for linux-arm-kernel@lists.infradead.org; Tue, 18 Jun 2024 17:43:06 +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 5F409DA7; Tue, 18 Jun 2024 10:43:26 -0700 (PDT) Received: from J2N7QTR9R3 (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 31B923F64C; Tue, 18 Jun 2024 10:43:00 -0700 (PDT) Date: Tue, 18 Jun 2024 18:42:54 +0100 From: Mark Rutland To: Puranjay Mohan , rostedt@goodmis.org, Catalin Marinas , Will Deacon Cc: "Madhavan T. Venkataraman" , Kalesh Singh , chenqiwu , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, puranjay12@gmail.com Subject: Re: [PATCH] arm64: stacktrace: fix the usage of ftrace_graph_ret_addr() Message-ID: References: <20240618162342.28275-1-puranjay@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240618162342.28275-1-puranjay@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240618_104305_099671_677EA315 X-CRM114-Status: GOOD ( 26.75 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Jun 18, 2024 at 04:23:42PM +0000, Puranjay Mohan wrote: > ftrace_graph_ret_addr() takes an 'idx' integer pointer that is used to > optimize the stack unwinding process. arm64 currently passes `NULL` for > this parameter which stops it from utilizing these optimizations. Yep, we removed that back in commit: c6d3cd32fd0064af ("arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR") ... because at the time, ftrace_graph_ret_addr() didn't use the 'idx' argument when HAVE_FUNCTION_GRAPH_RET_ADDR_PTR was set, and I assumed that was intentional. AFAICT this is a fix (or preparatory patch) for commit: 29c1c24a2707a579 ("function_graph: Fix up ftrace_graph_ret_addr())" ... which is queued up in linux-next and makes ftrace_graph_ret_addr() use 'idx' when HAVE_FUNCTION_GRAPH_RET_ADDR_PTR is set. Prior to that commit passing (or not passing) 'idx' has no effect whatsoever, and after that commit not passing 'idx' is a bug. > Further, the current code for ftrace_graph_ret_addr() will just return > the passed in return address if it is NULL which will break this usage. > > Pass a valid integer pointer to ftrace_graph_ret_addr() similar to > x86_64's stack unwinder. > > Signed-off-by: Puranjay Mohan > --- > arch/arm64/kernel/stacktrace.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index 6b3258860377..2729faaee4b4 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -25,6 +25,7 @@ > * > * @common: Common unwind state. > * @task: The task being unwound. > + * @graph_idx: Used by ftrace_graph_ret_addr() for optimized stack unwinding. > * @kr_cur: When KRETPROBES is selected, holds the kretprobe instance > * associated with the most recently encountered replacement lr > * value. > @@ -32,6 +33,7 @@ > struct kunwind_state { > struct unwind_state common; > struct task_struct *task; > + int graph_idx; Minor nit, but could we make that: #ifdef CONFIG_FUNCTION_GRAPH_TRACER int graph_idx; #endif Regardless, this looks good to me, and I've tested it with a few stacktrace scenarios including the example from commit: c6d3cd32fd0064af ("arm64: ftrace: use HAVE_FUNCTION_GRAPH_RET_ADDR_PTR") Reviewed-by: Mark Rutland Tested-by: Mark Rutland Steve, what's your plan for merging the ftrace bits, and how should we stage this relative to that? e.g. would it make sense for this to go through the ftrace tree along with those changes so that this doesn't end up transiently broken during the merge window? Catalin, Will, do you have any preference? Mark. > #ifdef CONFIG_KRETPROBES > struct llist_node *kr_cur; > #endif > @@ -106,7 +108,7 @@ kunwind_recover_return_address(struct kunwind_state *state) > if (state->task->ret_stack && > (state->common.pc == (unsigned long)return_to_handler)) { > unsigned long orig_pc; > - orig_pc = ftrace_graph_ret_addr(state->task, NULL, > + orig_pc = ftrace_graph_ret_addr(state->task, &state->graph_idx, > state->common.pc, > (void *)state->common.fp); > if (WARN_ON_ONCE(state->common.pc == orig_pc)) > -- > 2.40.1 >