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
Thx,
-Vineet
