Re: [PATCH 1/2] tracing: fgraph: Raise FTRACE_RETSTACK_ALLOC_SIZE to 1024

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

On 9/24/26 3:00 AM, Peter Zijlstra wrote:
> On Tue, Sep 22, 2026 at 03:55:25PM -0700, Vineet Gupta wrote:
>> When ftrace graphing is turned on, all tasks in the system missing
>> return stack page are assigned one. This is done in a simplistic
>> multi-sweep loop of FTRACE_RETSTACK_ALLOC_SIZE (currently 32) tasks
>> at a time as follows:
>>
>>     start_graph_tracing()
>> 	do {
>> 		alloc_retstack_tasklist
>> 	} while (-EAGAIN);
>>
>>     alloc_retstack_tasklist()
>> 	alloc x32           # GFP_KERNEL, may sleep
>> 	rcu_read_lock()     # preempt off
>> 		for_each_process_thread    walk N_total, no cond_resched
>>                       t->ret_stack = new_page
>> 	rcu_read_unlock()   # preempt enable but no explicit yield
>>
>> Each successive iteration of loop invokes for_each_process_thread()
>> which doesn't support cursor based resume and always restarts from the
>> init_task. Thus each successive loop needs to skip the tasks assigned
>> ret_stack in prior sweeps and thus take longer and longer to find the
>> candidate 32 tasks.
> Does this work?

Yes it does and much better at that: test run for 400k threads went down 
from 227 s (stock) to 7.2 s (my patch) to 92 ms. Woo hoo !
I didn't know about this cool trick.

A couple of things worth pointing out:


> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index ed455b53513b..155dafad474d 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -1036,10 +1036,9 @@ trace_func_graph_ent_t ftrace_graph_entry = ftrace_graph_entry_stub;
>   /* Try to assign a return stack array on FTRACE_RETSTACK_ALLOC_SIZE tasks. */
>   static int alloc_retstack_tasklist(unsigned long **ret_stack_list)
>   {
> -	int i;
> -	int ret = 0;
>   	int start = 0, end = FTRACE_RETSTACK_ALLOC_SIZE;
>   	struct task_struct *g, *t;
> +	int i, ret = 0;
>   
>   	if (WARN_ON_ONCE(!fgraph_stack_cachep))
>   		return -ENOMEM;
> @@ -1054,26 +1053,29 @@ 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) {
> +		for_each_process_thread(g, t) {
> +			unsigned long *rs;
> +
> +			if (t->ret_stack)
> +				continue;
> +
> +			rs = kmem_cache_alloc(fgraph_stack_cachep, GFP_NOWAIT);

It still pre-allocates FTRACE_RETSTACK_ALLOC_SIZE reserves in the fast 
path - my (limited) testing never hit the reserves, but I agree better 
to keep it and 32 seems like a reasonably low number?
> +			if (!rs) {
> +				if (start == end)
> +					return -EAGAIN;

This indicates the reserve pool is exhausted so it safe to elide the 
free: loop at the end: warrants a comment IMO.

For submission, would you take this up as reported-by me or do you 
prefer I send a v2 (your authorship and SoB etc) ? No big deal either 
ways just not sure what the std operating procedure is and nuances of 
all the tags etc.

Thx,
-Vineet

> +				rs = ret_stack_list[start++];
> +			}
>   
> -		if (t->ret_stack == NULL) {
>   			atomic_set(&t->trace_overrun, 0);
> -			ret_stack_init_task_vars(ret_stack_list[start]);
> +			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_wmb();
> -			t->ret_stack = ret_stack_list[start++];
> +			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]);