On Wed, Jun 05, 2024 at 02:03:36PM -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (Google)" <rost...@goodmis.org>
>
> While adding comments to the function __ftrace_hash_rec_update() and
> trying to describe in detail what the parameter for "ftrace_hash" does, I
> realized that it basically does exactly the same thing (but differently)
> if it is set or not!
Typo: "ftrace_hash" should be "filter_hash", and likewise in the commit
title.
> If it is set, the idea was the ops->filter_hash was being updated, and the
> code should focus on the functions that are in the ops->filter_hash and
> add them. But it still had to pay attention to the functions in the
> ops->notrace_hash, to ignore them.
>
> If it was cleared, it focused on the ops->notrace_hash, and would add
> functions that were not in the ops->notrace_hash but would still keep
> functions in the "ops->filter_hash". Basically doing the same thing.
>
> In reality, the __ftrace_hash_rec_update() only needs to either remove the
> functions associated to the give ops (if "inc" is set) or remove them (if
> "inc" is cleared). It has to pay attention to both the filter_hash and
> notrace_hash regardless.
AFAICT, we actually remove a latent bug here, but that bug wasn't
reachable because we never call __ftrace_hash_rec_update() with
"filter_hash" clear.
Before this patch, if we did call __ftrace_hash_rec_update() with
"filter_hash" clear, we'd setup:
all = false;
...
if (filter_hash) {
...
} else {
hash = ops->func_hash->notrace_hash;
other_hash = ops->func_hash->filter_hash;
}
... and then at the tail of the loop where we do:
count++;
[...]
/* Shortcut, if we handled all records, we are done. */
if (!all && count == hash->count) {
pr_info("HARK: stopping after %d recs\n", count);
return update;
}
... we'd be checking whether we've updated notrace_hash->count entries,
when that could be smaller than the number of entries we actually need
to update (e.g. if the notrace hash contains a single entry, but the
filter_hash contained more).
As above, we never actually hit that because we never call with
"filter_hash" clear, so it might be good to explicitly say that before
this patch we never actually call __ftrace_hash_rec_update() with
"filter_hash" clear, and this is removing dead (and potentially broken)
code.
> Remove the "filter_hash" parameter from __filter_hash_rec_update() and
> comment the function for what it really is doing.
>
> Signed-off-by: Steven Rostedt (Google) <rost...@goodmis.org>
FWIW, this looks good to me, and works in testing, so:
Reviewed-by: Mark Rutland <mark.rutl...@arm.com>
Tested-by: Mark Rutland <mark.rutl...@arm.com>
I have one comment below, but the above stands regardless.
[...]
> +/*
> + * This is the main engine to the ftrace updates to the dyn_ftrace records.
> + *
> + * It will iterate through all the available ftrace functions
> + * (the ones that ftrace can have callbacks to) and set the flags
> + * in the associated dyn_ftrace records.
> + *
> + * @inc: If true, the functions associated to @ops are added to
> + * the dyn_ftrace records, otherwise they are removed.
> + */
> static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> - int filter_hash,
> bool inc)
> {
> struct ftrace_hash *hash;
> - struct ftrace_hash *other_hash;
> + struct ftrace_hash *notrace_hash;
> struct ftrace_page *pg;
> struct dyn_ftrace *rec;
> bool update = false;
> @@ -1718,35 +1725,16 @@ static bool __ftrace_hash_rec_update(struct
> ftrace_ops *ops,
> return false;
>
> /*
> - * In the filter_hash case:
> * If the count is zero, we update all records.
> * Otherwise we just update the items in the hash.
> - *
> - * In the notrace_hash case:
> - * We enable the update in the hash.
> - * As disabling notrace means enabling the tracing,
> - * and enabling notrace means disabling, the inc variable
> - * gets inversed.
> */
> - if (filter_hash) {
> - hash = ops->func_hash->filter_hash;
> - other_hash = ops->func_hash->notrace_hash;
> - if (ftrace_hash_empty(hash))
> - all = true;
> - } else {
> - inc = !inc;
> - hash = ops->func_hash->notrace_hash;
> - other_hash = ops->func_hash->filter_hash;
> - /*
> - * If the notrace hash has no items,
> - * then there's nothing to do.
> - */
> - if (ftrace_hash_empty(hash))
> - return false;
> - }
> + hash = ops->func_hash->filter_hash;
> + notrace_hash = ops->func_hash->notrace_hash;
> + if (ftrace_hash_empty(hash))
> + all = true;
>
> do_for_each_ftrace_rec(pg, rec) {
> - int in_other_hash = 0;
> + int in_notrace_hash = 0;
> int in_hash = 0;
> int match = 0;
>
> @@ -1758,26 +1746,17 @@ static bool __ftrace_hash_rec_update(struct
> ftrace_ops *ops,
> * Only the filter_hash affects all records.
> * Update if the record is not in the notrace hash.
> */
> - if (!other_hash || !ftrace_lookup_ip(other_hash,
> rec->ip))
> + if (!notrace_hash || !ftrace_lookup_ip(notrace_hash,
> rec->ip))
> match = 1;
> } else {
> in_hash = !!ftrace_lookup_ip(hash, rec->ip);
> - in_other_hash = !!ftrace_lookup_ip(other_hash, rec->ip);
> + in_notrace_hash = !!ftrace_lookup_ip(notrace_hash,
> rec->ip);
>
> /*
> - * If filter_hash is set, we want to match all functions
> - * that are in the hash but not in the other hash.
> - *
> - * If filter_hash is not set, then we are decrementing.
> - * That means we match anything that is in the hash
> - * and also in the other_hash. That is, we need to turn
> - * off functions in the other hash because they are
> disabled
> - * by this hash.
> + * We want to match all functions that are in the hash
> but
> + * not in the other hash.
> */
> - if (filter_hash && in_hash && !in_other_hash)
> - match = 1;
> - else if (!filter_hash && in_hash &&
> - (in_other_hash ||
> ftrace_hash_empty(other_hash)))
> + if (in_hash && !in_notrace_hash)
> match = 1;
> }
> if (!match)
I wonder how much the (subsequent) shortcut for count == hash->count
matters in practice, because if we were happy to get rid of that, we
could get rid of 'all', 'count', 'in_hash', 'in_notrace_hash', and
simplify the above down to:
do_for_each_ftrace_rec(pg, rec) {
if (skip_record(rec))
continue;
/*
* When the hash is empty we update all records.
* Otherwise we just update the items in the hash.
*/
if (!ftrace_hash_empty(hash) &&
!ftrace_lookup_ip(hash, rec->ip))
continue;
if (!ftrace_lookup_ip(notrace_hash, rec->ip))
continue;
[...]
/* do the actual updates here */
[...]
} while_for_each_ftrace_rec();
... which'd be easier to follow.
FWIW, diff atop this patch below. It passes the selftests in my local
testing, but I understand if we'd rather keep the shortcut.
Mark.
---->8----
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index f1aab82fa465..165e8dd4f894 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1714,49 +1714,27 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops
*ops,
struct ftrace_page *pg;
struct dyn_ftrace *rec;
bool update = false;
- int count = 0;
- int all = false;
/* Only update if the ops has been registered */
if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
return false;
- /*
- * If the count is zero, we update all records.
- * Otherwise we just update the items in the hash.
- */
hash = ops->func_hash->filter_hash;
notrace_hash = ops->func_hash->notrace_hash;
- if (ftrace_hash_empty(hash))
- all = true;
do_for_each_ftrace_rec(pg, rec) {
- int in_notrace_hash = 0;
- int in_hash = 0;
- int match = 0;
-
if (skip_record(rec))
continue;
- if (all) {
- /*
- * Only the filter_hash affects all records.
- * Update if the record is not in the notrace hash.
- */
- if (!notrace_hash || !ftrace_lookup_ip(notrace_hash,
rec->ip))
- match = 1;
- } else {
- in_hash = !!ftrace_lookup_ip(hash, rec->ip);
- in_notrace_hash = !!ftrace_lookup_ip(notrace_hash,
rec->ip);
+ /*
+ * If the hash is empty, we update all records.
+ * Otherwise we just update the items in the hash.
+ */
+ if (!ftrace_hash_empty(hash) &&
+ !ftrace_lookup_ip(hash, rec->ip))
+ continue;
- /*
- * We want to match all functions that are in the hash
but
- * not in the other hash.
- */
- if (in_hash && !in_notrace_hash)
- match = 1;
- }
- if (!match)
+ if (ftrace_lookup_ip(notrace_hash, rec->ip))
continue;
if (inc) {
@@ -1846,14 +1824,9 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops
*ops,
else
rec->flags &= ~FTRACE_FL_CALL_OPS;
- count++;
-
/* Must match FTRACE_UPDATE_CALLS in ftrace_modify_all_code() */
update |= ftrace_test_record(rec, true) != FTRACE_UPDATE_IGNORE;
- /* Shortcut, if we handled all records, we are done. */
- if (!all && count == hash->count)
- return update;
} while_for_each_ftrace_rec();
return update;
