Hi Steve, Masami,
On Tue, Jun 04, 2024 at 08:18:50AM -0400, Steven Rostedt wrote:
>
> Masami,
>
> This series passed all my tests, are you comfortable with me pushing
> them to linux-next?
As a heads-up (and not to block pushing this into next), I just gave
this a spin on arm64 atop v6.10-rc2, and running the selftests I see:
ftrace - function pid filters
(instance) ftrace - function pid filters
... both go from [PASS] to [FAIL].
Everything else looks good -- I'll go dig into why that's happening.
It's possible that's just something odd with the filesystem I'm using
(e.g. the wnership test failed because this lacks 'stat').
Mark.
>
> -- Steve
>
>
> On Mon, 03 Jun 2024 15:07:04 -0400
> Steven Rostedt <rost...@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.563858...@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)
>
