Re: [PATCH v12 05/10] arm64: Copy unwind arguments to unwind_state

[Madhavan T. Venkataraman]

On 1/6/22 10:37 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:07AM -0600, madvenka@xxxxxxxxxxxxxxxxxxx wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@xxxxxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxxxxx>
>> ---
>>  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