On Thu, Jul 3, 2025 at 11:30 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Thu, 3 Jul 2025 20:15:07 +0800
> Menglong Dong <menglong8.dong@xxxxxxxxx> wrote:
>
> Note, the tracing subsystem uses capitalization in the subject:
>
> ftrace: Add reset_ftrace_direct_ips
Hi, Steven. Thanks for your feedback. I'll keep this point
in mind. I was wondering why Alexei changed the "make" to "Make"
in c11f34e30088 :/
>
>
> > For now, we can change the address of a direct ftrace_ops with
> > modify_ftrace_direct(). However, we can't change the functions to filter
> > for a direct ftrace_ops. Therefore, we introduce the function
> > reset_ftrace_direct_ips() to do such things, and this function will reset
> > the functions to filter for a direct ftrace_ops.
> >
> > This function do such thing in following steps:
> >
> > 1. filter out the new functions from ips that don't exist in the
> > ops->func_hash->filter_hash and add them to the new hash.
> > 2. add all the functions in the new ftrace_hash to direct_functions by
> > ftrace_direct_update().
> > 3. reset the functions to filter of the ftrace_ops to the ips with
> > ftrace_set_filter_ips().
> > 4. remove the functions that in the old ftrace_hash, but not in the new
> > ftrace_hash from direct_functions.
>
> Please also include a module that can be loaded for testing.
> See samples/ftrace/ftrace-direct*
>
> But make it a separate patch. And you'll need to add a test in selftests.
> See tools/testing/selftests/ftrace/test.d/direct
Okay!
>
> >
> > Signed-off-by: Menglong Dong <dongml2@xxxxxxxxxxxxxxx>
> > ---
> > include/linux/ftrace.h | 7 ++++
> > kernel/trace/ftrace.c | 75 ++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 82 insertions(+)
> >
> > diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> > index b672ca15f265..b7c60f5a4120 100644
> > --- a/include/linux/ftrace.h
> > +++ b/include/linux/ftrace.h
> > @@ -528,6 +528,8 @@ int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned long addr);
> >
> > void ftrace_stub_direct_tramp(void);
> >
> > +int reset_ftrace_direct_ips(struct ftrace_ops *ops, unsigned long *ips,
> > + unsigned int cnt);
> > #else
> > struct ftrace_ops;
> > static inline unsigned long ftrace_find_rec_direct(unsigned long ip)
> > @@ -551,6 +553,11 @@ static inline int modify_ftrace_direct_nolock(struct ftrace_ops *ops, unsigned l
> > {
> > return -ENODEV;
> > }
> > +static inline int reset_ftrace_direct_ips(struct ftrace_ops *ops, unsigned long *ips,
> > + unsigned int cnt)
> > +{
> > + return -ENODEV;
> > +}
> >
> > /*
> > * This must be implemented by the architecture.
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index f5f6d7bc26f0..db3aa61889d3 100644
> > --- a/kernel/trace/ftrace.c
> > +++ b/kernel/trace/ftrace.c
> > @@ -6224,6 +6224,81 @@ int modify_ftrace_direct(struct ftrace_ops *ops, unsigned long addr)
> > return err;
> > }
> > EXPORT_SYMBOL_GPL(modify_ftrace_direct);
> > +
> > +/* reset the ips for a direct ftrace (add or remove) */
>
> As this function is being used externally, it requires proper KernelDoc
> headers.
Okay!
>
> What exactly do you mean by "reset"?
It means to reset the filter hash of the ftrace_ops to ips. In
the origin logic, the filter hash of a direct ftrace_ops will not
be changed. However, in the tracing-multi case, there are
multi functions in the filter hash and can change. This function
is used to change the filter hash of a direct ftrace_ops.
>
> > +int reset_ftrace_direct_ips(struct ftrace_ops *ops, unsigned long *ips,
> > + unsigned int cnt)
> > +{
> > + struct ftrace_hash *hash, *free_hash;
> > + struct ftrace_func_entry *entry, *del;
> > + unsigned long ip;
> > + int err, size;
> > +
> > + if (check_direct_multi(ops))
> > + return -EINVAL;
> > + if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> > + return -EINVAL;
> > +
> > + mutex_lock(&direct_mutex);
> > + hash = alloc_ftrace_hash(FTRACE_HASH_DEFAULT_BITS);
> > + if (!hash) {
> > + err = -ENOMEM;
> > + goto out_unlock;
> > + }
> > +
> > + /* find out the new functions from ips and add to hash */
>
> Capitalize comment: /* Find out ...
>
> > + for (int i = 0; i < cnt; i++) {
> > + ip = ftrace_location(ips[i]);
> > + if (!ip) {
> > + err = -ENOENT;
> > + goto out_unlock;
> > + }
> > + if (__ftrace_lookup_ip(ops->func_hash->filter_hash, ip))
> > + continue;
> > + err = __ftrace_match_addr(hash, ip, 0);
> > + if (err)
> > + goto out_unlock;
> > + }
> > +
> > + free_hash = direct_functions;
>
> Add newline.
>
> > + /* add the new ips to direct hash. */
>
> Again capitalize.
>
> > + err = ftrace_direct_update(hash, ops->direct_call);
> > + if (err)
> > + goto out_unlock;
> > +
> > + if (free_hash && free_hash != EMPTY_HASH)
> > + call_rcu_tasks(&free_hash->rcu, register_ftrace_direct_cb);
>
> Since the above is now used more than once, let's make it into a helper
> function so that if things change, there's only one place to change it:
>
> free_ftrace_direct(free_hash);
>
> static inline void free_ftrace_direct(struct ftrace_hash *hash)
> {
> if (hash && hash != EMPTY_HASH)
> call_rcu_tasks(&free_hash->rcu, register_ftrace_direct_cb);
> }
Sounds nice~
>
> > +
> > + free_ftrace_hash(hash);
> > + hash = alloc_and_copy_ftrace_hash(FTRACE_HASH_DEFAULT_BITS,
> > + ops->func_hash->filter_hash);
> > + if (!hash) {
> > + err = -ENOMEM;
> > + goto out_unlock;
> > + }
> > + err = ftrace_set_filter_ips(ops, ips, cnt, 0, 1);
> > +
> > + /* remove the entries that don't exist in our filter_hash anymore
> > + * from the direct_functions.
> > + */
>
> This isn't the network subsystem, we use the default comment style for multiple lines:
>
> /*
> * line 1
> * line 2
> * ...
> */
Okay! I'll do the modification as your comment in this (and other)
patches.
Thanks!
Menglong Dong
>
> -- Steve
>
> > + size = 1 << hash->size_bits;
> > + for (int i = 0; i < size; i++) {
> > + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> > + if (__ftrace_lookup_ip(ops->func_hash->filter_hash, entry->ip))
> > + continue;
> > + del = __ftrace_lookup_ip(direct_functions, entry->ip);
> > + if (del && del->direct == ops->direct_call) {
> > + remove_hash_entry(direct_functions, del);
> > + kfree(del);
> > + }
> > + }
> > + }
> > +out_unlock:
> > + mutex_unlock(&direct_mutex);
> > + if (hash)
> > + free_ftrace_hash(hash);
> > + return err;
> > +}
> > #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
> >
> > /**
>
