On Thu, 3 Jul 2025 20:15:07 +0800
Menglong Dong <menglong8.d...@gmail.com> wrote:
Note, the tracing subsystem uses capitalization in the subject:
ftrace: Add reset_ftrace_direct_ips
> 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
>
> Signed-off-by: Menglong Dong <dong...@chinatelecom.cn>
> ---
> 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.
What exactly do you mean by "reset"?
> +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);
}
> +
> + 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
* ...
*/
-- 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 */
>
> /**
