From: Josh Poimboeuf <jpoimboe@kernel.org>
Now that the sframe infrastructure is fully in place, make it work by
hooking it up to the unwind_user interface.
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
arch/Kconfig | 1 +
include/linux/unwind_user_types.h | 1 +
kernel/unwind/user.c | 25 ++++++++++++++++++++++---
3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/Kconfig b/arch/Kconfig
index c54d35e2f860..0c6056ef13de 100644
--- a/arch/Kconfig
+++ b/arch/Kconfig
@@ -448,6 +448,7 @@ config HAVE_UNWIND_USER_COMPAT_FP
config HAVE_UNWIND_USER_SFRAME
bool
+ select UNWIND_USER
config HAVE_PERF_REGS
bool
diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
index 0b6563951ca4..4d50476e950e 100644
--- a/include/linux/unwind_user_types.h
+++ b/include/linux/unwind_user_types.h
@@ -13,6 +13,7 @@ enum unwind_user_type {
UNWIND_USER_TYPE_NONE,
UNWIND_USER_TYPE_FP,
UNWIND_USER_TYPE_COMPAT_FP,
+ UNWIND_USER_TYPE_SFRAME,
};
struct unwind_stacktrace {
diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
index 249d9e32fad7..6e7ca9f1293a 100644
--- a/kernel/unwind/user.c
+++ b/kernel/unwind/user.c
@@ -7,6 +7,7 @@
#include <linux/sched/task_stack.h>
#include <linux/unwind_user.h>
#include <linux/uaccess.h>
+#include <linux/sframe.h>
static struct unwind_user_frame fp_frame = {
ARCH_INIT_USER_FP_FRAME
@@ -31,6 +32,12 @@ static inline bool compat_fp_state(struct unwind_user_state *state)
state->type == UNWIND_USER_TYPE_COMPAT_FP;
}
+static inline bool sframe_state(struct unwind_user_state *state)
+{
+ return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_SFRAME) &&
+ state->type == UNWIND_USER_TYPE_SFRAME;
+}
+
#define unwind_get_user_long(to, from, state) \
({ \
int __ret; \
@@ -44,18 +51,28 @@ static inline bool compat_fp_state(struct unwind_user_state *state)
static int unwind_user_next(struct unwind_user_state *state)
{
struct unwind_user_frame *frame;
+ struct unwind_user_frame _frame;
unsigned long cfa = 0, fp, ra = 0;
unsigned int shift;
if (state->done)
return -EINVAL;
- if (compat_fp_state(state))
+ if (compat_fp_state(state)) {
frame = &compat_fp_frame;
- else if (fp_state(state))
+ } else if (sframe_state(state)) {
+ /* sframe expects the frame to be local storage */
+ frame = &_frame;
+ if (sframe_find(state->ip, frame)) {
+ if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
+ goto done;
+ frame = &fp_frame;
+ }
+ } else if (fp_state(state)) {
frame = &fp_frame;
- else
+ } else {
goto done;
+ }
if (frame->use_fp) {
if (state->fp < state->sp)
@@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)
if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
state->type = UNWIND_USER_TYPE_COMPAT_FP;
+ else if (current_has_sframe())
+ state->type = UNWIND_USER_TYPE_SFRAME;
else if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
state->type = UNWIND_USER_TYPE_FP;
else
--
2.47.2On 2025-07-07 22:11, Steven Rostedt wrote:
> From: Josh Poimboeuf <jpoimboe@kernel.org>
>
> Now that the sframe infrastructure is fully in place, make it work by
> hooking it up to the unwind_user interface.
>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
> arch/Kconfig | 1 +
> include/linux/unwind_user_types.h | 1 +
> kernel/unwind/user.c | 25 ++++++++++++++++++++++---
> 3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index c54d35e2f860..0c6056ef13de 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -448,6 +448,7 @@ config HAVE_UNWIND_USER_COMPAT_FP
>
> config HAVE_UNWIND_USER_SFRAME
> bool
> + select UNWIND_USER
>
> config HAVE_PERF_REGS
> bool
> diff --git a/include/linux/unwind_user_types.h b/include/linux/unwind_user_types.h
> index 0b6563951ca4..4d50476e950e 100644
> --- a/include/linux/unwind_user_types.h
> +++ b/include/linux/unwind_user_types.h
> @@ -13,6 +13,7 @@ enum unwind_user_type {
> UNWIND_USER_TYPE_NONE,
> UNWIND_USER_TYPE_FP,
> UNWIND_USER_TYPE_COMPAT_FP,
> + UNWIND_USER_TYPE_SFRAME,
> };
>
> struct unwind_stacktrace {
> diff --git a/kernel/unwind/user.c b/kernel/unwind/user.c
> index 249d9e32fad7..6e7ca9f1293a 100644
> --- a/kernel/unwind/user.c
> +++ b/kernel/unwind/user.c
> @@ -7,6 +7,7 @@
> #include <linux/sched/task_stack.h>
> #include <linux/unwind_user.h>
> #include <linux/uaccess.h>
> +#include <linux/sframe.h>
>
> static struct unwind_user_frame fp_frame = {
> ARCH_INIT_USER_FP_FRAME
> @@ -31,6 +32,12 @@ static inline bool compat_fp_state(struct unwind_user_state *state)
> state->type == UNWIND_USER_TYPE_COMPAT_FP;
> }
>
> +static inline bool sframe_state(struct unwind_user_state *state)
> +{
> + return IS_ENABLED(CONFIG_HAVE_UNWIND_USER_SFRAME) &&
> + state->type == UNWIND_USER_TYPE_SFRAME;
> +}
> +
> #define unwind_get_user_long(to, from, state) \
> ({ \
> int __ret; \
> @@ -44,18 +51,28 @@ static inline bool compat_fp_state(struct unwind_user_state *state)
> static int unwind_user_next(struct unwind_user_state *state)
> {
> struct unwind_user_frame *frame;
> + struct unwind_user_frame _frame;
> unsigned long cfa = 0, fp, ra = 0;
> unsigned int shift;
>
> if (state->done)
> return -EINVAL;
>
> - if (compat_fp_state(state))
> + if (compat_fp_state(state)) {
> frame = &compat_fp_frame;
> - else if (fp_state(state))
> + } else if (sframe_state(state)) {
> + /* sframe expects the frame to be local storage */
> + frame = &_frame;
> + if (sframe_find(state->ip, frame)) {
> + if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
> + goto done;
> + frame = &fp_frame;
> + }
> + } else if (fp_state(state)) {
> frame = &fp_frame;
> - else
> + } else {
> goto done;
> + }
>
> if (frame->use_fp) {
> if (state->fp < state->sp)
> @@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)
>
> if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
> state->type = UNWIND_USER_TYPE_COMPAT_FP;
> + else if (current_has_sframe())
> + state->type = UNWIND_USER_TYPE_SFRAME;
I think you'll want to update the state->type during the
traversal (in next()), because depending on whether
sframe is available for a given memory area of code
or not, the next() function can use either frame pointers
or sframe during the same traversal. It would be good
to know which is used after each specific call to next().
Thanks,
Mathieu
> else if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
> state->type = UNWIND_USER_TYPE_FP;
> else
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On Tue, 8 Jul 2025 15:58:56 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > @@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)
> >
> > if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
> > state->type = UNWIND_USER_TYPE_COMPAT_FP;
> > + else if (current_has_sframe())
> > + state->type = UNWIND_USER_TYPE_SFRAME;
>
> I think you'll want to update the state->type during the
> traversal (in next()), because depending on whether
> sframe is available for a given memory area of code
> or not, the next() function can use either frame pointers
> or sframe during the same traversal. It would be good
> to know which is used after each specific call to next().
From my understanding this sets up what is available for the task at the
beginning.
So once we say "this task has sframes" it will try to use it every time. In
next we have:
if (compat_fp_state(state)) {
frame = &compat_fp_frame;
} else if (sframe_state(state)) {
/* sframe expects the frame to be local storage */
frame = &_frame;
if (sframe_find(state->ip, frame)) {
if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
goto done;
frame = &fp_frame;
}
} else if (fp_state(state)) {
frame = &fp_frame;
} else {
goto done;
}
Where if sframe_find() fails and we switch over to frame pointers, if frame
pointers works, we can continue. But the next iteration, where the frame
pointer finds the previous ip, that ip may be in the sframe section again.
I've seen this work with my trace_printk()s. A function from code that is
running sframes calls into a library function that has frame pointers. The
walk walks through the frame pointers in the library, and when it hits the
code that has sframes, it starts using that again.
If we switched the state to just FP, it will never try to use sframes.
So this state is more about "what does this task have" than what was used
per iteration.
-- Steve
On 08.07.2025 22:11, Steven Rostedt wrote:
> On Tue, 8 Jul 2025 15:58:56 -0400
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>>> @@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)
>>>
>>> if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
>>> state->type = UNWIND_USER_TYPE_COMPAT_FP;
>>> + else if (current_has_sframe())
>>> + state->type = UNWIND_USER_TYPE_SFRAME;
>>
>> I think you'll want to update the state->type during the
>> traversal (in next()), because depending on whether
>> sframe is available for a given memory area of code
>> or not, the next() function can use either frame pointers
>> or sframe during the same traversal. It would be good
>> to know which is used after each specific call to next().
>
> From my understanding this sets up what is available for the task at the
> beginning.
>
> So once we say "this task has sframes" it will try to use it every time. In
> next we have:
>
> if (compat_fp_state(state)) {
> frame = &compat_fp_frame;
> } else if (sframe_state(state)) {
> /* sframe expects the frame to be local storage */
> frame = &_frame;
> if (sframe_find(state->ip, frame)) {
> if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
> goto done;
> frame = &fp_frame;
> }
> } else if (fp_state(state)) {
> frame = &fp_frame;
> } else {
> goto done;
> }
>
> Where if sframe_find() fails and we switch over to frame pointers, if frame
> pointers works, we can continue. But the next iteration, where the frame
> pointer finds the previous ip, that ip may be in the sframe section again.
>
> I've seen this work with my trace_printk()s. A function from code that is
> running sframes calls into a library function that has frame pointers. The
> walk walks through the frame pointers in the library, and when it hits the
> code that has sframes, it starts using that again.
I think Mathieu has a point, as unwind_user_next() calls the optional
architecture-specific arch_unwind_user_next() at the end. The x86
implementation does state->type specific processing (for
UNWIND_USER_TYPE_COMPAT_FP).
> If we switched the state to just FP, it will never try to use sframes.
>
> So this state is more about "what does this task have" than what was used
> per iteration.
While there is currently no fallback to UNWIND_USER_TYPE_COMPAT_FP that
would strictly require this, it could be useful to have both information.
Or the logic in unwind_user_start(), unwind_user_next(), and *_state()
may need to be adjusted so that state->type reflects the currently used
method, which unwind_user_next() determines and sets anew for every step.
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/
On Wed, 9 Jul 2025 09:58:26 +0200 Jens Remus <jremus@linux.ibm.com> wrote: > I think Mathieu has a point, as unwind_user_next() calls the optional > architecture-specific arch_unwind_user_next() at the end. The x86 > implementation does state->type specific processing (for > UNWIND_USER_TYPE_COMPAT_FP). I'm not too comfortable with the compat patches at this stage. I'm thinking of separating out the compat patches, and just reject the deferred unwind if the task is in compat mode (forcing perf or other tracers to use whatever it uses today). I'll take Mathieu's patches and merge them with Josh's, but make them a separate series. I'm aiming to get the core series into this merge window, and the less complexity we have, the better. Then everything can be worked on simultaneously in the next merge window as all the other patches will not have any dependency on each other. They all have dependency on the core set. -- Steve
On Thu, 10 Jul 2025 11:30:39 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> I'm not too comfortable with the compat patches at this stage. I'm
> thinking of separating out the compat patches, and just reject the
> deferred unwind if the task is in compat mode (forcing perf or other
> tracers to use whatever it uses today).
>
> I'll take Mathieu's patches and merge them with Josh's, but make them a
> separate series.
So I'm removing the two compat patches from the series for now and plan
to replace it with this:
-- Steve
From: Steven Rostedt <rostedt@goodmis.org>
Subject: [PATCH] unwind deferred/x86: Do not defer stack tracing for compat
tasks
Currently compat tasks are not supported. If a deferred user space stack
trace is requested on a compat task, it should fail and return an error so
that the profiler can use an alternative approach (whatever it uses
today).
Add a arch_unwind_can_defer() macro that is called in
unwind_deferred_request(). Have x86 define it to a function that makes
sure that the current task is running in 64bit mode, and if it is not, it
returns false. This will cause unwind_deferred_request() to error out and
the caller can use the current method of user space stack tracing.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
arch/x86/include/asm/unwind_user.h | 11 +++++++++++
include/linux/unwind_deferred.h | 5 +++++
kernel/unwind/deferred.c | 3 +++
3 files changed, 19 insertions(+)
diff --git a/arch/x86/include/asm/unwind_user.h b/arch/x86/include/asm/unwind_user.h
index 8597857bf896..220fd0a6e175 100644
--- a/arch/x86/include/asm/unwind_user.h
+++ b/arch/x86/include/asm/unwind_user.h
@@ -2,6 +2,17 @@
#ifndef _ASM_X86_UNWIND_USER_H
#define _ASM_X86_UNWIND_USER_H
+#ifdef CONFIG_IA32_EMULATION
+/* Currently compat mode is not supported for deferred stack trace */
+static inline bool arch_unwind_can_defer(void)
+{
+ struct pt_regs *regs = task_pt_regs(current);
+
+ return user_64bit_mode(regs);
+}
+# define arch_unwind_can_defer arch_unwind_can_defer
+#endif /* CONFIG_IA32_EMULATION */
+
#define ARCH_INIT_USER_FP_FRAME \
.cfa_off = (s32)sizeof(long) * 2, \
.ra_off = (s32)sizeof(long) * -1, \
diff --git a/include/linux/unwind_deferred.h b/include/linux/unwind_deferred.h
index a9d5b100d6b2..6ba4fff066dd 100644
--- a/include/linux/unwind_deferred.h
+++ b/include/linux/unwind_deferred.h
@@ -16,6 +16,11 @@ struct unwind_work {
int bit;
};
+/* Architectures can add a test to not defer unwinding */
+#ifndef arch_unwind_can_defer
+# define arch_unwind_can_defer() (true)
+#endif
+
#ifdef CONFIG_UNWIND_USER
#define UNWIND_PENDING_BIT (BITS_PER_LONG - 1)
diff --git a/kernel/unwind/deferred.c b/kernel/unwind/deferred.c
index 039e12700d49..745144e4717c 100644
--- a/kernel/unwind/deferred.c
+++ b/kernel/unwind/deferred.c
@@ -236,6 +236,9 @@ int unwind_deferred_request(struct unwind_work *work, u64 *cookie)
*cookie = 0;
+ if (!arch_unwind_can_defer())
+ return -EINVAL;
+
if ((current->flags & (PF_KTHREAD | PF_EXITING)) ||
!user_mode(task_pt_regs(current)))
return -EINVAL;
--
2.47.2
On 2025-07-09 03:58, Jens Remus wrote:
> On 08.07.2025 22:11, Steven Rostedt wrote:
>> On Tue, 8 Jul 2025 15:58:56 -0400
>> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>>>> @@ -111,6 +128,8 @@ static int unwind_user_start(struct unwind_user_state *state)
>>>>
>>>> if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) && in_compat_mode(regs))
>>>> state->type = UNWIND_USER_TYPE_COMPAT_FP;
>>>> + else if (current_has_sframe())
>>>> + state->type = UNWIND_USER_TYPE_SFRAME;
>>>
>>> I think you'll want to update the state->type during the
>>> traversal (in next()), because depending on whether
>>> sframe is available for a given memory area of code
>>> or not, the next() function can use either frame pointers
>>> or sframe during the same traversal. It would be good
>>> to know which is used after each specific call to next().
>>
>> From my understanding this sets up what is available for the task at the
>> beginning.
>>
>> So once we say "this task has sframes" it will try to use it every time. In
>> next we have:
>>
>> if (compat_fp_state(state)) {
>> frame = &compat_fp_frame;
>> } else if (sframe_state(state)) {
>> /* sframe expects the frame to be local storage */
>> frame = &_frame;
>> if (sframe_find(state->ip, frame)) {
>> if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
>> goto done;
>> frame = &fp_frame;
>> }
>> } else if (fp_state(state)) {
>> frame = &fp_frame;
>> } else {
>> goto done;
>> }
>>
>> Where if sframe_find() fails and we switch over to frame pointers, if frame
>> pointers works, we can continue. But the next iteration, where the frame
>> pointer finds the previous ip, that ip may be in the sframe section again.
>>
>> I've seen this work with my trace_printk()s. A function from code that is
>> running sframes calls into a library function that has frame pointers. The
>> walk walks through the frame pointers in the library, and when it hits the
>> code that has sframes, it starts using that again.
>
> I think Mathieu has a point, as unwind_user_next() calls the optional
> architecture-specific arch_unwind_user_next() at the end. The x86
> implementation does state->type specific processing (for
> UNWIND_USER_TYPE_COMPAT_FP).
>
>> If we switched the state to just FP, it will never try to use sframes.
>>
>> So this state is more about "what does this task have" than what was used
>> per iteration.
>
> While there is currently no fallback to UNWIND_USER_TYPE_COMPAT_FP that
> would strictly require this, it could be useful to have both information.
>
> Or the logic in unwind_user_start(), unwind_user_next(), and *_state()
> may need to be adjusted so that state->type reflects the currently used
> method, which unwind_user_next() determines and sets anew for every step.
I concur with Jens. I think we should keep track of both:
1) available unwind methods,
2) unwind method used for the current frame.
E.g.:
/*
* unwind types, listed in priority order: lower numbers are
* attempted first if available.
*/
enum unwind_user_type_bits {
UNWIND_USER_TYPE_SFRAME_BIT = 0,
UNWIND_USER_TYPE_FP_BIT = 1,
UNWIND_USER_TYPE_COMPAT_FP_BIT = 2,
_NR_UNWIND_USER_TYPE_BITS,
};
enum unwind_user_type {
UNWIND_USER_TYPE_NONE = 0,
UNWIND_USER_TYPE_SFRAME = (1U << UNWIND_USER_TYPE_SFRAME_BIT),
UNWIND_USER_TYPE_FP = (1U << UNWIND_USER_TYPE_FP_BIT),
UNWIND_USER_TYPE_COMPAT_FP = (1U << UNWIND_USER_TYPE_COMPAT_FP_BIT),
};
And have the following fields in struct unwind_user_state:
/* Unwind time used for the most recent unwind traversal iteration. */
enum unwind_user_type current_type;
/* Unwind types available in the current context. Bitmask of enum unwind_user_type. */
unsigned int available_types;
So as we end up adding stuff like registered JIT unwind info, we will
want to expand the "available types". And it makes sense to both keep
track of all available types (as a way to quickly know which mechanisms
we need to query for the current task) *and* to let the caller know
which unwind type was used for the current frame.
And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and FP in
the future, because the jit unwind info would be more reliable than FP. This
would require that we bump the number for FP and COMPAT_FP, but that would
be OK because this is not ABI.
Thoughts ?
Thanks,
Mathieu
>
> Regards,
> Jens
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On 2025-07-09 09:46, Mathieu Desnoyers wrote:
> On 2025-07-09 03:58, Jens Remus wrote:
>> On 08.07.2025 22:11, Steven Rostedt wrote:
>>> On Tue, 8 Jul 2025 15:58:56 -0400
>>> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>>
>>>>> @@ -111,6 +128,8 @@ static int unwind_user_start(struct
>>>>> unwind_user_state *state)
>>>>> if (IS_ENABLED(CONFIG_HAVE_UNWIND_USER_COMPAT_FP) &&
>>>>> in_compat_mode(regs))
>>>>> state->type = UNWIND_USER_TYPE_COMPAT_FP;
>>>>> + else if (current_has_sframe())
>>>>> + state->type = UNWIND_USER_TYPE_SFRAME;
>>>>
>>>> I think you'll want to update the state->type during the
>>>> traversal (in next()), because depending on whether
>>>> sframe is available for a given memory area of code
>>>> or not, the next() function can use either frame pointers
>>>> or sframe during the same traversal. It would be good
>>>> to know which is used after each specific call to next().
>>>
>>> From my understanding this sets up what is available for the task at
>>> the
>>> beginning.
>>>
>>> So once we say "this task has sframes" it will try to use it every
>>> time. In
>>> next we have:
>>>
>>> if (compat_fp_state(state)) {
>>> frame = &compat_fp_frame;
>>> } else if (sframe_state(state)) {
>>> /* sframe expects the frame to be local storage */
>>> frame = &_frame;
>>> if (sframe_find(state->ip, frame)) {
>>> if (!IS_ENABLED(CONFIG_HAVE_UNWIND_USER_FP))
>>> goto done;
>>> frame = &fp_frame;
>>> }
>>> } else if (fp_state(state)) {
>>> frame = &fp_frame;
>>> } else {
>>> goto done;
>>> }
>>>
>>> Where if sframe_find() fails and we switch over to frame pointers, if
>>> frame
>>> pointers works, we can continue. But the next iteration, where the frame
>>> pointer finds the previous ip, that ip may be in the sframe section
>>> again.
>>>
>>> I've seen this work with my trace_printk()s. A function from code
>>> that is
>>> running sframes calls into a library function that has frame
>>> pointers. The
>>> walk walks through the frame pointers in the library, and when it
>>> hits the
>>> code that has sframes, it starts using that again.
>>
>> I think Mathieu has a point, as unwind_user_next() calls the optional
>> architecture-specific arch_unwind_user_next() at the end. The x86
>> implementation does state->type specific processing (for
>> UNWIND_USER_TYPE_COMPAT_FP).
>>
>>> If we switched the state to just FP, it will never try to use sframes.
>>>
>>> So this state is more about "what does this task have" than what was
>>> used
>>> per iteration.
>>
>> While there is currently no fallback to UNWIND_USER_TYPE_COMPAT_FP that
>> would strictly require this, it could be useful to have both information.
>>
>> Or the logic in unwind_user_start(), unwind_user_next(), and *_state()
>> may need to be adjusted so that state->type reflects the currently used
>> method, which unwind_user_next() determines and sets anew for every step.
>
> I concur with Jens. I think we should keep track of both:
>
> 1) available unwind methods,
>
> 2) unwind method used for the current frame.
>
> E.g.:
>
> /*
> * unwind types, listed in priority order: lower numbers are
> * attempted first if available.
> */
> enum unwind_user_type_bits {
> UNWIND_USER_TYPE_SFRAME_BIT = 0,
> UNWIND_USER_TYPE_FP_BIT = 1,
> UNWIND_USER_TYPE_COMPAT_FP_BIT = 2,
>
> _NR_UNWIND_USER_TYPE_BITS,
> };
>
> enum unwind_user_type {
> UNWIND_USER_TYPE_NONE = 0,
> UNWIND_USER_TYPE_SFRAME = (1U << UNWIND_USER_TYPE_SFRAME_BIT),
> UNWIND_USER_TYPE_FP = (1U << UNWIND_USER_TYPE_FP_BIT),
> UNWIND_USER_TYPE_COMPAT_FP = (1U <<
> UNWIND_USER_TYPE_COMPAT_FP_BIT),
> };
>
> And have the following fields in struct unwind_user_state:
>
> /* Unwind time used for the most recent unwind traversal iteration. */
> enum unwind_user_type current_type;
>
> /* Unwind types available in the current context. Bitmask of enum
> unwind_user_type. */
> unsigned int available_types;
>
> So as we end up adding stuff like registered JIT unwind info, we will
> want to expand the "available types". And it makes sense to both keep
> track of all available types (as a way to quickly know which mechanisms
> we need to query for the current task) *and* to let the caller know
> which unwind type was used for the current frame.
>
> And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and
> FP in
> the future, because the jit unwind info would be more reliable than FP.
> This
> would require that we bump the number for FP and COMPAT_FP, but that would
> be OK because this is not ABI.
>
> Thoughts ?
One use-case for giving the "current_type" to iteration callers is to
let end users know whether they should trust the frame info. If it
comes from sframe, then it should be pretty solid. However, if it comes
from frame pointers used as a fallback on a system that omits frame
pointers, the user should consider the resulting data with a high level
of skepticism.
Thanks,
Mathieu
>
> Thanks,
>
> Mathieu
>
>>
>> Regards,
>> Jens
>
>
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
On 09.07.2025 15:51, Mathieu Desnoyers wrote:
> On 2025-07-09 09:46, Mathieu Desnoyers wrote:
>> I concur with Jens. I think we should keep track of both:
>>
>> 1) available unwind methods,
>>
>> 2) unwind method used for the current frame.
>>
>> E.g.:
>>
>> /*
>> * unwind types, listed in priority order: lower numbers are
>> * attempted first if available.
>> */
>> enum unwind_user_type_bits {
>> UNWIND_USER_TYPE_SFRAME_BIT = 0,
>> UNWIND_USER_TYPE_FP_BIT = 1,
>> UNWIND_USER_TYPE_COMPAT_FP_BIT = 2,
>>
>> _NR_UNWIND_USER_TYPE_BITS,
>> };
>>
>> enum unwind_user_type {
>> UNWIND_USER_TYPE_NONE = 0,
>> UNWIND_USER_TYPE_SFRAME = (1U << UNWIND_USER_TYPE_SFRAME_BIT),
>> UNWIND_USER_TYPE_FP = (1U << UNWIND_USER_TYPE_FP_BIT),
>> UNWIND_USER_TYPE_COMPAT_FP = (1U << UNWIND_USER_TYPE_COMPAT_FP_BIT),
>> };
>>
>> And have the following fields in struct unwind_user_state:
>>
>> /* Unwind time used for the most recent unwind traversal iteration. */
>> enum unwind_user_type current_type;
>>
>> /* Unwind types available in the current context. Bitmask of enum unwind_user_type. */
>> unsigned int available_types;
>>
>> So as we end up adding stuff like registered JIT unwind info, we will
>> want to expand the "available types". And it makes sense to both keep
>> track of all available types (as a way to quickly know which mechanisms
>> we need to query for the current task) *and* to let the caller know
>> which unwind type was used for the current frame.
>>
>> And AFAIU we'd be inserting a "jit unwind info" type between SFRAME and FP in
>> the future, because the jit unwind info would be more reliable than FP. This
>> would require that we bump the number for FP and COMPAT_FP, but that would
>> be OK because this is not ABI.
>>
>> Thoughts ?
>
> One use-case for giving the "current_type" to iteration callers is to
> let end users know whether they should trust the frame info. If it
> comes from sframe, then it should be pretty solid. However, if it comes
> from frame pointers used as a fallback on a system that omits frame
> pointers, the user should consider the resulting data with a high level
> of skepticism.
The current_type may be different for every unwind step (frame). So
struct unwind_stacktrace would probably need the following added:
/* Unwind types used for taking the stack trace. */
unsigned int used_types;
So that the user of unwind_user() could decide whether to trust the
stack trace. But as Steve suggested in his reply, all of this could
be added later, once there is a need.
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/
On Wed, 9 Jul 2025 09:51:09 -0400 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > One use-case for giving the "current_type" to iteration callers is to > let end users know whether they should trust the frame info. If it > comes from sframe, then it should be pretty solid. However, if it comes > from frame pointers used as a fallback on a system that omits frame > pointers, the user should consider the resulting data with a high level > of skepticism. That would be in the trace sent to the callback. We could add something like the '?' if it's not trusted. But for now, until we have a use case that we are implementing, I want to keep this simple, otherwise it will never get done. I don't want to add features for hypothetical scenarios. Currently, the traceback is just an array of addresses. But this could change in the future. What we are discussing right now is the internal functionality of the user unwind code where I have made most of theses functions static. The only external functions that get called during the iteration is the architecture specific code. If that code needs to know the difference between sframes and frame pointers then we can modify it, but until then, I rather keep this as is. Jens, is there something that the architecture code needs now? If so, then lets fix it, otherwise lets do it when there is something. This isn't user API, it can change in the future. -- Steve
On 09.07.2025 16:06, Steven Rostedt wrote: > But for now, until we have a use case that we are implementing, I want > to keep this simple, otherwise it will never get done. I don't want to > add features for hypothetical scenarios. > Jens, is there something that the architecture code needs now? If so, > then lets fix it, otherwise lets do it when there is something. This > isn't user API, it can change in the future. I am fine with your suggestion. My s390 sframe support does not require any changes in this regard. 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/
On 2025-07-09 10:06, Steven Rostedt wrote: > On Wed, 9 Jul 2025 09:51:09 -0400 > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > >> One use-case for giving the "current_type" to iteration callers is to >> let end users know whether they should trust the frame info. If it >> comes from sframe, then it should be pretty solid. However, if it comes >> from frame pointers used as a fallback on a system that omits frame >> pointers, the user should consider the resulting data with a high level >> of skepticism. > > That would be in the trace sent to the callback. We could add something > like the '?' if it's not trusted. > > But for now, until we have a use case that we are implementing, I want > to keep this simple, otherwise it will never get done. I don't want to > add features for hypothetical scenarios. > > Currently, the traceback is just an array of addresses. But this could > change in the future. What we are discussing right now is the internal > functionality of the user unwind code where I have made most of theses > functions static. > > The only external functions that get called during the iteration is the > architecture specific code. If that code needs to know the difference > between sframes and frame pointers then we can modify it, but until > then, I rather keep this as is. Indeed it's only kernel internal API, but this is API that will be expected by each architecture supporting unwind_user. Changing this later on will cause a lot of friction and cross-architecture churn compared to doing it right in the first place. Thanks, Mathieu > > Jens, is there something that the architecture code needs now? If so, > then lets fix it, otherwise lets do it when there is something. This > isn't user API, it can change in the future. > > -- Steve -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
On Wed, 9 Jul 2025 10:10:30 -0400 Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > Indeed it's only kernel internal API, but this is API that will be > expected by each architecture supporting unwind_user. Changing > this later on will cause a lot of friction and cross-architecture churn > compared to doing it right in the first place. The changes you are suggesting is added info if an architecture needs it. That is easy to do. All you need is to add an extra field in the state structure and the architectures that need it can use it, and the rest can ignore it. Again, I'm not worried about it. If you want to send me a patch, feel free, but I'm not doing this extra work, until I see a real problem. -- Steve.
On 2025-07-09 10:29, Steven Rostedt wrote: > On Wed, 9 Jul 2025 10:10:30 -0400 > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > >> Indeed it's only kernel internal API, but this is API that will be >> expected by each architecture supporting unwind_user. Changing >> this later on will cause a lot of friction and cross-architecture churn >> compared to doing it right in the first place. > > The changes you are suggesting is added info if an architecture needs > it. That is easy to do. All you need is to add an extra field in the > state structure and the architectures that need it can use it, and the > rest can ignore it. > > Again, I'm not worried about it. If you want to send me a patch, feel > free, but I'm not doing this extra work, until I see a real problem. OK, I'll do it. As I look into the existing state, the priority order appears to be incorrect on compat mode: if we have both compat mode and sframe available, COMPAT_FP is preferred. I think we want to favor using sframe first. But then if you select "sframe" in start(), then you don't have the "compat state" information for the compat-fp fallback. So the "type" logic is all intermingled between "fp vs sframe" and "compat vs non-compat" there. My changes will clean this up. Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. https://www.efficios.com
© 2016 - 2025 Red Hat, Inc.