On Mon, Sep 28, 2026 at 07:25:00PM -0700, Andrii Nakryiko wrote: > On Mon, Sep 28, 2026 at 6:22 PM Vineet Gupta <[email protected]> wrote: > > > > On 9/28/26 6:03 PM, [email protected] 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 <[email protected]> > > > 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.
