On Mon, 18 Jul 2022 16:59:51 +0000
Song Liu <songliubraving@xxxxxx> wrote:
> >> vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
> >>
> >> 8182
> >> 8183 /**
> >> 8184 * register_ftrace_function - register a function for profiling
> >> 8185 * @ops: ops structure that holds the function for profiling.
> >> 8186 *
> >> 8187 * Register a function to be called by all functions in the
> >> 8188 * kernel.
> >> 8189 *
> >> 8190 * Note: @ops->func and all the functions it calls must be labeled
> >> 8191 * with "notrace", otherwise it will go into a
> >> 8192 * recursive loop.
> >> 8193 */
> >> 8194 int register_ftrace_function(struct ftrace_ops *ops)
> >> 8195 __releases(&direct_mutex)
> >> 8196 {
> >>> 8197 bool direct_mutex_locked = false;
> >> 8198 int ret;
> >> 8199
> >> 8200 ftrace_ops_init(ops);
> >> 8201
> >> 8202 ret = prepare_direct_functions_for_ipmodify(ops);
> >> 8203 if (ret < 0)
> >> 8204 return ret;
> >> 8205 else if (ret == 1)
> >> 8206 direct_mutex_locked = true;
> >
> > Honestly, this is another horrible trick. Would it be possible to
> > call prepare_direct_functions_for_ipmodify() with direct_mutex
> > already taken?
Agreed. I'm not sure why I didn't notice this in the other versions.
Probably was looking too much at the other logic. :-/
> >
> > I mean something like:
> >
> > mutex_lock(&direct_mutex);
> >
> > ret = prepare_direct_functions_for_ipmodify(ops);
> > if (ret)
> > goto out:
> >
> > mutex_lock(&ftrace_lock);
> > ret = ftrace_startup(ops, 0);
> > mutex_unlock(&ftrace_lock);
> >
> > out:
> > mutex_unlock(&direct_mutex);
> > return ret;
>
> Yeah, we can actually do something like this. We can also move the
> ops->flags & FTRACE_OPS_FL_IPMODIFY check to
> register_ftrace_function(), so we only lock direct_mutex when when
> it is necessary.
No need. Just take the direct_mutex, and perhaps add a:
lockdep_assert_held(&direct_mutex);
in the prepare_direct_functions_for_ipmodify().
This is far from a fast path to do any tricks in trying to optimize it.
-- Steve
