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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> On Jul 19, 2022, at 11:32 AM, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> 
> 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.

Got it. I will fix these and send v5. 

Thanks,
Song






[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux