On Thu, 6 Jun 2024 18:53:12 +0100
Mark Rutland <mark.rutl...@arm.com> wrote:
> 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.
Let me go fix up linux-next :-p
>
> > 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).
I noticed this too as well as:
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;
}
That "return false" is actually a mistake as well. But I tried to hit
it, but found that I could not. I think I updated the code due to bugs
where I prevent that from happening, but the real fix would have been
this patch. :-p
>
> 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.
Right.
>
> > 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.
I'll have to do benchmarks. This loop is what takes up a lot of time
when you enable function tracing. It loops over 40,000 records (just do
a wc -l available_filter_functions to get the true count).
But if you want to send a formal patch, I could test it.
Thanks,
-- Steve
>
> 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;
>
