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

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Sep 28, 2026 at 07:25:00PM -0700, Andrii Nakryiko wrote:
> On Mon, Sep 28, 2026 at 6:22 PM Vineet Gupta <vineet.gupta@linux.dev> wrote:
> >
> > On 9/28/26 6:03 PM, sashiko-bot@kernel.org wrote:
> > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > - [Low] cleanup: mixing goto-based error handling and scope-based cleanup
> > > --
> > >
> > > commit a6de528fef00ef67da15965a7a8b3944fa92b053
> > > Author: Vineet Gupta <vineet.gupta@linux.dev>
> > > Subject: tracing: fgraph: allocate shadow stacks inline with GFP_NOWAIT
> > >
> > > This commit aims to resolve performance regressions with ftrace graph tracing
> > > when there are a large number of tasks. It replaces the pre-allocated batch
> > > approach with inline allocation using GFP_NOWAIT, avoiding repeated sweeps of
> > > the task list. Additionally, it transitions the RCU read-side section to use
> > > scoped_guard(rcu) instead of manual rcu_read_lock and rcu_read_unlock calls.
> > >
> > >> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> > >> index 40d373d65f9b9..1c5ccc3f80d53 100644
> > >> --- a/kernel/trace/fgraph.c
> > >> +++ b/kernel/trace/fgraph.c
> > > [ ... ]
> > >> @@ -1057,26 +1056,37 @@ 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;
> > >> -            }
> > >> +    scoped_guard (rcu) {
> > > [Severity: Low]
> > > Does this mix goto-based error handling and scope-based cleanup within the same
> > > function?
> > >
> > > Looking at alloc_retstack_tasklist(), the function retains the legacy goto free
> > > path in the initial pre-allocation loop (just before this block), while
> > > introducing scoped_guard(rcu) here.
> > >
> > > The cleanup subsystem guidelines recommend that for a given routine, we should
> > > convert all resources that need a goto cleanup to scope-based cleanup, or
> > > convert none of them. Mixing these paradigms creates confusing ownership
> > > semantics and is highly prone to double-free or resource leak regressions upon
> > > future modifications.
> > >
> > > Could we either convert the ret_stack_list array cleanup to use scope-based
> > > helpers, or stick to the manual rcu_read_lock() and rcu_read_unlock() here to
> > > keep the error handling style consistent?
> >
> > And a third option is to drop the fallback allocation completely and
> > return -EAGAIN and come back. Granted I don't have much experience of
> > typical handling of GFP_NOWAIT fails and retrying immediately: would
> > that recover at all or does that take us back to where we started ? Peter
> >
> 
> Not Peter, but I don't see a problem with scoped rcu and goto-based
> free/cleanup. Unless Peter objects, let's keep it as is?

Yeah, the silly robot is being silly. Code is fine as is.

The guideline is just that, a guide. Its not a hard requirement. It is
possible to create a terrible mess of things when doing a partial
conversion of a large multi-stage goto unwind fest. But in general, goto
over the scope (as here) is fine. Also goto out of a scope is also fine.