Steven Rostedt <[email protected]> 于2026年11月2日周一 09:22写道： > > On Sat, 31 Oct 2026 10:52:13 -0700 > Weiming Shi <[email protected]> wrote: > > > ftrace_shutdown_subops() unlinks a subop before rebuilding the manager > > hashes, but leaves it enabled and managed when rebuilding or updating > > fails. unregister_ftrace_graph() cannot recover from that partial > > teardown, and a later registration can corrupt the empty manager state. > > > > Always detach and clear the subop once shutdown begins. Keep conservative > > manager hashes behind per-subop filtering when an update fails, and disable > > direct graph dispatch until those hashes are exact again. If the last > > manager cannot be shut down, disable ftrace and normalize its list, flags, > > count, and hashes before the caller releases the graph operation. > > What exactly is the above saying? It looks like pure AI output (non human > readable). AKA, AI slop! > > Is it fixing something when a subop is unregistered but fails? Where > exactly is the failure? What failed? Have an example of what happened? > > Direct calls is mentioned once, but there seems to be a bit of changes for > that too. Some of it looks like it should be a separate patch. > > > > > Cc: [email protected] > > Fixes: 0ae6b8ce200d ("ftrace: Fix accounting of subop hashes") > > Reported-by: Xiang Mei <[email protected]> > > Assisted-by: OpenAI Codex <[email protected]> > > Signed-off-by: Weiming Shi <[email protected]> > > --- > > kernel/trace/fgraph.c | 35 +++++++++++++++++---- > > kernel/trace/ftrace.c | 57 +++++++++++++++++++++------------- > > kernel/trace/ftrace_internal.h | 3 ++ > > 3 files changed, 68 insertions(+), 27 deletions(-) > > > > diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c > > index ed455b535..1a8e092ec 100644 > > --- a/kernel/trace/fgraph.c > > +++ b/kernel/trace/fgraph.c > > @@ -1306,7 +1306,7 @@ static void ftrace_graph_enable_direct(bool > > enable_branch, struct fgraph_ops *go > > > > static_call_update(fgraph_func, func); > > static_call_update(fgraph_retfunc, retfunc); > > - if (enable_branch) > > + if (enable_branch && !static_key_enabled(&fgraph_do_direct.key)) > > static_branch_enable(&fgraph_do_direct); > > Ug that is horrible. If anything static_branch_enable() should handle this. > Or we have to have another type of accounting. > > > > } > > > > @@ -1315,13 +1315,27 @@ static void ftrace_graph_disable_direct(bool > > disable_branch) > > if (FGRAPH_NO_DIRECT) > > return; > > > > - if (disable_branch) > > + if (disable_branch && static_key_enabled(&fgraph_do_direct.key)) > > static_branch_disable(&fgraph_do_direct); > > Same. > > > static_call_update(fgraph_func, ftrace_graph_entry_stub); > > static_call_update(fgraph_retfunc, ftrace_graph_ret_stub); > > fgraph_direct_gops = &fgraph_stub; > > } > > > > +void fgraph_update_direct_func(struct ftrace_ops *ops, bool enable) > > What is this for? It requires a kerneldoc as it is not a static function. > > It also looks unrelated to the partial shutdown. Is his needed as a > separate patch? > > > +{ > > + struct fgraph_ops *gops; > > + > > + if (ftrace_graph_active != 1 || !(ops->flags & FTRACE_OPS_FL_GRAPH)) > > + return; > > + > > + gops = container_of(ops, struct fgraph_ops, ops); > > + if (enable) > > + ftrace_graph_enable_direct(true, gops); > > + else > > + ftrace_graph_disable_direct(true); > > +} > > + > > /* The cpu_boot init_task->ret_stack will never be freed */ > > static int fgraph_cpu_init(unsigned int cpu) > > { > > @@ -1406,16 +1420,24 @@ int register_ftrace_graph(struct fgraph_ops *gops) > > gops->ops.flags |= FTRACE_OPS_FL_GRAPH; > > > > ret = ftrace_startup_subops(&graph_ops, &gops->ops, command); > > - if (!ret) > > + if (!ret) { > > fgraph_array[i] = gops; > > + if (ftrace_graph_active == 1) > > + ftrace_graph_enable_direct(true, gops); > > + } > > > > error: > > if (ret) { > > ftrace_graph_active--; > > + gops->ops.flags &= ~FTRACE_OPS_FL_GRAPH; > > Hmm, clearing the flag looks like a separate bug that should be fixed in > its own patch. > > > > gops->saved_func = NULL; > > fgraph_lru_release_index(i); > > - if (!ftrace_graph_active) > > + if (ftrace_graph_active == 1) { > > + ftrace_graph_enable_direct(true, NULL); > > + } else if (!ftrace_graph_active) { > > + ftrace_graph_disable_direct(false); > > unregister_pm_notifier(&ftrace_suspend_notifier); > > + } > > Nit, but I would have the above opposite. That is, don't make the current > if statement the else, unless there's a good reason to. Because it makes > reviewing harder. > > if (!ftrace_graph_active) { > ftrace_graph_disable_direct(false); > unregister_pm_notifier(&ftrace_suspend_notifier); > } else if (ftrace_graph_active == 1) { > ftrace_graph_enable_direct(true, NULL); > } > > > > } > > return ret; > > } > > @@ -1423,6 +1445,7 @@ int register_ftrace_graph(struct fgraph_ops *gops) > > void unregister_ftrace_graph(struct fgraph_ops *gops) > > { > > int command = 0; > > + int ret; > > > > if (WARN_ONCE(!(gops->ops.flags & FTRACE_OPS_FL_GRAPH), > > "function graph ops unregistered without registering")) > > @@ -1447,9 +1470,9 @@ void unregister_ftrace_graph(struct fgraph_ops *gops) > > if (!ftrace_graph_active) > > command = FTRACE_STOP_FUNC_RET; > > > > - ftrace_shutdown_subops(&graph_ops, &gops->ops, command); > > + ret = ftrace_shutdown_subops(&graph_ops, &gops->ops, command); > > > > - if (ftrace_graph_active == 1) > > + if (ftrace_graph_active == 1 && !ret) > > ftrace_graph_enable_direct(true, NULL); > > else if (!ftrace_graph_active) > > ftrace_graph_disable_direct(false); > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index 673a54fdf..831f04de6 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -3667,6 +3667,8 @@ static int rebuild_hashes(struct ftrace_hash > > **filter_hash, struct ftrace_hash * > > bool first = true; > > int ret; > > > > + *filter_hash = EMPTY_HASH; > > + *notrace_hash = EMPTY_HASH; > > temp_hash.filter_hash = EMPTY_HASH; > > temp_hash.notrace_hash = EMPTY_HASH; > > > > @@ -3708,7 +3710,13 @@ static int rebuild_hashes(struct ftrace_hash > > **filter_hash, struct ftrace_hash * > > * will not affect functions that are being traced by other subops that > > * still exist in @ops. > > * > > - * If the last subops is removed from @ops, then @ops is shutdown normally. > > + * Once removal starts, @subops is detached even if rebuilding the manager > > + * hashes fails. The old manager hashes are conservative in that case and > > the > > + * remaining subops continue to filter callbacks. If the last-manager > > shutdown > > + * fails, ftrace is disabled before the manager state is discarded. > > + * > > + * Return: 0 on success or a negative error code if the manager could not > > be > > + * updated or shut down. > > */ > > int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops > > *subops, int command) > > { > > @@ -3716,9 +3724,6 @@ int ftrace_shutdown_subops(struct ftrace_ops *ops, > > struct ftrace_ops *subops, in > > struct ftrace_hash *notrace_hash = EMPTY_HASH; > > int ret; > > > > - if (unlikely(ftrace_disabled)) > > - return -ENODEV; > > Wait! Why this??? > > > - > > if (WARN_ON_ONCE(!(subops->flags & FTRACE_OPS_FL_ENABLED))) > > return -EINVAL; > > > > @@ -3727,36 +3732,45 @@ int ftrace_shutdown_subops(struct ftrace_ops *ops, > > struct ftrace_ops *subops, in > > if (list_empty(&ops->subop_list)) { > > /* Last one, just disable the current ops */ > > > > - ret = ftrace_shutdown(ops, command); > > + if (unlikely(ftrace_disabled)) > > + ret = -ENODEV; > > + else > > + ret = ftrace_shutdown(ops, command); > > Ug no! > > > if (ret < 0) { > > - list_add(&subops->list, &ops->subop_list); > > - return ret; > > + bool enabled = ops->flags & FTRACE_OPS_FL_ENABLED; > > + > > + FTRACE_WARN_ON_ONCE(!ftrace_disabled); > > + remove_ftrace_ops(&ftrace_ops_list, ops); > > + if (enabled && ftrace_start_up > 0) > > + ftrace_start_up--; > > + ops->flags &= ~(FTRACE_OPS_FL_ENABLED | > > + FTRACE_OPS_FL_ADDING | > > + FTRACE_OPS_FL_REMOVING); > > + ops->func = ops->saved_func; > > Please explain the above. > > > } > > > > - subops->flags &= ~FTRACE_OPS_FL_ENABLED; > > - > > free_ftrace_hash(ops->func_hash->filter_hash); > > free_ftrace_hash(ops->func_hash->notrace_hash); > > ops->func_hash->filter_hash = EMPTY_HASH; > > ops->func_hash->notrace_hash = EMPTY_HASH; > > - subops->flags &= ~(FTRACE_OPS_FL_ENABLED | > > FTRACE_OPS_FL_SUBOP); > > - subops->managed = NULL; > > + goto out; > > + } > > > > - return 0; > > + if (unlikely(ftrace_disabled)) { > > + ret = -ENODEV; > > + goto out; > > } > > > > /* Rebuild the hashes without subops */ > > ret = rebuild_hashes(&filter_hash, ¬race_hash, ops); > > if (ret < 0) > > - return ret; > > + goto out; > > > > ret = ftrace_update_ops(ops, filter_hash, notrace_hash); > > - if (ret < 0) { > > - list_add(&subops->list, &ops->subop_list); > > - } else { > > - subops->flags &= ~(FTRACE_OPS_FL_ENABLED | > > FTRACE_OPS_FL_SUBOP); > > - subops->managed = NULL; > > - } > > + > > +out: > > + subops->flags &= ~(FTRACE_OPS_FL_ENABLED | FTRACE_OPS_FL_SUBOP); > > + subops->managed = NULL; > > > The above is doing a lot without really explaining why. > > -- Steve > > > free_ftrace_hash(filter_hash); > > free_ftrace_hash(notrace_hash); > > return ret; > > @@ -3767,8 +3781,8 @@ static int ftrace_hash_move_and_update_subops(struct > > ftrace_ops *subops, > > struct ftrace_hash *hash) > > { > > struct ftrace_ops *ops = subops->managed; > > - struct ftrace_hash *notrace_hash; > > - struct ftrace_hash *filter_hash; > > + struct ftrace_hash *notrace_hash = EMPTY_HASH; > > + struct ftrace_hash *filter_hash = EMPTY_HASH; > > struct ftrace_hash *save_hash; > > struct ftrace_hash *new_hash; > > int ret; > > @@ -3800,6 +3814,7 @@ static int ftrace_hash_move_and_update_subops(struct > > ftrace_ops *subops, > > } else { > > free_ftrace_hash_rcu(save_hash); > > } > > + fgraph_update_direct_func(subops, !ret); > > return ret; > > } > > > > diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h > > index 3235470e6..3d5934b1c 100644 > > --- a/kernel/trace/ftrace_internal.h > > +++ b/kernel/trace/ftrace_internal.h > > @@ -16,6 +16,7 @@ int ftrace_startup(struct ftrace_ops *ops, int command); > > int ftrace_shutdown(struct ftrace_ops *ops, int command); > > int ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs); > > int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops > > *subops, int command); > > +/* Once started, shutdown always detaches @subops, even when it returns an > > error. */ > > int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops > > *subops, int command); > > > > #else /* !CONFIG_DYNAMIC_FTRACE */ > > @@ -52,6 +53,7 @@ static inline int ftrace_shutdown_subops(struct > > ftrace_ops *ops, struct ftrace_o > > > > #ifdef CONFIG_FUNCTION_GRAPH_TRACER > > extern int ftrace_graph_active; > > +void fgraph_update_direct_func(struct ftrace_ops *ops, bool enable); > > # ifdef CONFIG_DYNAMIC_FTRACE > > extern void fgraph_update_pid_func(void); > > # else > > @@ -59,6 +61,7 @@ static inline void fgraph_update_pid_func(void) {} > > # endif > > #else /* !CONFIG_FUNCTION_GRAPH_TRACER */ > > # define ftrace_graph_active 0 > > +static inline void fgraph_update_direct_func(struct ftrace_ops *ops, bool > > enable) {} > > static inline void fgraph_update_pid_func(void) {} > > #endif /* CONFIG_FUNCTION_GRAPH_TRACER */ > > >
Hi Steven, Please disregard the v3 patch, as it was sent by mistake. I am still working on a new patch. There are some issues that I am currently investigating, and I will send an updated version once they are resolved. I apologize again for the confusion and for any inconvenience this may have caused. Best regards, Weiming
