lkml.org 
[lkml]   [2022]   [Jul]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function
On Mon, 18 Jul 2022 16:59:51 +0000
Song Liu <songliubraving@fb.com> 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

\
 
 \ /
  Last update: 2022-07-19 20:34    [W:0.059 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site