Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

[Steven Rostedt]

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