Message-ID: <20250703113001.099dc88f@batman.local.home>
Date: Thu, 3 Jul 2025 11:30:01 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Menglong Dong <menglong8.dong@...il.com>
Cc: alexei.starovoitov@...il.com, jolsa@...nel.org, bpf@...r.kernel.org,
 Menglong Dong <dongml2@...natelecom.cn>, Mark Rutland
 <mark.rutland@....com>, Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
 linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 04/18] ftrace: add reset_ftrace_direct_ips

On Thu,  3 Jul 2025 20:15:07 +0800
Menglong Dong <menglong8.dong@...il.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 <dongml2@...natelecom.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 */
>  
>  /**

Powered by blists - more mailing lists