On Tue, 19 Jul 2022 22:57:52 +0000
Song Liu <songliubraving@xxxxxx> wrote:
> >> + hash = ops->func_hash->filter_hash;
> >> + size = 1 << hash->size_bits;
> >> + for (i = 0; i < size; i++) {
> >> + hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> >> + unsigned long ip = entry->ip;
> >> + bool found_op = false;
> >> +
> >> + mutex_lock(&ftrace_lock);
> >> + do_for_each_ftrace_op(op, ftrace_ops_list) {
> >> + if (!(op->flags & FTRACE_OPS_FL_DIRECT))
> >> + continue;
> >> + if (ops_references_ip(op, ip)) {
> >> + found_op = true;
> >> + break;
> >
> > I think you want a goto here. The macros "do_for_each_ftrace_op() { .. }
> > while_for_each_ftrace_op()" is a double loop. The break just moves to the
> > next set of pages and does not break out of the outer loop.
>
> Hmmm... really? I didn't see it ...
>
>
> #define do_for_each_ftrace_op(op, list) \
> op = rcu_dereference_raw_check(list); \
> do
>
> #define while_for_each_ftrace_op(op) \
> while (likely(op = rcu_dereference_raw_check((op)->next)) && \
> unlikely((op) != &ftrace_list_end))
>
> Did I miss something...?
Bah, you're right. I was confusing it with do_for_each_ftrace_rec(), which
*is* a double loop.
Never mind ;-)
>
> >
> > goto out_loop;
> >
> >> + }
> >> + } while_for_each_ftrace_op(op);
>
> [...]
>
> >
> >> mutex_lock(&ftrace_lock);
> >>
> >> ret = ftrace_startup(ops, 0);
> >>
> >> mutex_unlock(&ftrace_lock);
> >>
> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >> + if (direct_mutex_locked)
> >> + mutex_unlock(&direct_mutex);
> >> +#endif
> >
> > Change this to:
> >
> > out_unlock:
> > mutex_unlock(&direct_mutex);
> >
>
> We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as
> direct_mutex is not defined without that config.
Ah, right. I just meant to get rid of the if statement.
To keep the code clean, perhaps we should have:
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
# define lock_direct_mutex() mutex_lock(&direct_mutex)
# define unlock_direct_mutex() mutex_unlock(&direct_mutex)
#else
# define lock_direct_mutex() do { } while (0)
# define unlock_direct_mutex() do { } while (0)
#endif
And use that here.
-- Steve
