(2013/12/12 10:34), Steven Rostedt wrote:
> On Tue, 10 Dec 2013 09:57:14 +0000
> Masami Hiramatsu <masami.hiramatsu.pt@xxxxxxxxxxx> 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@xxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
