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

[Vineet Gupta <vineet.gupta@linux.dev>]

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