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 <[email protected]> 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/[email protected]?part=2
