Re: [PATCHv5 bpf-next 6/9] ftrace: Add update_ftrace_direct_mod function

Jiri Olsa
2025-12-19T09:27:54Z

On Thu, Dec 18, 2025 at 10:19:42AM -0500, Steven Rostedt wrote:
> On Mon, 15 Dec 2025 22:13:59 +0100
> Jiri Olsa <jolsa@kernel.org> wrote:
>
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 48dc0de5f2ce..95a38fb18ed7 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6489,6 +6489,78 @@ int update_ftrace_direct_del(struct ftrace_ops *ops, struct ftrace_hash *hash)
> > return err;
> > }
> >
>
> Kerneldoc needed.

will add

>
> > +int update_ftrace_direct_mod(struct ftrace_ops *ops, struct ftrace_hash *hash, bool do_direct_lock)
> > +{
> > + struct ftrace_func_entry *entry, *tmp;
> > + static struct ftrace_ops tmp_ops = {
> > + .func = ftrace_stub,
> > + .flags = FTRACE_OPS_FL_STUB,
> > + };
> > + struct ftrace_hash *orig_hash;
> > + unsigned long size, i;
> > + int err = -EINVAL;
> > +
> > + if (!hash_count(hash))
> > + return -EINVAL;
> > + if (check_direct_multi(ops))
> > + return -EINVAL;
> > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > + return -EINVAL;
> > + if (direct_functions == EMPTY_HASH)
> > + return -EINVAL;
> > +
> > + if (do_direct_lock)
> > + mutex_lock(&direct_mutex);
>
> This optional taking of the direct_mutex lock needs some serious rationale
> and documentation.

it mirrors the use of modify_ftrace_direct/modify_ftrace_direct_nolock
when we do trampoline update from within ftrace_ops->ops_func callback

I'll add comments with more details

>
> > +
> > + orig_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> > + if (!orig_hash)
> > + goto unlock;
> > +
> > + /* Enable the tmp_ops to have the same functions as the direct ops */
> > + ftrace_ops_init(&tmp_ops);
> > + tmp_ops.func_hash = ops->func_hash;
> > +
> > + err = register_ftrace_function_nolock(&tmp_ops);
> > + if (err)
> > + goto unlock;
> > +
> > + /*
> > + * Call __ftrace_hash_update_ipmodify() here, so that we can call
> > + * ops->ops_func for the ops. This is needed because the above
> > + * register_ftrace_function_nolock() worked on tmp_ops.
> > + */
> > + err = __ftrace_hash_update_ipmodify(ops, orig_hash, orig_hash, true);
> > + if (err)
> > + goto out;
> > +
> > + /*
> > + * Now the ftrace_ops_list_func() is called to do the direct callers.
> > + * We can safely change the direct functions attached to each entry.
> > + */
> > + mutex_lock(&ftrace_lock);
>
> I'm going to need some time staring at this code. It looks like it may be
> relying on some internals here.

ok, thanks

jirka