Re: [PATCH v3 00/27] function_graph: Allow multiple users for function graph tracing

Steven Rostedt
2024-06-04T12:18:50Z

Masami,

This series passed all my tests, are you comfortable with me pushing
them to linux-next?

-- Steve

On Mon, 03 Jun 2024 15:07:04 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> This is a continuation of the function graph multi user code.
> I wrote a proof of concept back in 2019 of this code[1] and
> Masami started cleaning it up. I started from Masami's work v10
> that can be found here:
>
> https://lore.kernel.org/linux-trace-kernel/171509088006.162236.7227326999861366050.stgit@devnote2/
>
> This is *only* the code that allows multiple users of function
> graph tracing. This is not the fprobe work that Masami is working
> to add on top of it. As Masami took my proof of concept, there
> was still several things I disliked about that code. Instead of
> having Masami clean it up even more, I decided to take over on just
> my code and change it up a bit.
>
> Changes since v2: https://lore.kernel.org/linux-trace-kernel/20240602033744.563858532@goodmis.org
>
> - Added comments describing which hashes the append and intersect
> functions were used for.
>
> - Replaced checks of (NULL or EMPTY_HASH) with ftrace_hash_empty()
> helper function.
>
> - Added check at the end of intersect_hash() to convert the hash
> to EMPTY hash if it doesn't have any functions.
>
> - Renamed compare_ops() to ops_equal() and return boolean (inversed return
> value).
>
> - Broke out __ftrace_hash_move_and_update_ops() to use in both
> ftrace_hash_move_and_update_ops() and ftrace_hash_move_and_update_subops().
>
> Diff between last version at end of this email.
>
> Masami Hiramatsu (Google) (3):
> function_graph: Handle tail calls for stack unwinding
> function_graph: Use a simple LRU for fgraph_array index number
> ftrace: Add multiple fgraph storage selftest
>
> Steven Rostedt (Google) (9):
> ftrace: Add subops logic to allow one ops to manage many
> ftrace: Allow subops filtering to be modified
> function_graph: Add pid tracing back to function graph tracer
> function_graph: Use for_each_set_bit() in __ftrace_return_to_handler()
> function_graph: Use bitmask to loop on fgraph entry
> function_graph: Use static_call and branch to optimize entry function
> function_graph: Use static_call and branch to optimize return function
> selftests/ftrace: Add function_graph tracer to func-filter-pid test
> selftests/ftrace: Add fgraph-multi.tc test
>
> Steven Rostedt (VMware) (15):
> function_graph: Convert ret_stack to a series of longs
> fgraph: Use BUILD_BUG_ON() to make sure we have structures divisible by long
> function_graph: Add an array structure that will allow multiple callbacks
> function_graph: Allow multiple users to attach to function graph
> function_graph: Remove logic around ftrace_graph_entry and return
> ftrace/function_graph: Pass fgraph_ops to function graph callbacks
> ftrace: Allow function_graph tracer to be enabled in instances
> ftrace: Allow ftrace startup flags to exist without dynamic ftrace
> function_graph: Have the instances use their own ftrace_ops for filtering
> function_graph: Add "task variables" per task for fgraph_ops
> function_graph: Move set_graph_function tests to shadow stack global var
> function_graph: Move graph depth stored data to shadow stack global var
> function_graph: Move graph notrace bit to shadow stack global var
> function_graph: Implement fgraph_reserve_data() and fgraph_retrieve_data()
> function_graph: Add selftest for passing local variables
>
> ----
> include/linux/ftrace.h | 43 +-
> include/linux/sched.h | 2 +-
> include/linux/trace_recursion.h | 39 -
> kernel/trace/fgraph.c | 1044 ++++++++++++++++----
> kernel/trace/ftrace.c | 522 +++++++++-
> kernel/trace/ftrace_internal.h | 5 +-
> kernel/trace/trace.h | 94 +-
> kernel/trace/trace_functions.c | 8 +
> kernel/trace/trace_functions_graph.c | 96 +-
> kernel/trace/trace_irqsoff.c | 10 +-
> kernel/trace/trace_sched_wakeup.c | 10 +-
> kernel/trace/trace_selftest.c | 259 ++++-
> .../selftests/ftrace/test.d/ftrace/fgraph-multi.tc | 103 ++
> .../ftrace/test.d/ftrace/func-filter-pid.tc | 27 +-
> 14 files changed, 1945 insertions(+), 317 deletions(-)
> create mode 100644 tools/testing/selftests/ftrace/test.d/ftrace/fgraph-multi.tc
>
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 41fabc6d30e4..da7e6abf48b4 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -3170,7 +3170,7 @@ int ftrace_shutdown(struct ftrace_ops *ops, int command)
> /* Simply make a copy of @src and return it */
> static struct ftrace_hash *copy_hash(struct ftrace_hash *src)
> {
> - if (!src || src == EMPTY_HASH)
> + if (ftrace_hash_empty(src))
> return EMPTY_HASH;
>
> return alloc_and_copy_ftrace_hash(src->size_bits, src);
> @@ -3187,6 +3187,9 @@ static struct ftrace_hash *copy_hash(struct ftrace_hash *src)
> *
> * Otherwise, go through all of @new_hash and add anything that @hash
> * doesn't already have, to @hash.
> + *
> + * The filter_hash updates uses just the append_hash() function
> + * and the notrace_hash does not.
> */
> static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash)
> {
> @@ -3195,11 +3198,11 @@ static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash)
> int i;
>
> /* An empty hash does everything */
> - if (!*hash || *hash == EMPTY_HASH)
> + if (ftrace_hash_empty(*hash))
> return 0;
>
> /* If new_hash has everything make hash have everything */
> - if (!new_hash || new_hash == EMPTY_HASH) {
> + if (ftrace_hash_empty(new_hash)) {
> free_ftrace_hash(*hash);
> *hash = EMPTY_HASH;
> return 0;
> @@ -3217,7 +3220,12 @@ static int append_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash)
> return 0;
> }
>
> -/* Add to @hash only those that are in both @new_hash1 and @new_hash2 */
> +/*
> + * Add to @hash only those that are in both @new_hash1 and @new_hash2
> + *
> + * The notrace_hash updates uses just the intersect_hash() function
> + * and the filter_hash does not.
> + */
> static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_hash1,
> struct ftrace_hash *new_hash2)
> {
> @@ -3229,8 +3237,7 @@ static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_has
> * 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) {
> + if (ftrace_hash_empty(new_hash1) || ftrace_hash_empty(new_hash2)) {
> free_ftrace_hash(*hash);
> *hash = EMPTY_HASH;
> return 0;
> @@ -3245,6 +3252,11 @@ static int intersect_hash(struct ftrace_hash **hash, struct ftrace_hash *new_has
> return -ENOMEM;
> }
> }
> + /* If nothing intersects, make it the empty set */
> + if (ftrace_hash_empty(*hash)) {
> + free_ftrace_hash(*hash);
> + *hash = EMPTY_HASH;
> + }
> return 0;
> }
>
> @@ -3266,7 +3278,7 @@ static struct ftrace_hash *append_hashes(struct ftrace_ops *ops)
> return NULL;
> }
> /* Nothing more to do if new_hash is empty */
> - if (new_hash == EMPTY_HASH)
> + if (ftrace_hash_empty(new_hash))
> break;
> }
> return new_hash;
> @@ -3300,59 +3312,76 @@ static struct ftrace_hash *intersect_hashes(struct ftrace_ops *ops)
> return NULL;
> }
> /* Nothing more to do if new_hash is empty */
> - if (new_hash == EMPTY_HASH)
> + if (ftrace_hash_empty(new_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)
> +static bool ops_equal(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 (ftrace_hash_empty(A))
> + return ftrace_hash_empty(B);
>
> - if (!B || B == EMPTY_HASH)
> - return !(!A || A == EMPTY_HASH);
> + if (ftrace_hash_empty(B))
> + return ftrace_hash_empty(A);
>
> if (A->count != B->count)
> - return 1;
> + return false;
>
> 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 false;
> }
> }
>
> - return 0;
> + return true;
> }
>
> -static int ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
> - struct ftrace_hash **orig_hash,
> - struct ftrace_hash *hash,
> - int enable);
> +static void ftrace_ops_update_code(struct ftrace_ops *ops,
> + struct ftrace_ops_hash *old_hash);
> +
> +static int __ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
> + struct ftrace_hash **orig_hash,
> + struct ftrace_hash *hash,
> + int enable)
> +{
> + struct ftrace_ops_hash old_hash_ops;
> + struct ftrace_hash *old_hash;
> + int ret;
> +
> + old_hash = *orig_hash;
> + old_hash_ops.filter_hash = ops->func_hash->filter_hash;
> + old_hash_ops.notrace_hash = ops->func_hash->notrace_hash;
> + ret = ftrace_hash_move(ops, enable, orig_hash, hash);
> + if (!ret) {
> + ftrace_ops_update_code(ops, &old_hash_ops);
> + free_ftrace_hash_rcu(old_hash);
> + }
> + return ret;
> +}
>
> 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 (!ops_equal(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 (!ops_equal(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;
> }
> @@ -3438,8 +3467,8 @@ int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int
> * 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) {
> + if (ftrace_hash_empty(ops->func_hash->filter_hash) ||
> + ftrace_hash_empty(subops->func_hash->filter_hash)) {
> filter_hash = EMPTY_HASH;
> } else {
> size_bits = max(ops->func_hash->filter_hash->size_bits,
> @@ -3454,8 +3483,8 @@ int ftrace_startup_subops(struct ftrace_ops *ops, struct ftrace_ops *subops, int
> }
> }
>
> - if (ops->func_hash->notrace_hash == EMPTY_HASH ||
> - subops->func_hash->notrace_hash == EMPTY_HASH) {
> + if (ftrace_hash_empty(ops->func_hash->notrace_hash) ||
> + ftrace_hash_empty(subops->func_hash->notrace_hash)) {
> notrace_hash = EMPTY_HASH;
> } else {
> size_bits = max(ops->func_hash->filter_hash->size_bits,
> @@ -3591,7 +3620,7 @@ static int ftrace_hash_move_and_update_subops(struct ftrace_ops *subops,
> }
>
> /* Move the hash over to the new hash */
> - ret = ftrace_hash_move_and_update_ops(ops, orig_hash, new_hash, enable);
> + ret = __ftrace_hash_move_and_update_ops(ops, orig_hash, new_hash, enable);
>
> free_ftrace_hash(new_hash);
>
> @@ -4822,11 +4851,6 @@ static int ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
> struct ftrace_hash *hash,
> int enable)
> {
> - struct ftrace_ops_hash old_hash_ops;
> - struct ftrace_hash *old_hash;
> - struct ftrace_ops *op;
> - int ret;
> -
> if (ops->flags & FTRACE_OPS_FL_SUBOP)
> return ftrace_hash_move_and_update_subops(ops, orig_hash, hash, enable);
>
> @@ -4838,6 +4862,8 @@ static int ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
> * it will not affect subops that share it.
> */
> if (!(ops->flags & FTRACE_OPS_FL_ENABLED)) {
> + struct ftrace_ops *op;
> +
> /* Check if any other manager subops maps to this hash */
> do_for_each_ftrace_op(op, ftrace_ops_list) {
> struct ftrace_ops *subops;
> @@ -4851,15 +4877,7 @@ static int ftrace_hash_move_and_update_ops(struct ftrace_ops *ops,
> } while_for_each_ftrace_op(op);
> }
>
> - old_hash = *orig_hash;
> - old_hash_ops.filter_hash = ops->func_hash->filter_hash;
> - old_hash_ops.notrace_hash = ops->func_hash->notrace_hash;
> - ret = ftrace_hash_move(ops, enable, orig_hash, hash);
> - if (!ret) {
> - ftrace_ops_update_code(ops, &old_hash_ops);
> - free_ftrace_hash_rcu(old_hash);
> - }
> - return ret;
> + return __ftrace_hash_move_and_update_ops(ops, orig_hash, hash, enable);
> }
>
> static bool module_exists(const char *module)