Re: [PATCHv5 bpf-next 4/9] ftrace: Add update_ftrace_direct_add function

[Steven Rostedt <rostedt@kernel.org>]

On Mon, 15 Dec 2025 22:13:57 +0100
Jiri Olsa <jolsa@kernel.org> wrote:


> +/**
> + * hash_add - adds two struct ftrace_hash and returns the result
> + * @a: struct ftrace_hash object
> + * @b: struct ftrace_hash object
> + *
> + * Returns struct ftrace_hash object on success, NULL on error.
> + */
> +static struct ftrace_hash *hash_add(struct ftrace_hash *a, struct ftrace_hash *b)
> +{
> +	struct ftrace_func_entry *entry;
> +	struct ftrace_hash *add;
> +	int size, i;
> +
> +	size = hash_count(a) + hash_count(b);
> +	if (size > 32)
> +		size = 32;
> +
> +	add = alloc_and_copy_ftrace_hash(fls(size), a);
> +	if (!add)
> +		goto error;

You can just return NULL here, as add is NULL.

> +
> +	size = 1 << b->size_bits;
> +	for (i = 0; i < size; i++) {
> +		hlist_for_each_entry(entry, &b->buckets[i], hlist) {
> +			if (add_hash_entry_direct(add, entry->ip, entry->direct) == NULL)
> +				goto error;

Could remove the error and have:

			if (add_hash_entry_direct(add, entry->ip, entry->direct) == NULL) {
				free_ftrace_hash(add);
				return NULL;
			}


> +		}
> +	}
> +	return add;
> +
> + error:
> +	free_ftrace_hash(add);
> +	return NULL;
> +}
> +

Non static functions require a kerneldoc header.

> +int update_ftrace_direct_add(struct ftrace_ops *ops, struct ftrace_hash *hash)
> +{
> +	struct ftrace_hash *old_direct_functions = NULL, *new_direct_functions;
> +	struct ftrace_hash *old_filter_hash, *new_filter_hash = NULL;

BTW, I prefer to not double up on variables. That is to have each on
their own lines. Makes it easier to read for me.

> +	struct ftrace_func_entry *entry;
> +	int i, size, err = -EINVAL;

Even here.

> +	bool reg;
> +
> +	if (!hash_count(hash))
> +		return -EINVAL;
> +
> +	mutex_lock(&direct_mutex);
> +
> +	/* Make sure requested entries are not already registered. */
> +	size = 1 << hash->size_bits;
> +	for (i = 0; i < size; i++) {

If you want, you can remove the i declaration and use for(int i = 0; ... here.

> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> +			if (__ftrace_lookup_ip(direct_functions, entry->ip))
> +				goto out_unlock;
> +		}
> +	}
> +
> +	old_filter_hash = ops->func_hash ? ops->func_hash->filter_hash : NULL;
> +
> +	/* If there's nothing in filter_hash we need to register the ops. */
> +	reg = hash_count(old_filter_hash) == 0;
> +	if (reg) {
> +		if (ops->func || ops->trampoline)
> +			goto out_unlock;
> +		if (ops->flags & FTRACE_OPS_FL_ENABLED)
> +			goto out_unlock;
> +	}
> +
> +	err = -ENOMEM;
> +	new_filter_hash = hash_add(old_filter_hash, hash);
> +	if (!new_filter_hash)
> +		goto out_unlock;
> +
> +	new_direct_functions = hash_add(direct_functions, hash);
> +	if (!new_direct_functions)
> +		goto out_unlock;
> +
> +	old_direct_functions = direct_functions;
> +	rcu_assign_pointer(direct_functions, new_direct_functions);
> +
> +	if (reg) {
> +		ops->func = call_direct_funcs;
> +		ops->flags |= MULTI_FLAGS;
> +		ops->trampoline = FTRACE_REGS_ADDR;
> +		ops->local_hash.filter_hash = new_filter_hash;
> +
> +		err = register_ftrace_function_nolock(ops);
> +		if (err) {
> +			/* restore old filter on error */
> +			ops->local_hash.filter_hash = old_filter_hash;
> +
> +			/* cleanup for possible another register call */
> +			ops->func = NULL;
> +			ops->trampoline = 0;
> +		} else {
> +			new_filter_hash = old_filter_hash;
> +		}
> +	} else {
> +		err = ftrace_update_ops(ops, new_filter_hash, EMPTY_HASH);
> +		/*
> +		 * new_filter_hash is dup-ed, so we need to release it anyway,
> +		 * old_filter_hash either stays on error or is already released
> +		 */
> +	}
> +
> +	if (err) {
> +		/* reset direct_functions and free the new one */
> +		rcu_assign_pointer(direct_functions, old_direct_functions);
> +		old_direct_functions = new_direct_functions;
> +	}
> +
> + out_unlock:
> +	mutex_unlock(&direct_mutex);
> +
> +	if (old_direct_functions && old_direct_functions != EMPTY_HASH)
> +		call_rcu_tasks(&old_direct_functions->rcu, register_ftrace_direct_cb);
> +	if (new_filter_hash)

free_ftrace_hash() checks for NULL, so you don't need the above if statement.

> +		free_ftrace_hash(new_filter_hash);
> +
> +	return err;

-- Steve

> +}
> +
>  #endif /* CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS */
>  
>  /**