Re: [PATCHv6 2.6.35-rc3-tip 8/12] trace: Extract out common code for kprobes/uprobes traceevents.

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

Hi Sriker,

Srikar Dronamraju wrote:
> trace: Extract out common code for kprobes/uprobes traceevents.
> 
> Changelog from v5: Addressed comments from Masami Hiramatsu
> 	and Steven Rostedt. Also shared lot more code from kprobes
>         traceevents.
> 
> Move common parts of trace_kprobe.c and trace_uprobe.c
> Adjust kernel/trace/trace_kprobe.c after moving common code to
> kernel/trace/trace_probe.h and kernel/trace/trace_probe.c.

Great! That's what I need.

I have just some comments. But basically it looks good to me.

> 
> TODO: Merge both events to a single probe event.
> 
> Signed-off-by: Srikar Dronamraju <srikar@linux.vnet.ibm.com>
> ---
[...]
> diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c
> new file mode 100644
> index 0000000..91056d8
> --- /dev/null
> +++ b/kernel/trace/trace_probe.c
> @@ -0,0 +1,521 @@
> +/*
> + * Common code for probe-based Dynamic events.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + *
> + * Copyright (C) IBM Corporation, 2010
> + * Author:     Srikar Dronamraju
> + *
> + * Derived from kernel/trace/trace_kprobe.c written by
> + * Masami Hiramatsu <mhiramat@redhat.com>
> + */
> +
> +#include "trace_probe.h"
> +
> +const char *reserved_field_names[] = {
> +	"common_type",
> +	"common_flags",
> +	"common_preempt_count",
> +	"common_pid",
> +	"common_tgid",
> +	"common_lock_depth",
> +	FIELD_STRING_IP,
> +	FIELD_STRING_RETIP,
> +	FIELD_STRING_FUNC,
> +};
> +
> +/* Printing function type */
> +#define PRINT_TYPE_FUNC_NAME(type)	print_type_##type
> +#define PRINT_TYPE_FMT_NAME(type)	print_type_format_##type
> +
> +/* Printing  in basic type function template */
> +#define DEFINE_BASIC_PRINT_TYPE_FUNC(type, fmt, cast)			\
> +static __kprobes int PRINT_TYPE_FUNC_NAME(type)(struct trace_seq *s,	\
> +						const char *name, void *data)\
> +{									\
> +	return trace_seq_printf(s, " %s=" fmt, name, (cast)*(type *)data);\
> +}									\
> +static const char PRINT_TYPE_FMT_NAME(type)[] = fmt;
> +
> +DEFINE_BASIC_PRINT_TYPE_FUNC(u8, "%x", unsigned int)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(u16, "%x", unsigned int)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(u32, "%lx", unsigned long)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(u64, "%llx", unsigned long long)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(s8, "%d", int)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(s16, "%d", int)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(s32, "%ld", long)
> +DEFINE_BASIC_PRINT_TYPE_FUNC(s64, "%lld", long long)
> +
> +__kprobes void tp_call_fetch(struct fetch_param *fprm,
> +				 struct pt_regs *regs, void *dest)
> +{
> +	return fprm->fn(regs, fprm->data, dest);
> +}

I'd like to put this call_fetch() in header, as an inline, because
it's just wrapping handler calling.

And also, I feel "tp_" implies "tracepoint". Perhaps "probe_" or
"traceprobe_" is more obvious. It will increase amount of typing,
but more readable. ;)

BTW, could you update my e-mail address to masami.hiramatsu.pt@hitachi.com ? :)

Thank you,

-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com