On Thu, 24 Oct 2024 21:05:15 -0400 Steven Rostedt <[email protected]> wrote:
> On Fri, 25 Oct 2024 00:21:21 +0900 > Masami Hiramatsu (Google) <[email protected]> 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. OK, then let it goes with this in this version. > > 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. It is not buggy as far as I reviewed, just concerned about the performance overhead. So, Reviewed-by: Masami Hiramatsu (Google) <[email protected]> Thank you, > > -- Steve -- Masami Hiramatsu (Google) <[email protected]>
