From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
To: Mark Rutland <mark.rutland@arm.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 v12 05/10] arm64: Copy unwind arguments to unwind_state
Date: Thu, 6 Jan 2022 14:17:30 -0600 [thread overview]
Message-ID: <6fce7dd5-2f45-5034-bdf8-6c3a3499e9e7@linux.microsoft.com> (raw)
In-Reply-To: <Ydcax2E9u4D4/3Q9@FVFF77S0Q05N>
On 1/6/22 10:37 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> Copy the following arguments passed to arch_stack_walk() to unwind_state
>> so that they can be passed to unwind functions via unwind_state rather
>> than as separate arguments:
>>
>> - task
>
> I agree the task should be placed in the unwind state, since it's a key part of
> the environment for the unwind.
>
>> - regs
>
> This isn't relevant in all cases, and so for now I'd strongly prefer *not* to
> have this in the unwind state as it's liable to lead to confusion and get
> misused.
>
>> - consume_entry
>> - cookie
>
> These are only relevant for the invocation of the consume_entry() function, and
> so similarly I do not think they should be part of the state. It's simpler for
> these to be local variables.
>
OK.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
>> ---
>> arch/arm64/include/asm/stacktrace.h | 12 ++++++++
>> arch/arm64/kernel/stacktrace.c | 45 ++++++++++++++++-------------
>> 2 files changed, 37 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/stacktrace.h b/arch/arm64/include/asm/stacktrace.h
>> index fc828c3c5dfd..322817d40a75 100644
>> --- a/arch/arm64/include/asm/stacktrace.h
>> +++ b/arch/arm64/include/asm/stacktrace.h
>> @@ -51,6 +51,14 @@ struct stack_info {
>> * @kr_cur: When KRETPOLINES is selected, holds the kretprobe instance
>> * associated with the most recently encountered replacement lr
>> * value.
>> + *
>> + * @task: Pointer to the task structure.
>> + *
>> + * @regs: Registers, if any.
>> + *
>> + * @consume_pc Consume PC function pointer.
>> + *
>> + * @cookie Argument to consume_pc().
>> */
>> struct unwind_state {
>> unsigned long fp;
>> @@ -61,6 +69,10 @@ struct unwind_state {
>> #ifdef CONFIG_KRETPROBES
>> struct llist_node *kr_cur;
>> #endif
>> + struct task_struct *task;
>> + struct pt_regs *regs;
>> + stack_trace_consume_fn consume_pc;
>> + void *cookie;
>> };
>>
>> extern void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index bd797e3f7789..3ecb8242caa5 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -33,8 +33,17 @@
>> */
>>
>>
>> -static void unwind_init_common(struct unwind_state *state)
>> +static void unwind_init_common(struct unwind_state *state,
>> + struct task_struct *task,
>> + struct pt_regs *regs,
>> + stack_trace_consume_fn consume_pc,
>> + void *cookie)
>> {
>> + state->task = task;
>> + state->regs = regs;
>> + state->consume_pc = consume_pc;
>> + state->cookie = cookie;
>> +
>> #ifdef CONFIG_KRETPROBES
>> state->kr_cur = NULL;
>> #endif
>> @@ -56,11 +65,10 @@ static void unwind_init_common(struct unwind_state *state)
>> /*
>> * TODO: document requirements here.
>> */
>> -static inline void unwind_init_regs(struct unwind_state *state,
>> - struct pt_regs *regs)
>> +static inline void unwind_init_regs(struct unwind_state *state)
>> {
>> - state->fp = regs->regs[29];
>> - state->pc = regs->pc;
>> + state->fp = state->regs->regs[29];
>> + state->pc = state->regs->pc;
>> }
>>
>> /*
>> @@ -80,11 +88,10 @@ static __always_inline void unwind_init_current(struct unwind_state *state)
>> *
>> * The caller guarantees that the task is not running.
>> */
>> -static inline void unwind_init_task(struct unwind_state *state,
>> - struct task_struct *task)
>> +static inline void unwind_init_task(struct unwind_state *state)
>> {
>> - state->fp = thread_saved_fp(task);
>> - state->pc = thread_saved_pc(task);
>> + state->fp = thread_saved_fp(state->task);
>> + state->pc = thread_saved_pc(state->task);
>> }
>>
>> /*
>> @@ -94,9 +101,9 @@ static inline void unwind_init_task(struct unwind_state *state,
>> * records (e.g. a cycle), determined based on the location and fp value of A
>> * and the location (but not the fp value) of B.
>> */
>> -static int notrace unwind_next(struct task_struct *tsk,
>> - struct unwind_state *state)
>> +static int notrace unwind_next(struct unwind_state *state)
>> {
>> + struct task_struct *tsk = state->task;
>> unsigned long fp = state->fp;
>> struct stack_info info;
>>
>> @@ -170,16 +177,14 @@ static int notrace unwind_next(struct task_struct *tsk,
>> }
>> NOKPROBE_SYMBOL(unwind_next);
>>
>> -static void notrace unwind(struct task_struct *tsk,
>> - struct unwind_state *state,
>> - bool (*fn)(void *, unsigned long), void *data)
>> +static void notrace unwind(struct unwind_state *state)
>> {
>> while (1) {
>> int ret;
>>
>> - if (!fn(data, state->pc))
>> + if (!state->consume_pc(state->cookie, state->pc))
>> break;
>> - ret = unwind_next(tsk, state);
>> + ret = unwind_next(state);
>> if (ret < 0)
>> break;
>> }
>> @@ -225,14 +230,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>> {
>> struct unwind_state state;
>>
>> - unwind_init_common(&state);
>> + unwind_init_common(&state, task, regs, consume_entry, cookie);
>>
>> if (regs)
>> - unwind_init_regs(&state, regs);
>> + unwind_init_regs(&state);
>> else if (task == current)
>> unwind_init_current(&state);
>> else
>> - unwind_init_task(&state, task);
>> + unwind_init_task(&state);
>>
>> - unwind(task, &state, consume_entry, cookie);
>> + unwind(&state);
>
> I don't like the changes here in particular since they hide the information
> flow relevant to each case.
>
Per previous comment I agreed to, I will pass all the arguments other than task
directly.
Thanks.
Madhavan
next prev parent reply other threads:[~2022-01-06 20:17 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <0d0eb36f348fb5a6af6eb592c0525f6e94007328>
2022-01-03 16:52 ` [PATCH v12 00/10] arm64: Reorganize the unwinder and implement stack trace reliability checks madvenka
2022-01-03 16:52 ` [PATCH v12 01/10] arm64: Remove NULL task check from unwind_frame() madvenka
2022-01-06 16:07 ` Mark Rutland
2022-01-03 16:52 ` [PATCH v12 02/10] arm64: Rename unwinder functions madvenka
2022-01-06 16:10 ` Mark Rutland
2022-01-03 16:52 ` [PATCH v12 03/10] arm64: Rename stackframe to unwind_state madvenka
2022-01-04 14:59 ` Mark Brown
2022-01-06 16:11 ` Mark Rutland
2022-01-03 16:52 ` [PATCH v12 04/10] arm64: Split unwind_init() madvenka
2022-01-06 16:31 ` Mark Rutland
2022-01-06 20:13 ` Madhavan T. Venkataraman
2022-01-03 16:52 ` [PATCH v12 05/10] arm64: Copy unwind arguments to unwind_state madvenka
2022-01-05 16:57 ` Mark Brown
2022-01-06 16:37 ` Mark Rutland
2022-01-06 20:17 ` Madhavan T. Venkataraman [this message]
2022-01-03 16:52 ` [PATCH v12 06/10] arm64: Make the unwind loop in unwind() similar to other architectures madvenka
2022-01-03 16:52 ` [PATCH v12 07/10] arm64: Introduce stack trace reliability checks in the unwinder madvenka
2022-01-05 16:58 ` Mark Brown
2022-01-05 23:58 ` Madhavan T. Venkataraman
2022-01-06 11:43 ` Mark Brown
2022-01-03 16:52 ` [PATCH v12 08/10] arm64: Create a list of SYM_CODE functions, check return PC against list madvenka
2022-01-03 16:52 ` [PATCH v12 09/10] arm64: Introduce arch_stack_walk_reliable() madvenka
2022-01-03 16:52 ` [PATCH v12 10/10] arm64: Select HAVE_RELIABLE_STACKTRACE madvenka
Reply instructions:
You may reply publicly 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=6fce7dd5-2f45-5034-bdf8-6c3a3499e9e7@linux.microsoft.com \
--to=madvenka@linux.microsoft.com \
--cc=ardb@kernel.org \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=jmorris@namei.org \
--cc=jpoimboe@redhat.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=live-patching@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nobuta.keiya@fujitsu.com \
--cc=sjitindarsingh@gmail.com \
--cc=will@kernel.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).