From: "Steven Rostedt (VMware)" <[email protected]>
There are cases where a single system will use a single function callback
to handle multiple users. For example, to allow function_graph tracer to
have multiple users where each can trace their own set of functions, it is
useful to only have one ftrace_ops registered to ftrace that will call a
function by the function_graph tracer to handle the multiplexing with the
different registered function_graph tracers.
Add a "subop_list" to the ftrace_ops that will hold a list of other
ftrace_ops that the top ftrace_ops will manage.
The function ftrace_startup_subops() that takes the manager ftrace_ops and
a subop ftrace_ops it will manage. If there are no subops with the
ftrace_ops yet, it will copy the ftrace_ops subop filters to the manager
ftrace_ops and register that with ftrace_startup(), and adds the subop to
its subop_list. If the manager ops already has something registered, it
will then merge the new subop filters with what it has and enable the new
functions that covers all the subops it has.
To remove a subop, ftrace_shutdown_subops() is called which will use the
subop_list of the manager ops to rebuild all the functions it needs to
trace, and update the ftrace records to only call the functions it now has
registered. If there are no more functions registered, it will then call
ftrace_shutdown() to disable itself completely.
Note, it is up to the manager ops callback to always make sure that the
subops callbacks are called if its filter matches, as there are times in
the update where the callback could be calling more functions than those
that are currently registered.
This could be updated to handle other systems other than function_graph,
for example, fprobes could use this (but will need an interface to call
ftrace_startup_subops()).
Signed-off-by: Steven Rostedt (Google) <[email protected]>
---
include/linux/ftrace.h | 1 +
kernel/trace/fgraph.c | 3 +-
kernel/trace/ftrace.c | 390 ++++++++++++++++++++++++++++++++-
kernel/trace/ftrace_internal.h | 1 +
kernel/trace/trace.h | 1 +
5 files changed, 394 insertions(+), 2 deletions(-)
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 586018744785..978a1d3b270a 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -334,6 +334,7 @@ struct ftrace_ops {
unsigned long trampoline;
unsigned long trampoline_size;
struct list_head list;
+ struct list_head subop_list;
ftrace_ops_func_t ops_func;
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
unsigned long direct_call;
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 54ed2ed2036b..e39042c40937 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -21,7 +21,8 @@
#ifdef CONFIG_DYNAMIC_FTRACE
#define ASSIGN_OPS_HASH(opsname, val) \
.func_hash = val, \
- .local_hash.regex_lock = __MUTEX_INITIALIZER(opsname.local_hash.regex_lock),
+ .local_hash.regex_lock = __MUTEX_INITIALIZER(opsname.local_hash.regex_lock), \
+ .subop_list = LIST_HEAD_INIT(opsname.subop_list),
#else
#define ASSIGN_OPS_HASH(opsname, val)
#endif
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index b85f00b0ffe7..38fb2a634b04 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -74,7 +74,8 @@
#ifdef CONFIG_DYNAMIC_FTRACE
#define INIT_OPS_HASH(opsname) \
.func_hash = &opsname.local_hash, \
- .local_hash.regex_lock = __MUTEX_INITIALIZER(opsname.local_hash.regex_lock),
+ .local_hash.regex_lock = __MUTEX_INITIALIZER(opsname.local_hash.regex_lock), \
+ .subop_list = LIST_HEAD_INIT(opsname.subop_list),
#else
#define INIT_OPS_HASH(opsname)
#endif
@@ -161,6 +162,7 @@ static inline void ftrace_ops_init(struct ftrace_ops *ops)
#ifdef CONFIG_DYNAMIC_FTRACE
if (!(ops->flags & FTRACE_OPS_FL_INITIALIZED)) {
mutex_init(&ops->local_hash.regex_lock);
+ INIT_LIST_HEAD(&ops->subop_list);
ops->func_hash = &ops->local_hash;
ops->flags |= FTRACE_OPS_FL_INITIALIZED;
}
@@ -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.
+ *
+ * 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 */
+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;
+ continue;
+ }
+ size_bits = new_hash->size_bits;
+ next_hash = new_hash;
+ new_hash = alloc_ftrace_hash(size_bits);
+ ret = intersect_hash(&new_hash, next_hash, subops->func_hash->notrace_hash);
+ 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)
+ break;
+ }
+ return new_hash;
+}
+
+/* Returns 0 on equal or non-zero on non-equal */
+static int compare_ops(struct ftrace_hash *A, struct ftrace_hash *B)
+{
+ 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;
+}
+
+static int ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
+ struct ftrace_hash **orig_hash,
+ struct ftrace_hash *hash,
+ int enable);
+
+static int ftrace_update_ops(struct ftrace_ops *ops, struct ftrace_hash *filter_hash,
+ struct ftrace_hash *notrace_hash)
+{
+ int ret;
+
+ if (compare_ops(filter_hash, ops->func_hash->filter_hash)) {
+ ret = ftrace_hash_move_and_update_ops(ops, &ops->func_hash->filter_hash,
+ filter_hash, 1);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (compare_ops(notrace_hash, ops->func_hash->notrace_hash)) {
+ ret = ftrace_hash_move_and_update_ops(ops, &ops->func_hash->notrace_hash,
+ notrace_hash, 0);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
+/**
+ * ftrace_startup_subops - enable tracing for subops of an ops
+ * @ops: Manager ops (used to pick all the functions of its subops)
+ * @subops: A new ops to add to @ops
+ * @command: Extra commands to use to enable tracing
+ *
+ * The @ops is a manager @ops that has the filter that includes all the functions
+ * that its list of subops are tracing. Adding a new @subops will add the
+ * functions of @subops to @ops.
+ */
+int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command)
+{
+ struct ftrace_hash *filter_hash;
+ struct ftrace_hash *notrace_hash;
+ struct ftrace_hash *save_filter_hash;
+ struct ftrace_hash *save_notrace_hash;
+ int size_bits;
+ int ret;
+
+ if (unlikely(ftrace_disabled))
+ return -ENODEV;
+
+ ftrace_ops_init(ops);
+ ftrace_ops_init(subops);
+
+ if (WARN_ON_ONCE(subops->flags & FTRACE_OPS_FL_ENABLED))
+ return -EBUSY;
+
+ /* Make everything canonical (Just in case!) */
+ if (!ops->func_hash->filter_hash)
+ ops->func_hash->filter_hash = EMPTY_HASH;
+ if (!ops->func_hash->notrace_hash)
+ ops->func_hash->notrace_hash = EMPTY_HASH;
+ if (!subops->func_hash->filter_hash)
+ subops->func_hash->filter_hash = EMPTY_HASH;
+ if (!subops->func_hash->notrace_hash)
+ subops->func_hash->notrace_hash = EMPTY_HASH;
+
+ /* For the first subops to ops just enable it normally */
+ if (list_empty(&ops->subop_list)) {
+ /* Just use the subops hashes */
+ filter_hash = copy_hash(subops->func_hash->filter_hash);
+ notrace_hash = copy_hash(subops->func_hash->notrace_hash);
+ if (!filter_hash || !notrace_hash) {
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ return -ENOMEM;
+ }
+
+ save_filter_hash = ops->func_hash->filter_hash;
+ save_notrace_hash = ops->func_hash->notrace_hash;
+
+ ops->func_hash->filter_hash = filter_hash;
+ ops->func_hash->notrace_hash = notrace_hash;
+ list_add(&subops->list, &ops->subop_list);
+ ret = ftrace_startup(ops, command);
+ if (ret < 0) {
+ list_del(&subops->list);
+ ops->func_hash->filter_hash = save_filter_hash;
+ ops->func_hash->notrace_hash = save_notrace_hash;
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ } else {
+ free_ftrace_hash(save_filter_hash);
+ free_ftrace_hash(save_notrace_hash);
+ subops->flags |= FTRACE_OPS_FL_ENABLED;
+ }
+ return ret;
+ }
+
+ /*
+ * Here there's already something attached. Here are the rules:
+ * o If either filter_hash is empty then the final stays empty
+ * o Otherwise, the final is a superset of both hashes
+ * o If either notrace_hash is empty then the final stays empty
+ * o Otherwise, the final is an intersection between the hashes
+ */
+ if (ops->func_hash->filter_hash == EMPTY_HASH ||
+ subops->func_hash->filter_hash == EMPTY_HASH) {
+ filter_hash = EMPTY_HASH;
+ } else {
+ size_bits = max(ops->func_hash->filter_hash->size_bits,
+ subops->func_hash->filter_hash->size_bits);
+ filter_hash = alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->filter_hash);
+ if (!filter_hash)
+ return -ENOMEM;
+ ret = append_hash(&filter_hash, subops->func_hash->filter_hash);
+ if (ret < 0) {
+ free_ftrace_hash(filter_hash);
+ return ret;
+ }
+ }
+
+ if (ops->func_hash->notrace_hash == EMPTY_HASH ||
+ subops->func_hash->notrace_hash == EMPTY_HASH) {
+ notrace_hash = EMPTY_HASH;
+ } else {
+ size_bits = max(ops->func_hash->filter_hash->size_bits,
+ subops->func_hash->filter_hash->size_bits);
+ notrace_hash = alloc_ftrace_hash(size_bits);
+ if (!notrace_hash) {
+ free_ftrace_hash(filter_hash);
+ return -ENOMEM;
+ }
+
+ ret = intersect_hash(¬race_hash, ops->func_hash->filter_hash,
+ subops->func_hash->filter_hash);
+ if (ret < 0) {
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ return ret;
+ }
+ }
+
+ list_add(&subops->list, &ops->subop_list);
+
+ ret = ftrace_update_ops(ops, filter_hash, notrace_hash);
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ if (ret < 0)
+ list_del(&subops->list);
+ else
+ subops->flags |= FTRACE_OPS_FL_ENABLED;
+
+ return ret;
+}
+
+/**
+ * ftrace_shutdown_subops - Remove a subops from a manager ops
+ * @ops: A manager ops to remove @subops from
+ * @subops: The subops to remove from @ops
+ * @command: Any extra command flags to add to modifying the text
+ *
+ * Removes the functions being traced by the @subops from @ops. Note, it
+ * will not affect functions that are being traced by other subops that
+ * still exist in @ops.
+ *
+ * If the last subops is removed from @ops, then @ops is shutdown normally.
+ */
+int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command)
+{
+ struct ftrace_hash *filter_hash;
+ struct ftrace_hash *notrace_hash;
+ int ret;
+
+ if (unlikely(ftrace_disabled))
+ return -ENODEV;
+
+ if (WARN_ON_ONCE(!(subops->flags & FTRACE_OPS_FL_ENABLED)))
+ return -EINVAL;
+
+ list_del(&subops->list);
+
+ if (list_empty(&ops->subop_list)) {
+ /* Last one, just disable the current ops */
+
+ ret = ftrace_shutdown(ops, command);
+ if (ret < 0) {
+ list_add(&subops->list, &ops->subop_list);
+ return ret;
+ }
+
+ subops->flags &= ~FTRACE_OPS_FL_ENABLED;
+
+ free_ftrace_hash(ops->func_hash->filter_hash);
+ free_ftrace_hash(ops->func_hash->notrace_hash);
+ ops->func_hash->filter_hash = EMPTY_HASH;
+ ops->func_hash->notrace_hash = EMPTY_HASH;
+
+ return 0;
+ }
+
+ /* Rebuild the hashes without subops */
+ filter_hash = append_hashes(ops);
+ notrace_hash = intersect_hashes(ops);
+ if (!filter_hash || !notrace_hash) {
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ list_add(&subops->list, &ops->subop_list);
+ return -ENOMEM;
+ }
+
+ ret = ftrace_update_ops(ops, filter_hash, notrace_hash);
+ if (ret < 0)
+ list_add(&subops->list, &ops->subop_list);
+ else
+ subops->flags &= ~FTRACE_OPS_FL_ENABLED;
+
+ free_ftrace_hash(filter_hash);
+ free_ftrace_hash(notrace_hash);
+ return ret;
+}
+
static u64 ftrace_update_time;
unsigned long ftrace_update_tot_cnt;
unsigned long ftrace_number_of_pages;
diff --git a/kernel/trace/ftrace_internal.h b/kernel/trace/ftrace_internal.h
index 19eddcb91584..cdfd12c44ab4 100644
--- a/kernel/trace/ftrace_internal.h
+++ b/kernel/trace/ftrace_internal.h
@@ -15,6 +15,7 @@ extern struct ftrace_ops global_ops;
int ftrace_startup(struct ftrace_ops *ops, int command);
int ftrace_shutdown(struct ftrace_ops *ops, int command);
int ftrace_ops_test(struct ftrace_ops *ops, unsigned long ip, void *regs);
+int ftrace_shutdown_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command);
#else /* !CONFIG_DYNAMIC_FTRACE */
diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
index a5070f9b977b..9a70beb2cc46 100644
--- a/kernel/trace/trace.h
+++ b/kernel/trace/trace.h
@@ -1136,6 +1136,7 @@ extern int ftrace_set_filter(struct ftrace_ops *ops, unsigned char *buf,
int len, int reset);
extern int ftrace_set_notrace(struct ftrace_ops *ops, unsigned char *buf,
int len, int reset);
+extern int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int command);
#else
struct ftrace_func_command;
--
2.43.0
Hi Steve,
On Sat, 01 Jun 2024 23:37:54 -0400
Steven Rostedt <[email protected]> wrote:
> From: "Steven Rostedt (VMware)" <[email protected]>
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) <[email protected]>
On Mon, 3 Jun 2024 10:33:16 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> Hi Steve,
>
> On Sat, 01 Jun 2024 23:37:54 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > From: "Steven Rostedt (VMware)" <[email protected]>
>
> I think this is a new patch, correct? I'm a bit confused.
Ah, good catch!
I originally started writing this code and updating an old commit (one that
I did at VMware), and then split it out. But that keeps the original
authorship (rebase, copy the commit twice, and fix it up).
Will fix.
>
> 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. :)
I did purposely leave it out, because it describes what it does. It's not
that it's only for filter_hash and not for notrace_hash, but it's more that
filter_hash only needs this and notrace_hash only needs the intersection
function ;-)
But, sure, to get rid of confusion, I'll add a comment saying such.
>
> > + *
> > + * 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`.
Could we just change the above to be: ?
new_hash = ftrace_hash_empty(ops->func_hash->notrace_hash) ? EMPTY_HASH :
alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->notrace_hash);
if (!new_hash)
return NULL;
>
> > + 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 ?
Sure. I guess I was thinking too much of strcmp() logic :-p
>
> Thank you,
Thanks for the review.
-- Steve
>
> > +{
> > + 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;
> > +}
> > +
>
>
On Sun, 2 Jun 2024 22:06:13 -0400
Steven Rostedt <[email protected]> wrote:
> > > +/* 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`.
>
> Could we just change the above to be: ?
>
> new_hash = ftrace_hash_empty(ops->func_hash->notrace_hash) ? EMPTY_HASH :
> alloc_and_copy_ftrace_hash(size_bits, ops->func_hash->notrace_hash);
> if (!new_hash)
> return NULL;
Yeah, and if new_hash is EMPTY_HASH, we don't need looping on the rest of
the hashes, right?
>
>
> >
> > > + 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;
> > }
And we still need this (I think this should be done in intersect_hash(), we just
need to count the number of entries.)
> >
> > > +}
> > > +
> > > +/* 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 ?
>
> Sure. I guess I was thinking too much of strcmp() logic :-p
Yeah, it's the curse of the C programmer :( (even it is good for sorting.)
Thank you,
>
> >
> > Thank you,
>
> Thanks for the review.
>
> -- Steve
>
> >
> > > +{
> > > + 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) <[email protected]>
On Mon, 3 Jun 2024 11:46:36 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> > > if (ftrace_hash_empty(new_hash)) {
> > > free_ftrace_hash(new_hash);
> > > new_hash = EMPTY_HASH;
> > > break;
> > > }
>
> And we still need this (I think this should be done in intersect_hash(), we just
> need to count the number of entries.)
Actually, ftrace_hash_empty() may be what I use instead of checking against
EMPTY_HASH. I forgot about that function when writing the code.
-- Steve
On Mon, 3 Jun 2024 11:46:36 +0900
Masami Hiramatsu (Google) <[email protected]> wrote:
> > > 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;
> > > }
>
> And we still need this (I think this should be done in intersect_hash(), we just
> need to count the number of entries.)
Ah, I see. if it ends with nothing intersecting it should be empty. I added:
/* If nothing intersects, make it the empty set */
if (ftrace_hash_empty(*hash)) {
free_ftrace_hash(*hash);
*hash = EMPTY_HASH;
}
to the end of intersect_hash().
-- Steve