This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
Re: Re: [PATCH -tip v5.1 12/18] ftrace/kprobes: Use NOKPROBE_SYMBOL macro in ftrace
- From: Masami Hiramatsu <masami dot hiramatsu dot pt at hitachi dot com>
- To: Steven Rostedt <rostedt at goodmis dot org>
- Cc: Ingo Molnar <mingo at kernel dot org>, linux-arch at vger dot kernel dot org, Ananth N Mavinakayanahalli <ananth at in dot ibm dot com>, Sandeepa Prabhu <sandeepa dot prabhu at linaro dot org>, Frederic Weisbecker <fweisbec at gmail dot com>, x86 at kernel dot org, lkml <linux-kernel at vger dot kernel dot org>, Ingo Molnar <mingo at redhat dot com>, systemtap at sourceware dot org, "David S. Miller" <davem at davemloft dot net>
- Date: Thu, 12 Dec 2013 13:13:18 +0900
- Subject: Re: Re: [PATCH -tip v5.1 12/18] ftrace/kprobes: Use NOKPROBE_SYMBOL macro in ftrace
- Authentication-results: sourceware.org; auth=none
- References: <20131209120450 dot 4da43296 at gandalf dot local dot home> <20131210095714 dot 8225 dot 57495 dot stgit at kbuild-fedora dot novalocal> <20131211203435 dot 43531ca5 at gandalf dot local dot home>
(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