Re: [PATCH v12 04/10] arm64: Split unwind_init()

["Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>]

On 1/6/22 10:31 AM, Mark Rutland wrote:
> On Mon, Jan 03, 2022 at 10:52:06AM -0600, madvenka@linux.microsoft.com wrote:
>> From: "Madhavan T. Venkataraman" <madvenka@linux.microsoft.com>
>>
>> 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_regs() - initialize from regs passed by caller.
>>
>> 	- unwind_init_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.
>>
>> 	- unwind_init_common() - initialize fields that are common across
>> 	  the above 3 cases.
>>
>> This is done so that specialized initialization can be added to each case
>> in the future.
>>
>> Signed-off-by: Madhavan T. Venkataraman <madvenka@linux.microsoft.com>
>> ---
>>  arch/arm64/kernel/stacktrace.c | 50 +++++++++++++++++++++++++++-------
>>  1 file changed, 40 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index a1a7ff93b84f..bd797e3f7789 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,40 @@ static void unwind_init(struct unwind_state *state, unsigned long fp,
>>  	state->prev_type = STACK_TYPE_UNKNOWN;
>>  }
>>  
>> +/*
>> + * TODO: document requirements here.
>> + */
>> +static inline void unwind_init_regs(struct unwind_state *state,
>> +				    struct pt_regs *regs)
>> +{
>> +	state->fp = regs->regs[29];
>> +	state->pc = regs->pc;
>> +}
> 
> When I suggested this back in:
> 
>   https://lore.kernel.org/linux-arm-kernel/20211123193723.12112-1-madvenka@linux.microsoft.com/T/#md91fbfe08ceab2a02d9f5c326e17997786e53208
> 
> ... my intent was that each unwind_init_from_*() helpers was the sole
> initializer of the structure, and the caller only had to call one function.
> That way it's not possible to construct an object with an erroneous combination
> of arguments because the prototype enforces the set of arguments, and the
> helper function can operate on a consistent snapshot of those arguments.
> 
> So I'd much prefer that each of these helpers called unwind_init_common(),
> rather than leaving that to the caller to do. I don't mind if those pass
> arguments to unwind_init_common(), or explciitly initialize their respective
> fields, but I don' think the caller should have to care about unwind_init_common().
> 
> I'd also prefer the unwind_init_from*() naming I'd previously suggested, so
> that it's clear which direction information is flowing.
> 

OK. No problem.

>>
>> +
>> +/*
>> + * 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_current(struct unwind_state *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.
>> + */
>> +static inline void unwind_init_task(struct unwind_state *state,
>> +				    struct task_struct *task)
>> +{
>> +	state->fp = thread_saved_fp(task);
>> +	state->pc = thread_saved_pc(task);
>> +}
>> +
>>  /*
>>   * Unwind from one frame record (A) to the next frame record (B).
>>   *
>> @@ -194,15 +225,14 @@ noinline notrace void arch_stack_walk(stack_trace_consume_fn consume_entry,
>>  {
>>  	struct unwind_state state;
>>  
>> +	unwind_init_common(&state);
> 
> As above, I really don't like that the caller has to call both the common
> initializer and a specialized initializer here.
> 

OK. Will change this.

Thanks.

Madhavan