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 2E48BC433EF for ; Tue, 15 Feb 2022 13:08:06 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238080AbiBONIO (ORCPT ); Tue, 15 Feb 2022 08:08:14 -0500 Received: from mxb-00190b01.gslb.pphosted.com ([23.128.96.19]:50738 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231377AbiBONIM (ORCPT ); Tue, 15 Feb 2022 08:08:12 -0500 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 2178AB16EC; Tue, 15 Feb 2022 05:08:02 -0800 (PST) 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 DD2631480; Tue, 15 Feb 2022 05:08:01 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.89.173]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 099533F718; Tue, 15 Feb 2022 05:07:59 -0800 (PST) Date: Tue, 15 Feb 2022 13:07:56 +0000 From: Mark Rutland To: madvenka@linux.microsoft.com 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 v13 04/11] arm64: Split unwind_init() Message-ID: References: <95691cae4f4504f33d0fc9075541b1e7deefe96f> <20220117145608.6781-1-madvenka@linux.microsoft.com> <20220117145608.6781-5-madvenka@linux.microsoft.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220117145608.6781-5-madvenka@linux.microsoft.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Madhavan, The diff itself largely looks good, but we need to actually write the comments. Can you pleaes pick up the wording I've written below for those? That and renaming `unwind_init_from_current` to `unwind_init_from_caller`. With those I think this is good, but I'd like to see the updated version before I provide Acked-by or Reviewed-by tags -- hopefully that's just a formality! :) On Mon, Jan 17, 2022 at 08:56:01AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" > > unwind_init() is currently a single function that initializes all of the > unwind state. Split it into the following functions and call them > appropriately: > > - unwind_init_from_regs() - initialize from regs passed by caller. > > - unwind_init_from_current() - initialize for the current task > from the caller of arch_stack_walk(). > > - unwind_init_from_task() - initialize from the saved state of a > task other than the current task. In this case, the other > task must not be running. > > This is done for two reasons: > > - the different ways of initializing are clear > > - specialized code can be added to each initializer in the future. > > Signed-off-by: Madhavan T. Venkataraman > --- > arch/arm64/kernel/stacktrace.c | 54 +++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index a1a7ff93b84f..b2b568e5deba 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -33,11 +33,8 @@ > */ > > > -static void unwind_init(struct unwind_state *state, unsigned long fp, > - unsigned long pc) > +static void unwind_init_common(struct unwind_state *state) > { > - state->fp = fp; > - state->pc = pc; > #ifdef CONFIG_KRETPROBES > state->kr_cur = NULL; > #endif > @@ -56,6 +53,46 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, > state->prev_type = STACK_TYPE_UNKNOWN; > } > > +/* > + * TODO: document requirements here. > + */ Please make this: /* * Start an unwind from a pt_regs. * * The unwind will begin at the PC within the regs. * * The regs must be on a stack currently owned by the calling task. */ > +static inline void unwind_init_from_regs(struct unwind_state *state, > + struct pt_regs *regs) > +{ In future we could add: WARN_ON_ONCE(!on_accessible_stack(current, regs, sizeof(*regs), NULL)); ... to validate the requirements, but I'm happy to lave that for a future patch so this patch can be a pure refactoring. > + unwind_init_common(state); > + > + state->fp = regs->regs[29]; > + state->pc = regs->pc; > +} > + > +/* > + * 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. > + */ Please make this: /* * Start an unwind from a caller. * * The unwind will begin at the caller of whichever function this is inlined * into. * * The function which invokes this must be noinline. */ > +static __always_inline void unwind_init_from_current(struct unwind_state *state) Can we please rename s/current/caller/ here? That way it's clear *where* in current we're unwinding from, and the fact that it's current is implicit but obvious. > +{ Similarly to unwind_init_from_regs(), in a future patch we could add: WARN_ON_ONCE(task == current); ... but for now we can omit that so this patch can be a pure refactoring. > + unwind_init_common(state); > + > + state->fp = (unsigned long)__builtin_frame_address(1); > + state->pc = (unsigned long)__builtin_return_address(0); > +} > + > +/* > + * TODO: document requirements here. > + * > + * The caller guarantees that the task is not running. > + */ Please make this: /* * Start an unwind from a blocked task. * * The unwind will begin at the blocked tasks saved PC (i.e. the caller of * cpu_switch_to()). * * The caller should ensure the task is blocked in cpu_switch_to() for the * duration of the unwind, or the unwind will be bogus. It is never valid to * call this for the current task. */ Thanks, Mark. > +static inline void unwind_init_from_task(struct unwind_state *state, > + struct task_struct *task) > +{ > + unwind_init_common(state); > + > + state->fp = thread_saved_fp(task); > + state->pc = thread_saved_pc(task); > +} > + > /* > * Unwind from one frame record (A) to the next frame record (B). > * > @@ -195,14 +232,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > struct unwind_state state; > > if (regs) > - unwind_init(&state, regs->regs[29], regs->pc); > + unwind_init_from_regs(&state, regs); > else if (task == current) > - unwind_init(&state, > - (unsigned long)__builtin_frame_address(1), > - (unsigned long)__builtin_return_address(0)); > + unwind_init_from_current(&state); > else > - unwind_init(&state, thread_saved_fp(task), > - thread_saved_pc(task)); > + unwind_init_from_task(&state, task); > > unwind(task, &state, consume_entry, cookie); > } > -- > 2.25.1 > 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 E7A1DC433F5 for ; Tue, 15 Feb 2022 13:09: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: 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=7giBkQ8J2gloHbVA9fg6ErBnfOu58RqfE1G/+G3daVU=; b=VSQ1UKDe4NKWd0 np+Vw27HO/qi+cdg3O9qtYxnJnFHo2rMkwHO+pp+W4N5uSvraFo1yHAMlOfE5HSWFTmKe9Vxg+DBg 32s4V2mOIN1XzwPgU+fRVWkFmq8LBxN0JZPPZRRJyqTwGVX3rkG5s8CmmZNgrQUMvUjTHOwtkTU7u u2AaD1dhNelgsq7qoS6jCiOxOI/P/AvPnwQ+2+Z/Hd6TueQMgiQTtWhDaIc40Pae2fr8VjUGB2M00 6BPnhLpZwleOkkzfZsTGFy1/WIp9vDd4su3MUgMbFWc/1dhJ4w9MZbA6T1bDJa4/02ia1jQ+ARC7T iytqmmI0rzpIlQcbSONQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nJxYx-002mM9-0p; Tue, 15 Feb 2022 13:08:11 +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 1nJxYp-002mJq-8C for linux-arm-kernel@lists.infradead.org; Tue, 15 Feb 2022 13:08:09 +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 DD2631480; Tue, 15 Feb 2022 05:08:01 -0800 (PST) Received: from FVFF77S0Q05N (unknown [10.57.89.173]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 099533F718; Tue, 15 Feb 2022 05:07:59 -0800 (PST) Date: Tue, 15 Feb 2022 13:07:56 +0000 From: Mark Rutland To: madvenka@linux.microsoft.com 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 v13 04/11] arm64: Split unwind_init() Message-ID: References: <95691cae4f4504f33d0fc9075541b1e7deefe96f> <20220117145608.6781-1-madvenka@linux.microsoft.com> <20220117145608.6781-5-madvenka@linux.microsoft.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20220117145608.6781-5-madvenka@linux.microsoft.com> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220215_050803_417588_8F5D62B7 X-CRM114-Status: GOOD ( 33.83 ) 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 Hi Madhavan, The diff itself largely looks good, but we need to actually write the comments. Can you pleaes pick up the wording I've written below for those? That and renaming `unwind_init_from_current` to `unwind_init_from_caller`. With those I think this is good, but I'd like to see the updated version before I provide Acked-by or Reviewed-by tags -- hopefully that's just a formality! :) On Mon, Jan 17, 2022 at 08:56:01AM -0600, madvenka@linux.microsoft.com wrote: > From: "Madhavan T. Venkataraman" > > unwind_init() is currently a single function that initializes all of the > unwind state. Split it into the following functions and call them > appropriately: > > - unwind_init_from_regs() - initialize from regs passed by caller. > > - unwind_init_from_current() - initialize for the current task > from the caller of arch_stack_walk(). > > - unwind_init_from_task() - initialize from the saved state of a > task other than the current task. In this case, the other > task must not be running. > > This is done for two reasons: > > - the different ways of initializing are clear > > - specialized code can be added to each initializer in the future. > > Signed-off-by: Madhavan T. Venkataraman > --- > arch/arm64/kernel/stacktrace.c | 54 +++++++++++++++++++++++++++------- > 1 file changed, 44 insertions(+), 10 deletions(-) > > diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c > index a1a7ff93b84f..b2b568e5deba 100644 > --- a/arch/arm64/kernel/stacktrace.c > +++ b/arch/arm64/kernel/stacktrace.c > @@ -33,11 +33,8 @@ > */ > > > -static void unwind_init(struct unwind_state *state, unsigned long fp, > - unsigned long pc) > +static void unwind_init_common(struct unwind_state *state) > { > - state->fp = fp; > - state->pc = pc; > #ifdef CONFIG_KRETPROBES > state->kr_cur = NULL; > #endif > @@ -56,6 +53,46 @@ static void unwind_init(struct unwind_state *state, unsigned long fp, > state->prev_type = STACK_TYPE_UNKNOWN; > } > > +/* > + * TODO: document requirements here. > + */ Please make this: /* * Start an unwind from a pt_regs. * * The unwind will begin at the PC within the regs. * * The regs must be on a stack currently owned by the calling task. */ > +static inline void unwind_init_from_regs(struct unwind_state *state, > + struct pt_regs *regs) > +{ In future we could add: WARN_ON_ONCE(!on_accessible_stack(current, regs, sizeof(*regs), NULL)); ... to validate the requirements, but I'm happy to lave that for a future patch so this patch can be a pure refactoring. > + unwind_init_common(state); > + > + state->fp = regs->regs[29]; > + state->pc = regs->pc; > +} > + > +/* > + * 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. > + */ Please make this: /* * Start an unwind from a caller. * * The unwind will begin at the caller of whichever function this is inlined * into. * * The function which invokes this must be noinline. */ > +static __always_inline void unwind_init_from_current(struct unwind_state *state) Can we please rename s/current/caller/ here? That way it's clear *where* in current we're unwinding from, and the fact that it's current is implicit but obvious. > +{ Similarly to unwind_init_from_regs(), in a future patch we could add: WARN_ON_ONCE(task == current); ... but for now we can omit that so this patch can be a pure refactoring. > + unwind_init_common(state); > + > + state->fp = (unsigned long)__builtin_frame_address(1); > + state->pc = (unsigned long)__builtin_return_address(0); > +} > + > +/* > + * TODO: document requirements here. > + * > + * The caller guarantees that the task is not running. > + */ Please make this: /* * Start an unwind from a blocked task. * * The unwind will begin at the blocked tasks saved PC (i.e. the caller of * cpu_switch_to()). * * The caller should ensure the task is blocked in cpu_switch_to() for the * duration of the unwind, or the unwind will be bogus. It is never valid to * call this for the current task. */ Thanks, Mark. > +static inline void unwind_init_from_task(struct unwind_state *state, > + struct task_struct *task) > +{ > + unwind_init_common(state); > + > + state->fp = thread_saved_fp(task); > + state->pc = thread_saved_pc(task); > +} > + > /* > * Unwind from one frame record (A) to the next frame record (B). > * > @@ -195,14 +232,11 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry, > struct unwind_state state; > > if (regs) > - unwind_init(&state, regs->regs[29], regs->pc); > + unwind_init_from_regs(&state, regs); > else if (task == current) > - unwind_init(&state, > - (unsigned long)__builtin_frame_address(1), > - (unsigned long)__builtin_return_address(0)); > + unwind_init_from_current(&state); > else > - unwind_init(&state, thread_saved_fp(task), > - thread_saved_pc(task)); > + unwind_init_from_task(&state, task); > > unwind(task, &state, consume_entry, cookie); > } > -- > 2.25.1 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel