Re: [PATCH 2/2] fgraph: Free ret_stack when task is done with it

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 25 Oct 2024 00:21:21 +0900
Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote:

> > +static void fgraph_ret_stack_work_func(struct work_struct *work)
> > +{
> > +	mutex_lock(&ftrace_lock);
> > +	if (!ftrace_graph_active)
> > +		free_ret_stacks();
> > +	mutex_unlock(&ftrace_lock);
> > +}  
> 
> Hmm, will you scan all tasks everytime? Shouldn't we have another global
> list of skipped tasks in remove_ret_stack(), like below?
> 
> static void remove_ret_stack(struct task_struct *t, struct list_head *freelist, struct list_head *skiplist, int list_index)
> {
> 	struct ret_stack_free_data *free_data;
> 	struct list_head *head;
> 
> 	/* If the ret_stack is still in use, skip this */
> 	if (t->curr_ret_depth >= 0)
> 		head = skiplist;
> 	else
> 		head = freelist;
> 
> 	free_data = (struct ret_stack_free_data*)(t->ret_stack + list_index);
> 	list_add(&free_data->list, head);
> 	free_data->task = t;
> }
> 
> Then we can scan only skiplist in free_ret_stacks() in fgraph_ret_stack_work_func().
> 
> Of course this will need to decouple preparing freelist/skiplist and
> actual free function.

I thought about doing it this way, but I felt that it made the code
more complex with little benefit. Yeah, we scan all tasks, but it only
happens in a work queue that is grabbing the ftrace_lock mutex. If
anything, I rather keep it this way and if it ends up being an issue we
can change it later.

One thing Thomas always says is "correctness first, optimize later".
This is much easier to get correct. Adding a skip list will add
complexity. Like I said, nothing prevents us from adding that feature
later, and if it ends up buggy, we can know which change caused the bug.

-- Steve