Re: [PATCH 1/8] ksym_tracer: Extract trace entry from struct trace_ksym

Frederic Weisbecker
2009-07-07T21:25:38Z

On Tue, Jul 07, 2009 at 01:52:36PM +0800, Li Zefan wrote:
> struct trace_ksym is used as an entry in hbp list, and is also
> used as trace_entry stored in ring buffer.
>
> This is not necessary and is a waste of memory in ring buffer.
>
> There is also a bug that dereferencing field->ksym_hbp in
> ksym_trace_output() can be invalid.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> kernel/trace/trace.h | 13 ++++---------
> kernel/trace/trace_ksym.c | 26 ++++++++++++++++++--------
> 2 files changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
> index 61b4e94..6868902 100644
> --- a/kernel/trace/trace.h
> +++ b/kernel/trace/trace.h
> @@ -215,17 +215,12 @@ struct syscall_trace_exit {
> #define KSYM_SELFTEST_ENTRY "ksym_selftest_dummy"
> extern int process_new_ksym_entry(char *ksymname, int op, unsigned long addr);
>
> -struct trace_ksym {
> +struct ksym_trace_entry {
> struct trace_entry ent;
> - struct hw_breakpoint *ksym_hbp;
> - unsigned long ksym_addr;
> unsigned long ip;
> -#ifdef CONFIG_PROFILE_KSYM_TRACER
> - unsigned long counter;
> -#endif
> - struct hlist_node ksym_hlist;
> + unsigned char type;
> char ksym_name[KSYM_NAME_LEN];
> - char p_name[TASK_COMM_LEN];
> + char cmd[TASK_COMM_LEN];
> };
>
> /*
> @@ -343,7 +338,7 @@ extern void __ftrace_bad_type(void);
> TRACE_SYSCALL_ENTER); \
> IF_ASSIGN(var, ent, struct syscall_trace_exit, \
> TRACE_SYSCALL_EXIT); \
> - IF_ASSIGN(var, ent, struct trace_ksym, TRACE_KSYM); \
> + IF_ASSIGN(var, ent, struct ksym_trace_entry, TRACE_KSYM);\
> __ftrace_bad_type(); \
> } while (0)
>
> diff --git a/kernel/trace/trace_ksym.c b/kernel/trace/trace_ksym.c
> index eef97e7..085ff05 100644
> --- a/kernel/trace/trace_ksym.c
> +++ b/kernel/trace/trace_ksym.c
> @@ -37,6 +37,15 @@
> #define KSYM_TRACER_OP_LEN 3 /* rw- */
> #define KSYM_FILTER_ENTRY_LEN (KSYM_NAME_LEN + KSYM_TRACER_OP_LEN + 1)
>
> +struct trace_ksym {
> + struct hw_breakpoint *ksym_hbp;
> + unsigned long ksym_addr;
> +#ifdef CONFIG_PROFILE_KSYM_TRACER
> + unsigned long counter;
> +#endif
> + struct hlist_node ksym_hlist;
> +};
> +
> static struct trace_array *ksym_trace_array;
>
> static unsigned int ksym_filter_entry_count;
> @@ -71,7 +80,7 @@ void ksym_hbp_handler(struct hw_breakpoint *hbp, struct pt_regs *regs)
> {
> struct ring_buffer_event *event;
> struct trace_array *tr;
> - struct trace_ksym *entry;
> + struct ksym_trace_entry *entry;
> int pc;
>
> if (!ksym_tracing_enabled)
> @@ -85,11 +94,12 @@ void ksym_hbp_handler(struct hw_breakpoint *hbp, struct pt_regs *regs)
> if (!event)
> return;
>
> - entry = ring_buffer_event_data(event);
> + entry = ring_buffer_event_data(event);
> + entry->ip = instruction_pointer(regs);
> + entry->type = hbp->info.type;
> strlcpy(entry->ksym_name, hbp->info.name, KSYM_SYMBOL_LEN);
> - entry->ksym_hbp = hbp;
> - entry->ip = instruction_pointer(regs);
> - strlcpy(entry->p_name, current->comm, TASK_COMM_LEN);
> + strlcpy(entry->cmd, current->comm, TASK_COMM_LEN);

Ah, I did not realized that the cmdline was that roughly saved.
It would be more ring-buffer-friendly to use tracing_record_cmdline.

> +
> #ifdef CONFIG_PROFILE_KSYM_TRACER
> ksym_collect_stats(hbp->info.address);
> #endif /* CONFIG_PROFILE_KSYM_TRACER */
> @@ -380,7 +390,7 @@ static enum print_line_t ksym_trace_output(struct trace_iterator *iter)
> {
> struct trace_entry *entry = iter->ent;
> struct trace_seq *s = &iter->seq;
> - struct trace_ksym *field;
> + struct ksym_trace_entry *field;
> char str[KSYM_SYMBOL_LEN];
> int ret;
>
> @@ -389,12 +399,12 @@ static enum print_line_t ksym_trace_output(struct trace_iterator *iter)
>
> trace_assign_type(field, entry);
>
> - ret = trace_seq_printf(s, "%-15s %-5d %-3d %-20s ", field->p_name,
> + ret = trace_seq_printf(s, "%-15s %-5d %-3d %-20s ", field->cmd,
> entry->pid, iter->cpu, field->ksym_name);
> if (!ret)
> return TRACE_TYPE_PARTIAL_LINE;
>
> - switch (field->ksym_hbp->info.type) {
> + switch (field->type) {
> case HW_BREAKPOINT_WRITE:
> ret = trace_seq_printf(s, " W ");
> break;

Thanks!

Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

Prasad, any objection?