Hi Steve,
On Sat, 01 Jun 2024 23:37:54 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> From: "Steven Rostedt (VMware)" <rostedt@xxxxxxxxxxx>
I think this is a new patch, correct? I'm a bit confused.
And I have some comments below;
[..]
> @@ -3164,6 +3166,392 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command)
> return 0;
> }
>
> +/* Simply make a copy of @src and return it */
> +static struct ftrace_hash *copy_hash(struct ftrace_hash *src)
> +{
> + if (!src || src == EMPTY_HASH)
> + return EMPTY_HASH;
> +
> + return alloc_and_copy_ftrace_hash(src->size_bits, src);
> +}
> +
> +/*
> + * Append @new_hash entries to @hash:
> + *
> + * If @hash is the EMPTY_HASH then it traces all functions and nothing
> + * needs to be done.
> + *
> + * If @new_hash is the EMPTY_HASH, then make *hash the EMPTY_HASH so
> + * that it traces everything.
This lacks the most important comment, this function is only for
filter_hash, not for notrace_hash. :)
> + *
> + * Otherwise, go through all of @new_hash and add anything that @hash
> + * doesn't already have, to @hash.
> + */
> +static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash)
> +{
> + struct ftrace_func_entry *entry;
> + int size;
> + int i;
> +
> + /* An empty hash does everything */
> + if (!*hash || *hash == EMPTY_HASH)
> + return 0;
> +
> + /* If new_hash has everything make hash have everything */
> + if (!new_hash || new_hash == EMPTY_HASH) {
> + free_ftrace_hash(*hash);
> + *hash = EMPTY_HASH;
> + return 0;
> + }
> +
> + size = 1 << new_hash->size_bits;
> + for (i = 0; i < size; i++) {
> + hlist_for_each_entry(entry, &new_hash->buckets[i], hlist) {
> + /* Only add if not already in hash */
> + if (!__ftrace_lookup_ip(*hash, entry->ip) &&
> + add_hash_entry(*hash, entry->ip) == NULL)
> + return -ENOMEM;
> + }
> + }
> + return 0;
> +}
> +
> +/* Add to @hash only those that are in both @new_hash1 and @new_hash2 */
Ditto, this is only for the notrace_hash.
> +static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash1,
> + struct ftrace_hash *new_hash2)
> +{
> + struct ftrace_func_entry *entry;
> + int size;
> + int i;
> +
> + /*
> + * If new_hash1 or new_hash2 is the EMPTY_HASH then make the hash
> + * empty as well as empty for notrace means none are notraced.
> + */
> + if (!new_hash1 || new_hash1 == EMPTY_HASH ||
> + !new_hash2 || new_hash2 == EMPTY_HASH) {
> + free_ftrace_hash(*hash);
> + *hash = EMPTY_HASH;
> + return 0;
> + }
> +
> + size = 1 << new_hash1->size_bits;
> + for (i = 0; i < size; i++) {
> + hlist_for_each_entry(entry, &new_hash1->buckets[i], hlist) {
> + /* Only add if in both @new_hash1 and @new_hash2 */
> + if (__ftrace_lookup_ip(new_hash2, entry->ip) &&
> + add_hash_entry(*hash, entry->ip) == NULL)
> + return -ENOMEM;
> + }
> + }
> + return 0;
> +}
> +
> +/* Return a new hash that has a union of all @ops->filter_hash entries */
> +static struct ftrace_hash *append_hashes(struct ftrace_ops *ops)
> +{
> + struct ftrace_hash *new_hash;
> + struct ftrace_ops *subops;
> + int ret;
> +
> + new_hash = alloc_ftrace_hash(ops->func_hash->filter_hash->size_bits);
> + if (!new_hash)
> + return NULL;
> +
> + list_for_each_entry(subops, &ops->subop_list, list) {
> + ret = append_hash(&new_hash, subops->func_hash->filter_hash);
> + if (ret < 0) {
> + free_ftrace_hash(new_hash);
> + return NULL;
> + }
> + /* Nothing more to do if new_hash is empty */
> + if (new_hash == EMPTY_HASH)
> + break;
> + }
> + return new_hash;
> +}
> +
> +/* Make @ops trace evenything except what all its subops do not trace */
> +static struct ftrace_hash *intersect_hashes(struct ftrace_ops *ops)
> +{
> + struct ftrace_hash *new_hash = NULL;
> + struct ftrace_ops *subops;
> + int size_bits;
> + int ret;
> +
> + list_for_each_entry(subops, &ops->subop_list, list) {
> + struct ftrace_hash *next_hash;
> +
> + if (!new_hash) {
> + size_bits = subops->func_hash->notrace_hash->size_bits;
> + new_hash = alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->notrace_hash);
> + if (!new_hash)
> + return NULL;
If the first subops has EMPTY_HASH, this allocates small empty hash (!= EMPTY_HASH)
on `new_hash`.
> + continue;
> + }
> + size_bits = new_hash->size_bits;
> + next_hash = new_hash;
And it is assigned to `next_hash`.
> + new_hash = alloc_ftrace_hash(size_bits);
> + ret = intersect_hash(&new_hash, next_hash, subops->func_hash->notrace_hash);
Since the `next_hash` != EMPTY_HASH but it is empty, this keeps `new_hash`
empty but allocated.
> + free_ftrace_hash(next_hash);
> + if (ret < 0) {
> + free_ftrace_hash(new_hash);
> + return NULL;
> + }
> + /* Nothing more to do if new_hash is empty */
> + if (new_hash == EMPTY_HASH)
Since `new_hash` is empty but != EMPTY_HASH, this does not pass. Keep looping on.
> + break;
> + }
> + return new_hash;
And this will return empty but not EMPTY_HASH hash.
So, we need;
#define FTRACE_EMPTY_HASH_OR_NULL(hash) (!(hash) || (hash) == EMPTY_HASH)
if (FTRACE_EMPTY_HASH_OR_NULL(subops->func_hash->notrace_hash)) {
free_ftrace_hash(new_hash);
new_hash = EMPTY_HASH;
break;
}
at the beginning of the loop.
Also, at the end of the loop,
if (ftrace_hash_empty(new_hash)) {
free_ftrace_hash(new_hash);
new_hash = EMPTY_HASH;
break;
}
> +}
> +
> +/* Returns 0 on equal or non-zero on non-equal */
> +static int compare_ops(struct ftrace_hash *A, struct ftrace_hash *B)
nit: Isn't it better to be `bool hash_equal()` and return true if A == B ?
Thank you,
> +{
> + struct ftrace_func_entry *entry;
> + int size;
> + int i;
> +
> + if (!A || A == EMPTY_HASH)
> + return !(!B || B == EMPTY_HASH);
> +
> + if (!B || B == EMPTY_HASH)
> + return !(!A || A == EMPTY_HASH);
> +
> + if (A->count != B->count)
> + return 1;
> +
> + size = 1 << A->size_bits;
> + for (i = 0; i < size; i++) {
> + hlist_for_each_entry(entry, &A->buckets[i], hlist) {
> + if (!__ftrace_lookup_ip(B, entry->ip))
> + return 1;
> + }
> + }
> +
> + return 0;
> +}
> +
--
Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>
