Re: [PATCH 4/5] ftrace: Make ftrace_hash_rec_enable return update bool

Namhyung Kim
2016-03-11T14:29:34Z

On Wed, Mar 09, 2016 at 09:46:44PM +0100, Jiri Olsa wrote:
> Change __ftrace_hash_rec_update to return true in case
> we need to update dynamic ftrace call records. It return
> false in case no update is needed.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/ftrace.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index eca592f977b2..123dddc660e9 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -1610,7 +1610,7 @@ static bool test_rec_ops_needs_regs(struct dyn_ftrace *rec)
> return keep_regs;
> }
>
> -static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> +static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
> int filter_hash,
> bool inc)
> {
> @@ -1618,12 +1618,13 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> struct ftrace_hash *other_hash;
> struct ftrace_page *pg;
> struct dyn_ftrace *rec;
> + bool update = false;
> int count = 0;
> int all = 0;
>
> /* Only update if the ops has been registered */
> if (!(ops->flags & FTRACE_OPS_FL_ENABLED))
> - return;
> + return false;
>
> /*
> * In the filter_hash case:
> @@ -1650,7 +1651,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> * then there's nothing to do.
> */
> if (ftrace_hash_empty(hash))
> - return;
> + return false;
> }
>
> do_for_each_ftrace_rec(pg, rec) {
> @@ -1694,7 +1695,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> if (inc) {
> rec->flags++;
> if (FTRACE_WARN_ON(ftrace_rec_count(rec) == FTRACE_REF_MAX))
> - return;
> + return false;
>
> /*
> * If there's only a single callback registered to a
> @@ -1720,7 +1721,7 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> rec->flags |= FTRACE_FL_REGS;
> } else {
> if (FTRACE_WARN_ON(ftrace_rec_count(rec) == 0))
> - return;
> + return false;
> rec->flags--;
>
> /*
> @@ -1753,22 +1754,27 @@ static void __ftrace_hash_rec_update(struct ftrace_ops *ops,
> */
> }
> count++;
> +
> + update |= ftrace_test_record(rec, 1) != FTRACE_UPDATE_IGNORE;

Shouldn't it use 'inc' instead of 1 for the second argument of
the ftrace_test_record()?

Thanks,
Namhyung

> +
> /* Shortcut, if we handled all records, we are done. */
> if (!all && count == hash->count)
> - return;
> + return update;
> } while_for_each_ftrace_rec();
> +
> + return update;
> }
>
> -static void ftrace_hash_rec_disable(struct ftrace_ops *ops,
> +static bool ftrace_hash_rec_disable(struct ftrace_ops *ops,
> int filter_hash)
> {
> - __ftrace_hash_rec_update(ops, filter_hash, 0);
> + return __ftrace_hash_rec_update(ops, filter_hash, 0);
> }
>
> -static void ftrace_hash_rec_enable(struct ftrace_ops *ops,
> +static bool ftrace_hash_rec_enable(struct ftrace_ops *ops,
> int filter_hash)
> {
> - __ftrace_hash_rec_update(ops, filter_hash, 1);
> + return __ftrace_hash_rec_update(ops, filter_hash, 1);
> }
>
> static void ftrace_hash_rec_update_modify(struct ftrace_ops *ops,
> --
> 2.4.3
>