Re: Re: [PATCH -tip v5.1 12/18] ftrace/kprobes: Use NOKPROBE_SYMBOL macro in ftrace

[Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>]

(2013/12/12 10:34), Steven Rostedt wrote:
> On Tue, 10 Dec 2013 09:57:14 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com> wrote:
> 
> 
>> --- a/kernel/trace/trace_kprobe.c
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -51,45 +51,45 @@ struct event_file_link {
>>  	(sizeof(struct probe_arg) * (n)))
>>  
>>  
>> -static __kprobes bool trace_probe_is_return(struct trace_probe *tp)
>> +static __always_inline bool trace_probe_is_return(struct trace_probe *tp)
> 
> I wonder if we should have a comment somewhere explaining why we are
> using __always_inline.  Maybe we should add a new annotation:
> 
> #define kprobes_inline   __always_inline
> 
> ?
> 
> The above would be self documenting, and we can also include a comment
> with the define that states why it is there. Otherwise 10 years from
> now, someone is going to see these and say "WTF!" and remove them.

Hm, agreed, and I think nokprobe_inline is better since it is
similar to NOKPROBE_SYMBOL(). :)

[...]
>> @@ -755,8 +755,8 @@ static const struct file_operations kprobe_profile_ops = {
>>  };
>>  
>>  /* Sum up total data length for dynamic arraies (strings) */
>> -static __kprobes int __get_data_size(struct trace_probe *tp,
>> -				     struct pt_regs *regs)
>> +static __always_inline
>> +int __get_data_size(struct trace_probe *tp, struct pt_regs *regs)
> 
> This function is used 4 times within the file and is not that small. I
> think it's a bit too big for an inline, and qualifies for a normal
> function with a NOKPROBE_SYMBOL() attached. 

OK, I'll do so.

>> @@ -771,9 +771,9 @@ static __kprobes int __get_data_size(struct trace_probe *tp,
>>  }
>>  
>>  /* Store the value of each argument */
>> -static __kprobes void store_trace_args(int ent_size, struct trace_probe *tp,
>> -				       struct pt_regs *regs,
>> -				       u8 *data, int maxlen)
>> +static __always_inline
>> +void store_trace_args(int ent_size, struct trace_probe *tp,
>> +		      struct pt_regs *regs, u8 *data, int maxlen)
> 
> Same here (even more so!)

OK.

>>  {
>>  	int i;
>>  	u32 end = tp->size;
>> @@ -803,7 +803,7 @@ static __kprobes void store_trace_args(int ent_size, struct trace_probe *tp,
>>  }
>>  
>>  /* Kprobe handler */
>> -static __kprobes void
>> +static __always_inline void
>>  __kprobe_trace_func(struct trace_probe *tp, struct pt_regs *regs,
>>  		    struct ftrace_event_file *ftrace_file)
> 
> OK, this one is big, but it's only used once.

Right, at least in my build binary, it is inlined.

Thank you,


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com