[PATCH v4 0/2] tracing: fgraph: fix the quadratic shadow stack walk

[PATCH v4 0/2] tracing: fgraph: fix the quadratic shadow stack walk

[Vineet Gupta]

alloc_retstack_tasklist() assigns a shadow stack to every thread when
fgraph is enabled. It did so FTRACE_RETSTACK_ALLOC_SIZE (32) tasks at a
time, and since for_each_process_thread() has no cursor, every sweep
restarted from init_task and re-walked the tasks already served, making
the ftrace_graph_active 0 -> 1 transition quadratic in the thread count.
On a 60-core Sapphire Rapids machine with 400000 idle threads it took
227 s, inside a single bpf() syscall for the kprobe_multi case.

 - 1/2 allocates inline with GFP_NOWAIT, which removes the batching:
   a single sweep serves every task, 227 s -> 0.092 s. It keeps the
   open-coded rcu_read_lock() for ease of backport.

 - 2/2 is the cleanup: convert that same walk to scoped_guard(rcu).
   No functional change, and not needed by stable.

Both were tested on bare metal and in a VM, including forcing the
GFP_NOWAIT failure path with failslab injection.

v1: https://lore.kernel.org/all/20260922225526.1554758-1-vineet.gupta@linux.dev/
v2: https://lore.kernel.org/all/20260929005411.4105448-1-vineet.gupta@linux.dev/
v3: https://lore.kernel.org/all/20261001102909.1152572-1-vineet.gupta@linux.dev/

Changes since v3:
 - split into two patches: the fix keeps goto-based unwinding so it
   backports cleanly, and scoped_guard(rcu) moves to a separate
   cleanup, per Masami and Steve:
   https://lore.kernel.org/all/20261013194542.7e036b03@226.227.38.69.naturalwireless.com/
 - in the cleanup, -EAGAIN now exits via the existing free label
   rather than returning from inside the guard, so there is no longer
   a path that skips the free loop

Changes since v2:
 - update the stale alloc_retstack_tasklist() comment, which still
   described the old batch-of-32 behaviour (bot+bpf-ci)

Changes since v1:
 - replace both v1 patches with Peter's inline GFP_NOWAIT approach,
   which removes the quadratic behaviour rather than dividing it by a
   constant. With a single sweep there is no retry loop left, so the
   v1 cond_resched() patch has nothing to attach to and is dropped.

Vineet Gupta (2):
  tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT
  tracing: fgraph: use scoped_guard() for the task list walk

 kernel/trace/fgraph.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.53.0-Meta

[PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Vineet Gupta]

When ftrace graphing is turned on, all tasks in the system missing
return stack page are assigned one. This is done in a simplistic
multi-sweep loop of FTRACE_RETSTACK_ALLOC_SIZE (currently 32) tasks
at a time as follows:

   start_graph_tracing()
	do {
		alloc_retstack_tasklist
	} while (-EAGAIN);

   alloc_retstack_tasklist()
	alloc x32           # GFP_KERNEL, may sleep
	rcu_read_lock()     # preempt off
		for_each_process_thread    walk N_total, no cond_resched
                     t->ret_stack = new_page
	rcu_read_unlock()   # preempt enable but no explicit yield

Each successive iteration of loop invokes for_each_process_thread()
which doesn't support cursor based resume and always restarts from the
init_task. Thus each successive loop needs to skip the tasks assigned
ret_stack in prior sweeps and thus take longer and longer to find the
candidate 32 tasks. And while the loop end calls rcu unlock,
and re-enables preemption briefly, there is no explicit yield.

The total number of iterations turn out to be

	N_total * N_null / (2 * FTRACE_RETSTACK_ALLOC_SIZE)

where N_total is the number of threads on the system and N_null the number
of those whose ret_stack is still NULL. After boot with no fgraph user that
is every thread.

This is not a tracing-only path. Since commit 4346ba160409 ("fprobe:
Rewrite fprobe on function-graph tracer") fprobe is built on fgraph,
so an ordinary kprobe_multi BPF attach that flips ftrace_graph_active
from 0 to 1 pays the whole cost inside one bpf() syscall. Commit
2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
narrows that to return and session probes but does not remove it.

A host running hundreds of thousands of threads, with the current batch
size of 32, turns the ftrace_graph_active 0 -> 1 transition into a
multi-second, and eventually multi-minute, operation which showed up as
RCU stall and softlockup_panic on heavily loaded Meta Fleet machines
with preemption disabled.

    rcu: INFO: rcu_sched self-detected stall on CPU
    rcu: 0-....: (20718 ticks this GP) (t=21000 jiffies g=913 q=8 ncpus=8)
    CPU: 0 UID: 0 PID: 160141 Comm: bpftrace
     ___slab_alloc+0x549/0xa20
     kmem_cache_alloc_noprof+0x16c/0x340
     register_ftrace_graph+0x3d7/0x6c0
     register_fprobe_ips+0x2d5/0x310
     bpf_kprobe_multi_link_attach+0x218/0x7e0
     __sys_bpf+0x267a/0x27a0

Fix this by allocating inline with GFP_NOWAIT instead of pre-allocating
a fixed batch. GFP_NOWAIT does not sleep, so it is legal inside the RCU
read-side critical section.

A single sweep now serves every task, so the walk becomes O(N) rather
than O(N_total * N_null), and the retry loop is no longer the normal
path.

The pre-allocated array remains as the fallback when GFP_NOWAIT fails.
The runs below never needed it, so it was exercised with failslab
injection: at a 100% failure rate every task falls back and -EAGAIN
retries ~1560 times for 50000 threads, with no stall or splat.

The open-coded rcu_read_lock() and the goto unwind are kept as-is so
this stays easy to backport; converting them to scope-based cleanup is
left to a follow-up.

smp_store_release() is a drop-in replacement for the smp_wmb() and
plain store it replaces.

Measured on a 60-core Sapphire Rapids machine running a PREEMPT_LAZY
kernel (CONFIG_PREEMPT_DYNAMIC=y, mode lazy), timing the write that
drives the ftrace_graph_active 0 -> 1 transition. Threads are spawned
while fgraph is inactive so every one of them has ret_stack == NULL,
and the loop was instrumented to count sweeps:

	threads   before              after
	          sweeps   time       sweeps   time
	100000     3126     15.3 s      1       0.019 s
	200000     6253     59.1 s      1       0.047 s
	400000    12503    227.8 s      1       0.092 s

The sweep count is 1 throughout. Time now scales linearly with the
thread count at roughly 230 ns per task, which is the allocation and
initialisation of one shadow stack.

Fixes: f201ae2356c7 ("tracing/function-return-tracer: store return stack into task_struct and allocate it dynamically")
Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
Tested-by: Vineet Gupta <vineet.gupta@linux.dev>
Cc: stable@vger.kernel.org
---
 kernel/trace/fgraph.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index ed455b53513b..4c74f1a15ad4 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -1033,13 +1033,16 @@ void ftrace_stub_graph(struct ftrace_graph_ret *trace, struct fgraph_ops *gops,
 trace_func_graph_ret_t ftrace_graph_return = ftrace_stub_graph;
 trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
 
-/* Try to assign a return stack array on FTRACE_RETSTACK_ALLOC_SIZE tasks. */
+/*
+ * Loop thru all the tasks in the system and assign ret_stack.
+ * The @ret_stack_list is a small pre-allocated fallback, in case
+ * the GFP_NOWAIT allocations here fail.
+ */
 static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
 {
-	int i;
-	int ret = 0;
 	int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
 	struct task_struct *g, *t;
+	int i, ret = 0;
 
 	if (WARN_ON_ONCE(!fgraph_stack_cachep))
 		return -ENOMEM;
@@ -1056,20 +1059,26 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
 
 	rcu_read_lock();
 	for_each_process_thread(g, t) {
-		if (start == end) {
-			ret = -EAGAIN;
-			goto unlock;
-		}
+		unsigned long *rs;
 
-		if (t->ret_stack == NULL) {
-			atomic_set(&t->trace_overrun, 0);
-			ret_stack_init_task_vars(ret_stack_list[start]);
-			t->curr_ret_stack = 0;
-			t->curr_ret_depth = -1;
-			/* Make sure the tasks see the 0 first: */
-			smp_wmb();
-			t->ret_stack = ret_stack_list[start++];
+		if (t->ret_stack)
+			continue;
+
+		rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
+		if (!rs) {
+			if (start == end) {
+				ret = -EAGAIN;
+				goto unlock;
+			}
+			rs = ret_stack_list[start++];
 		}
+
+		atomic_set(&t->trace_overrun, 0);
+		ret_stack_init_task_vars(rs);
+		t->curr_ret_stack = 0;
+		t->curr_ret_depth = -1;
+		/* Make sure the tasks see the 0 first: */
+		smp_store_release(&t->ret_stack, rs);
 	}
 
 unlock:

base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.53.0-Meta

Re: [PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Andrii Nakryiko]

On Wed, Oct 21, 2026 at 10:44 AM Vineet Gupta <vineet.gupta@linux.dev> wrote:
>
> When ftrace graphing is turned on, all tasks in the system missing
> return stack page are assigned one. This is done in a simplistic
> multi-sweep loop of FTRACE_RETSTACK_ALLOC_SIZE (currently 32) tasks
> at a time as follows:
>
>    start_graph_tracing()
>         do {
>                 alloc_retstack_tasklist
>         } while (-EAGAIN);
>
>    alloc_retstack_tasklist()
>         alloc x32           # GFP_KERNEL, may sleep
>         rcu_read_lock()     # preempt off
>                 for_each_process_thread    walk N_total, no cond_resched
>                      t->ret_stack = new_page
>         rcu_read_unlock()   # preempt enable but no explicit yield
>
> Each successive iteration of loop invokes for_each_process_thread()
> which doesn't support cursor based resume and always restarts from the
> init_task. Thus each successive loop needs to skip the tasks assigned
> ret_stack in prior sweeps and thus take longer and longer to find the
> candidate 32 tasks. And while the loop end calls rcu unlock,
> and re-enables preemption briefly, there is no explicit yield.
>
> The total number of iterations turn out to be
>
>         N_total * N_null / (2 * FTRACE_RETSTACK_ALLOC_SIZE)
>
> where N_total is the number of threads on the system and N_null the number
> of those whose ret_stack is still NULL. After boot with no fgraph user that
> is every thread.
>
> This is not a tracing-only path. Since commit 4346ba160409 ("fprobe:
> Rewrite fprobe on function-graph tracer") fprobe is built on fgraph,
> so an ordinary kprobe_multi BPF attach that flips ftrace_graph_active
> from 0 to 1 pays the whole cost inside one bpf() syscall. Commit
> 2c67dc457bc6 ("tracing: fprobe: optimization for entry only case")
> narrows that to return and session probes but does not remove it.
>
> A host running hundreds of thousands of threads, with the current batch
> size of 32, turns the ftrace_graph_active 0 -> 1 transition into a
> multi-second, and eventually multi-minute, operation which showed up as
> RCU stall and softlockup_panic on heavily loaded Meta Fleet machines
> with preemption disabled.
>
>     rcu: INFO: rcu_sched self-detected stall on CPU
>     rcu: 0-....: (20718 ticks this GP) (t=21000 jiffies g=913 q=8 ncpus=8)
>     CPU: 0 UID: 0 PID: 160141 Comm: bpftrace
>      ___slab_alloc+0x549/0xa20
>      kmem_cache_alloc_noprof+0x16c/0x340
>      register_ftrace_graph+0x3d7/0x6c0
>      register_fprobe_ips+0x2d5/0x310
>      bpf_kprobe_multi_link_attach+0x218/0x7e0
>      __sys_bpf+0x267a/0x27a0
>
> Fix this by allocating inline with GFP_NOWAIT instead of pre-allocating
> a fixed batch. GFP_NOWAIT does not sleep, so it is legal inside the RCU
> read-side critical section.
>
> A single sweep now serves every task, so the walk becomes O(N) rather
> than O(N_total * N_null), and the retry loop is no longer the normal
> path.
>
> The pre-allocated array remains as the fallback when GFP_NOWAIT fails.
> The runs below never needed it, so it was exercised with failslab
> injection: at a 100% failure rate every task falls back and -EAGAIN
> retries ~1560 times for 50000 threads, with no stall or splat.
>
> The open-coded rcu_read_lock() and the goto unwind are kept as-is so
> this stays easy to backport; converting them to scope-based cleanup is
> left to a follow-up.
>
> smp_store_release() is a drop-in replacement for the smp_wmb() and
> plain store it replaces.
>
> Measured on a 60-core Sapphire Rapids machine running a PREEMPT_LAZY
> kernel (CONFIG_PREEMPT_DYNAMIC=y, mode lazy), timing the write that
> drives the ftrace_graph_active 0 -> 1 transition. Threads are spawned
> while fgraph is inactive so every one of them has ret_stack == NULL,
> and the loop was instrumented to count sweeps:
>
>         threads   before              after
>                   sweeps   time       sweeps   time
>         100000     3126     15.3 s      1       0.019 s
>         200000     6253     59.1 s      1       0.047 s
>         400000    12503    227.8 s      1       0.092 s
>
> The sweep count is 1 throughout. Time now scales linearly with the
> thread count at roughly 230 ns per task, which is the allocation and
> initialisation of one shadow stack.
>
> Fixes: f201ae2356c7 ("tracing/function-return-tracer: store return stack into task_struct and allocate it dynamically")
> Suggested-by: Peter Zijlstra <peterz@infradead.org>
> Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
> Tested-by: Vineet Gupta <vineet.gupta@linux.dev>
> Cc: stable@vger.kernel.org
> ---
>  kernel/trace/fgraph.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>

Acked-by: Andrii Nakryiko <andrii@kernel.org>

I presume this will go through Masami and Steven's tree, is that right?

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index ed455b53513b..4c74f1a15ad4 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1033,13 +1033,16 @@ void ftrace_stub_graph(struct ftrace_graph_ret *trace, struct fgraph_ops *gops,
>  trace_func_graph_ret_t ftrace_graph_return = ftrace_stub_graph;
>  trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
>
> -/* Try to assign a return stack array on FTRACE_RETSTACK_ALLOC_SIZE tasks. */
> +/*
> + * Loop thru all the tasks in the system and assign ret_stack.
> + * The @ret_stack_list is a small pre-allocated fallback, in case
> + * the GFP_NOWAIT allocations here fail.
> + */
>  static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  {
> -       int i;
> -       int ret = 0;
>         int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
>         struct task_struct *g, *t;
> +       int i, ret = 0;
>
>         if (WARN_ON_ONCE(!fgraph_stack_cachep))
>                 return -ENOMEM;
> @@ -1056,20 +1059,26 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>
>         rcu_read_lock();
>         for_each_process_thread(g, t) {
> -               if (start == end) {
> -                       ret = -EAGAIN;
> -                       goto unlock;
> -               }
> +               unsigned long *rs;
>
> -               if (t->ret_stack == NULL) {
> -                       atomic_set(&t->trace_overrun, 0);
> -                       ret_stack_init_task_vars(ret_stack_list[start]);
> -                       t->curr_ret_stack = 0;
> -                       t->curr_ret_depth = -1;
> -                       /* Make sure the tasks see the 0 first: */
> -                       smp_wmb();
> -                       t->ret_stack = ret_stack_list[start++];
> +               if (t->ret_stack)
> +                       continue;
> +
> +               rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> +               if (!rs) {
> +                       if (start == end) {
> +                               ret = -EAGAIN;
> +                               goto unlock;
> +                       }
> +                       rs = ret_stack_list[start++];
>                 }
> +
> +               atomic_set(&t->trace_overrun, 0);
> +               ret_stack_init_task_vars(rs);
> +               t->curr_ret_stack = 0;
> +               t->curr_ret_depth = -1;
> +               /* Make sure the tasks see the 0 first: */
> +               smp_store_release(&t->ret_stack, rs);
>         }
>
>  unlock:
>
> base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
> --
> 2.53.0-Meta
>

Re: [PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Steven Rostedt]

On Fri, 23 Oct 2026 11:13:44 -0700
Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:

> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> 
> I presume this will go through Masami and Steven's tree, is that right?

Yes, but it came in during the merge window, and since it's just a speed up
and not fixing something that's broken, I was going to let this go in the
next merge window. I rather have a change like this sit through a full -rc
release.

-- Steve

Re: [PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Vineet Gupta]

On 10/23/26 11:38 AM, Steven Rostedt wrote:
> On Fri, 23 Oct 2026 11:13:44 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>>
>> I presume this will go through Masami and Steven's tree, is that right?
> Yes, but it came in during the merge window, and since it's just a speed up
> and not fixing something that's broken, I was going to let this go in the
> next merge window. I rather have a change like this sit through a full -rc
> release.

It is a speed up but not for optimization sake but to cure rcu stalls in 
production at Meta. IMHO it should go in sooner than wait a full release 
cycle.

Thx,
-Vineet

Re: [PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Steven Rostedt]

On Fri, 23 Oct 2026 13:13:41 -0700
Vineet Gupta <vineet.gupta@linux.dev> wrote:

> On 10/23/26 11:38 AM, Steven Rostedt wrote:
> > On Fri, 23 Oct 2026 11:13:44 -0700
> > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >  
> >> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> >>
> >> I presume this will go through Masami and Steven's tree, is that right?  
> > Yes, but it came in during the merge window, and since it's just a speed up
> > and not fixing something that's broken, I was going to let this go in the
> > next merge window. I rather have a change like this sit through a full -rc
> > release.  
> 
> It is a speed up but not for optimization sake but to cure rcu stalls in 
> production at Meta. IMHO it should go in sooner than wait a full release 
> cycle.
> 

Either way, I'd like to give it a bit more testing first. That is, the
earliest it will go is after -rc1.

-- Steve

Re: [PATCH v4 1/2] tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT

[Vineet Gupta]

On 10/23/26 3:09 PM, Steven Rostedt wrote:
> On Fri, 23 Oct 2026 13:13:41 -0700
> Vineet Gupta <vineet.gupta@linux.dev> wrote:
>
>> On 10/23/26 11:38 AM, Steven Rostedt wrote:
>>> On Fri, 23 Oct 2026 11:13:44 -0700
>>> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>>>   
>>>> Acked-by: Andrii Nakryiko <andrii@kernel.org>
>>>>
>>>> I presume this will go through Masami and Steven's tree, is that right?
>>> Yes, but it came in during the merge window, and since it's just a speed up
>>> and not fixing something that's broken, I was going to let this go in the
>>> next merge window. I rather have a change like this sit through a full -rc
>>> release.
>> It is a speed up but not for optimization sake but to cure rcu stalls in
>> production at Meta. IMHO it should go in sooner than wait a full release
>> cycle.
>>
> Either way, I'd like to give it a bit more testing first. That is, the
> earliest it will go is after -rc1.

That's fair.

Thx,
-Vineet

[PATCH v4 2/2] tracing: fgraph: use scoped_guard() for the task list walk

[Vineet Gupta]

alloc_retstack_tasklist() open-codes rcu_read_lock() and unwinds it
through an unlock label. Use scoped_guard(rcu) instead and let the
compiler emit the unlock on every exit path.

The -EAGAIN case now jumps straight to the free label rather than to
unlock; leaving the guard's scope releases the RCU read lock on the
way, so the unlock label has no remaining users and is removed.

No functional change: objdump of fgraph.o shows identical call targets
and counts, including the three __rcu_read_unlock() sites the compiler
emits for the exits from the read-side section. A PROVE_RCU +
DEBUG_ATOMIC_SLEEP build with the -EAGAIN path forced several thousand
times is clean.

The preceding patch deliberately left this alone so that it stays
straightforward to backport. This is the follow-up.

Suggested-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Vineet Gupta <vineet.gupta@linux.dev>
Tested-by: Vineet Gupta <vineet.gupta@linux.dev>
---
 kernel/trace/fgraph.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 4c74f1a15ad4..bbf6e4e2da2d 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -1057,32 +1057,31 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
 		}
 	}
 
-	rcu_read_lock();
-	for_each_process_thread(g, t) {
-		unsigned long *rs;
+	scoped_guard (rcu) {
+		for_each_process_thread(g, t) {
+			unsigned long *rs;
 
-		if (t->ret_stack)
-			continue;
+			if (t->ret_stack)
+				continue;
 
-		rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
-		if (!rs) {
-			if (start == end) {
-				ret = -EAGAIN;
-				goto unlock;
+			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
+			if (!rs) {
+				if (start == end) {
+					ret = -EAGAIN;
+					goto free;
+				}
+				rs = ret_stack_list[start++];
 			}
-			rs = ret_stack_list[start++];
-		}
 
-		atomic_set(&t->trace_overrun, 0);
-		ret_stack_init_task_vars(rs);
-		t->curr_ret_stack = 0;
-		t->curr_ret_depth = -1;
-		/* Make sure the tasks see the 0 first: */
-		smp_store_release(&t->ret_stack, rs);
+			atomic_set(&t->trace_overrun, 0);
+			ret_stack_init_task_vars(rs);
+			t->curr_ret_stack = 0;
+			t->curr_ret_depth = -1;
+			/* Make sure the tasks see the 0 first: */
+			smp_store_release(&t->ret_stack, rs);
+		}
 	}
 
-unlock:
-	rcu_read_unlock();
 free:
 	for (i = start; i < end; i++)
 		kmem_cache_free(fgraph_stack_cachep, ret_stack_list[i]);
-- 
2.53.0-Meta

Re: [PATCH v4 2/2] tracing: fgraph: use scoped_guard() for the task list walk

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] tracing: fgraph: mix of scoped_guard() and goto in alloc_retstack_tasklist()
--

commit 7d59298fba17927bc12b63b0da1c66b0d138332a
Author: Vineet Gupta <vineet.gupta@linux.dev>

tracing: fgraph: use scoped_guard() for the task list walk

This commit replaces the explicit rcu_read_lock() and rcu_read_unlock() pair
in alloc_retstack_tasklist() with scoped_guard(rcu) and removes the unlock
label by jumping to the free label on the -EAGAIN path.

> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 4c74f1a15ad48..bbf6e4e2da2d4 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1057,32 +1057,31 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>  		}
>  	}
>  
> -	rcu_read_lock();
> -	for_each_process_thread(g, t) {
> -		unsigned long *rs;
> +	scoped_guard (rcu) {
> +		for_each_process_thread(g, t) {
> +			unsigned long *rs;
>  
> -		if (t->ret_stack)
> -			continue;
> +			if (t->ret_stack)
> +				continue;
>  
> -		rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> -		if (!rs) {
> -			if (start == end) {
> -				ret = -EAGAIN;
> -				goto unlock;
> +			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
> +			if (!rs) {
> +				if (start == end) {
> +					ret = -EAGAIN;
> +					goto free;
> +				}
> +				rs = ret_stack_list[start++];
>  			}
> -			rs = ret_stack_list[start++];
> -		}
>  
> -		atomic_set(&t->trace_overrun, 0);
> -		ret_stack_init_task_vars(rs);
> -		t->curr_ret_stack = 0;
> -		t->curr_ret_depth = -1;
> -		/* Make sure the tasks see the 0 first: */
> -		smp_store_release(&t->ret_stack, rs);
> +			atomic_set(&t->trace_overrun, 0);
> +			ret_stack_init_task_vars(rs);
> +			t->curr_ret_stack = 0;
> +			t->curr_ret_depth = -1;
> +			/* Make sure the tasks see the 0 first: */
> +			smp_store_release(&t->ret_stack, rs);
> +		}
>  	}
>  
> -unlock:
> -	rcu_read_unlock();
>  free:
>  	for (i = start; i < end; i++)
>  		kmem_cache_free(fgraph_stack_cachep, ret_stack_list[i]);

[Severity: Low]
In kernel/trace/fgraph.c, alloc_retstack_tasklist() now uses
scoped_guard(rcu) while still jumping out of the guarded scope via goto free
when kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT) fails and start == end,
as well as jumping to free from the earlier pre-allocation loop:

kernel/trace/fgraph.c:alloc_retstack_tasklist() {
	for (i = 0; i < FTRACE_RETSTACK_ALLOC_SIZE; i++) {
		ret_stack_list[i] = kmem_cache_alloc(fgraph_stack_cachep, GFP_KERNEL);
		if (!ret_stack_list[i]) {
			start = 0;
			end = i;
			ret = -ENOMEM;
			goto free;
		}
	}
}

The documentation in include/linux/cleanup.h notes that goto-based cleanup
and scope-based cleanup helpers should not be mixed within the same function:

 * Lastly, given that the benefit of cleanup helpers is removal of
 * "goto", and that the "goto" statement can jump between scopes, the
 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function. I.e. for a given routine, convert all
 * resources that need a "goto" cleanup to scope-based cleanup, or
 * convert none of them.

Also, on the -EAGAIN path inside scoped_guard(rcu), start == end is already
true because all pre-allocated entries in ret_stack_list[] have been consumed,
so the cleanup loop at the free label runs zero iterations on that path.

Should alloc_retstack_tasklist() avoid mixing scoped_guard(rcu) with goto
free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021174406.2891598-1-vineet.gupta@linux.dev?part=2

Re: [PATCH v4 2/2] tracing: fgraph: use scoped_guard() for the task list walk

[Vineet Gupta]

On 10/21/26 10:53 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] tracing: fgraph: mix of scoped_guard() and goto in alloc_retstack_tasklist()
> --
>
> commit 7d59298fba17927bc12b63b0da1c66b0d138332a
> Author: Vineet Gupta <vineet.gupta@linux.dev>
>
> tracing: fgraph: use scoped_guard() for the task list walk
>
> This commit replaces the explicit rcu_read_lock() and rcu_read_unlock() pair
> in alloc_retstack_tasklist() with scoped_guard(rcu) and removes the unlock
> label by jumping to the free label on the -EAGAIN path.
>
>> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
>> index 4c74f1a15ad48..bbf6e4e2da2d4 100644
>> --- a/kernel/trace/fgraph.c
>> +++ b/kernel/trace/fgraph.c
>> @@ -1057,32 +1057,31 @@ static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>>   		}
>>   	}
>>   
>> -	rcu_read_lock();
>> -	for_each_process_thread(g, t) {
>> -		unsigned long *rs;
>> +	scoped_guard (rcu) {
>> +		for_each_process_thread(g, t) {
>> +			unsigned long *rs;
>>   
>> -		if (t->ret_stack)
>> -			continue;
>> +			if (t->ret_stack)
>> +				continue;
>>   
>> -		rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
>> -		if (!rs) {
>> -			if (start == end) {
>> -				ret = -EAGAIN;
>> -				goto unlock;
>> +			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);
>> +			if (!rs) {
>> +				if (start == end) {
>> +					ret = -EAGAIN;
>> +					goto free;
>> +				}
>> +				rs = ret_stack_list[start++];
>>   			}
>> -			rs = ret_stack_list[start++];
>> -		}
>>   
>> -		atomic_set(&t->trace_overrun, 0);
>> -		ret_stack_init_task_vars(rs);
>> -		t->curr_ret_stack = 0;
>> -		t->curr_ret_depth = -1;
>> -		/* Make sure the tasks see the 0 first: */
>> -		smp_store_release(&t->ret_stack, rs);
>> +			atomic_set(&t->trace_overrun, 0);
>> +			ret_stack_init_task_vars(rs);
>> +			t->curr_ret_stack = 0;
>> +			t->curr_ret_depth = -1;
>> +			/* Make sure the tasks see the 0 first: */
>> +			smp_store_release(&t->ret_stack, rs);
>> +		}
>>   	}
>>   
>> -unlock:
>> -	rcu_read_unlock();
>>   free:
>>   	for (i = start; i < end; i++)
>>   		kmem_cache_free(fgraph_stack_cachep, ret_stack_list[i]);
> [Severity: Low]
> In kernel/trace/fgraph.c, alloc_retstack_tasklist() now uses
> scoped_guard(rcu) while still jumping out of the guarded scope via goto free
> when kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT) fails and start == end,
> as well as jumping to free from the earlier pre-allocation loop:
>
> kernel/trace/fgraph.c:alloc_retstack_tasklist() {
> 	for (i = 0; i < FTRACE_RETSTACK_ALLOC_SIZE; i++) {
> 		ret_stack_list[i] = kmem_cache_alloc(fgraph_stack_cachep, GFP_KERNEL);
> 		if (!ret_stack_list[i]) {
> 			start = 0;
> 			end = i;
> 			ret = -ENOMEM;
> 			goto free;
> 		}
> 	}
> }
>
> The documentation in include/linux/cleanup.h notes that goto-based cleanup
> and scope-based cleanup helpers should not be mixed within the same function:
>
>   * Lastly, given that the benefit of cleanup helpers is removal of
>   * "goto", and that the "goto" statement can jump between scopes, the
>   * expectation is that usage of "goto" and cleanup helpers is never
>   * mixed in the same function. I.e. for a given routine, convert all
>   * resources that need a "goto" cleanup to scope-based cleanup, or
>   * convert none of them.
>
> Also, on the -EAGAIN path inside scoped_guard(rcu), start == end is already
> true because all pre-allocated entries in ret_stack_list[] have been consumed,
> so the cleanup loop at the free label runs zero iterations on that path.
>
> Should alloc_retstack_tasklist() avoid mixing scoped_guard(rcu) with goto
> free?

This has been discussed before and it was agreed this is fine.

Thx,
-Vineet

Re: [PATCH v4 0/2] tracing: fgraph: fix the quadratic shadow stack walk

[Alexei Starovoitov]

On Wed, Oct 21, 2026 at 10:44 AM Vineet Gupta <vineet.gupta@linux.dev> wrote:
> On a 60-core Sapphire Rapids machine with 400000 idle threads it took
> 227 s, inside a single bpf() syscall for the kprobe_multi case.

Acked-by: Alexei Starovoitov <ast@kernel.org>