1. Patchew
  2. linux
  3. [PATCH v5 00/17] perf: Deferred unwinding of user space stack traces
  4. View patch

[PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic

Steven Rostedt posted 17 patches 7 months, 4 weeks ago
There is a newer version of this series
[PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Steven Rostedt 7 months, 4 weeks ago 
From: Josh Poimboeuf <jpoimboe@kernel.org>

Simplify the get_perf_callchain() user logic a bit.  task_pt_regs()
should never be NULL.

Acked-by: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
---
 kernel/events/callchain.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
index abf258913ab6..bba7f8540ade 100644
--- a/kernel/events/callchain.c
+++ b/kernel/events/callchain.c
@@ -246,22 +246,20 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
 
 	if (user) {
 		if (!user_mode(regs)) {
-			if  (current->mm)
-				regs = task_pt_regs(current);
-			else
-				regs = NULL;
+			if (!current->mm)
+				goto exit_put;
+			regs = task_pt_regs(current);
 		}
 
-		if (regs) {
-			if (add_mark)
-				perf_callchain_store_context(&ctx, PERF_CONTEXT_USER);
+		if (add_mark)
+			perf_callchain_store_context(&ctx, PERF_CONTEXT_USER);
 
-			start_entry_idx = entry->nr;
-			perf_callchain_user(&ctx, regs);
-			fixup_uretprobe_trampoline_entries(entry, start_entry_idx);
-		}
+		start_entry_idx = entry->nr;
+		perf_callchain_user(&ctx, regs);
+		fixup_uretprobe_trampoline_entries(entry, start_entry_idx);
 	}
 
+exit_put:
 	put_callchain_entry(rctx);
 
 	return entry;
-- 
2.47.2
Re: [PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Peter Zijlstra 7 months, 4 weeks ago 
On Thu, Apr 24, 2025 at 12:25:40PM -0400, Steven Rostedt wrote:
> From: Josh Poimboeuf <jpoimboe@kernel.org>
> 
> Simplify the get_perf_callchain() user logic a bit.  task_pt_regs()
> should never be NULL.
> 
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Signed-off-by: Josh Poimboeuf <jpoimboe@kernel.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
> ---
>  kernel/events/callchain.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/events/callchain.c b/kernel/events/callchain.c
> index abf258913ab6..bba7f8540ade 100644
> --- a/kernel/events/callchain.c
> +++ b/kernel/events/callchain.c
> @@ -246,22 +246,20 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
>  
>  	if (user) {
>  		if (!user_mode(regs)) {
> -			if  (current->mm)
> -				regs = task_pt_regs(current);
> -			else
> -				regs = NULL;
> +			if (!current->mm)
> +				goto exit_put;
> +			regs = task_pt_regs(current);

I'm thinking this might be one of those is-kthread test written as
has-mm, and they're broken.

Notably things like the io-uring kthreads do have mm.

>  		}
>  
> -		if (regs) {
> -			if (add_mark)
> -				perf_callchain_store_context(&ctx, PERF_CONTEXT_USER);
> +		if (add_mark)
> +			perf_callchain_store_context(&ctx, PERF_CONTEXT_USER);
>  
> -			start_entry_idx = entry->nr;
> -			perf_callchain_user(&ctx, regs);
> -			fixup_uretprobe_trampoline_entries(entry, start_entry_idx);
> -		}
> +		start_entry_idx = entry->nr;
> +		perf_callchain_user(&ctx, regs);
> +		fixup_uretprobe_trampoline_entries(entry, start_entry_idx);
>  	}
>  
> +exit_put:
>  	put_callchain_entry(rctx);
>  
>  	return entry;
> -- 
> 2.47.2
> 
>
Re: [PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Steven Rostedt 7 months, 4 weeks ago 
On Thu, 24 Apr 2025 18:36:07 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> > +++ b/kernel/events/callchain.c
> > @@ -246,22 +246,20 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
> >  
> >  	if (user) {
> >  		if (!user_mode(regs)) {
> > -			if  (current->mm)
> > -				regs = task_pt_regs(current);
> > -			else
> > -				regs = NULL;
> > +			if (!current->mm)
> > +				goto exit_put;
> > +			regs = task_pt_regs(current);  
> 
> I'm thinking this might be one of those is-kthread test written as
> has-mm, and they're broken.
> 
> Notably things like the io-uring kthreads do have mm.

Would there ever be a case where:

	current->mm == NULL && !(current->flags & PF_KTHREAD)

?

That is, do we still need to check for current->mm if it's not a kernel
thread, or can we assume it exists?

-- Steve
Re: [PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Peter Zijlstra 7 months, 3 weeks ago 
On Thu, Apr 24, 2025 at 01:28:00PM -0400, Steven Rostedt wrote:
> On Thu, 24 Apr 2025 18:36:07 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > > +++ b/kernel/events/callchain.c
> > > @@ -246,22 +246,20 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
> > >  
> > >  	if (user) {
> > >  		if (!user_mode(regs)) {
> > > -			if  (current->mm)
> > > -				regs = task_pt_regs(current);
> > > -			else
> > > -				regs = NULL;
> > > +			if (!current->mm)
> > > +				goto exit_put;
> > > +			regs = task_pt_regs(current);  
> > 
> > I'm thinking this might be one of those is-kthread test written as
> > has-mm, and they're broken.
> > 
> > Notably things like the io-uring kthreads do have mm.
> 
> Would there ever be a case where:
> 
> 	current->mm == NULL && !(current->flags & PF_KTHREAD)
> 
> ?
> 
> That is, do we still need to check for current->mm if it's not a kernel
> thread, or can we assume it exists?

IIRC just checking PF_KTHREAD should be sufficient.
Re: [PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Mathieu Desnoyers 7 months, 4 weeks ago 
On 2025-04-24 13:28, Steven Rostedt wrote:
> On Thu, 24 Apr 2025 18:36:07 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
>>> +++ b/kernel/events/callchain.c
>>> @@ -246,22 +246,20 @@ get_perf_callchain(struct pt_regs *regs, bool kernel, bool user,
>>>   
>>>   	if (user) {
>>>   		if (!user_mode(regs)) {
>>> -			if  (current->mm)
>>> -				regs = task_pt_regs(current);
>>> -			else
>>> -				regs = NULL;
>>> +			if (!current->mm)
>>> +				goto exit_put;
>>> +			regs = task_pt_regs(current);
>>
>> I'm thinking this might be one of those is-kthread test written as
>> has-mm, and they're broken.
>>
>> Notably things like the io-uring kthreads do have mm.

Can a kthread such as io-uring have user_mode(regs) == true ?

> 
> Would there ever be a case where:
> 
> 	current->mm == NULL && !(current->flags & PF_KTHREAD)
> 
> ?
> 
> That is, do we still need to check for current->mm if it's not a kernel
> thread, or can we assume it exists?

The case I'd be careful about is if the code can nest over exit_mm()
(e.g. interrupt) after it sets current->mm = NULL.

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
Re: [PATCH v5 11/17] perf: Simplify get_perf_callchain() user logic
Posted by Steven Rostedt 7 months, 4 weeks ago 
On Thu, 24 Apr 2025 13:42:16 -0400
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> The case I'd be careful about is if the code can nest over exit_mm()
> (e.g. interrupt) after it sets current->mm = NULL.

Then probably we should always check if current->mm is non-NULL before
using it then, and not rely on just task->flags.

-- Steve

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.