Re: [PATCH v11 08/15] unwind_user/sframe: Wire up unwind_user to sframe

[Jens Remus <jremus@linux.ibm.com>]

On 10/24/2025 3:44 PM, Peter Zijlstra wrote:
> On Wed, Oct 22, 2025 at 04:43:19PM +0200, Jens Remus wrote:
> 
>> @@ -26,12 +27,10 @@ get_user_word(unsigned long *word, unsigned long base, int off, unsigned int ws)
>>  	return get_user(*word, addr);
>>  }
>>  
>> -static int unwind_user_next_fp(struct unwind_user_state *state)
>> +static int unwind_user_next_common(struct unwind_user_state *state,
>> +				   const struct unwind_user_frame *frame,
>> +				   struct pt_regs *regs)
>>  {
> 
> What is pt_regs for? AFAICT it isn't actually used in any of the
> following patches.

Good catch!  No idea.  It started to appear in v9 of the series:

[PATCH v8 06/12] unwind_user/sframe: Wire up unwind_user to sframe
https://lore.kernel.org/all/20250708021159.386608979@kernel.org/

[PATCH v9 06/11] unwind_user/sframe: Wire up unwind_user to sframe
https://lore.kernel.org/all/20250717012936.619600891@kernel.org/

My s390 support for unwind user sframe will make use of it, but it
should better be introduced there then.

@Steven: Any idea why you added pt_regs?  Your v9 even had this other
instance of unused pt_regs:

+static struct unwind_user_frame *get_fp_frame(struct pt_regs *regs)
+{
+	return &fp_frame;
+}

>> @@ -67,6 +66,26 @@ static int unwind_user_next_fp(struct unwind_user_state *state)
>>  	return 0;
>>  }
>>  
>> +static int unwind_user_next_sframe(struct unwind_user_state *state)
>> +{
>> +	struct unwind_user_frame _frame, *frame;
>> +
>> +	/* sframe expects the frame to be local storage */
>> +	frame = &_frame;
>> +	if (sframe_find(state->ip, frame))
>> +		return -ENOENT;
>> +	return unwind_user_next_common(state, frame, task_pt_regs(current));
>> +}
> 
> Would it not be simpler to write:
> 
> static int unwind_user_next_sframe(struct unwind_user_state *state)
> {
> 	struct unwind_user_frame frame;
> 
> 	/* sframe expects the frame to be local storage */
> 	if (sframe_find(state->ip, &frame))
> 		return -ENOENT;
> 	return unwind_user_next_common(state, &frame, task_pt_regs(current));
> }
> 
> hmm?

I agree.  Must have been a leftover from changes from v8 to v9.

>> @@ -80,6 +99,16 @@ static int unwind_user_next(struct unwind_user_state *state)
>>  
>>  		state->current_type = type;
>>  		switch (type) {
>> +		case UNWIND_USER_TYPE_SFRAME:
>> +			switch (unwind_user_next_sframe(state)) {
>> +			case 0:
>> +				return 0;
>> +			case -ENOENT:
>> +				continue;	/* Try next method. */
>> +			default:
>> +				state->done = true;
>> +			}
>> +			break;
> 
> Should it remove SFRAME from state->available_types at this point?

In the -ENOENT case?  If the reason is that there was either no SFrame
section or no SFrame information (SFrame FRE) for the IP, then SFRAME
could potentially be successful with the next IP in the call chain.
Provided the other unwind methods do correctly unwind both SP and FP.

@Steven: What is your opinion on this?

Thanks and regards,
Jens
-- 
Jens Remus
Linux on Z Development (D3303)
+49-7031-16-1128 Office
jremus@de.ibm.com

IBM

IBM Deutschland Research & Development GmbH; Vorsitzender des Aufsichtsrats: Wolfgang Wendt; Geschäftsführung: David Faller; Sitz der Gesellschaft: Böblingen; Registergericht: Amtsgericht Stuttgart, HRB 243294
IBM Data Privacy Statement: https://www.ibm.com/privacy/