| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 10 Mar 2009 09:55:05 -0400 (EDT)
From: Steven Rostedt <rostedt@...dmis.org>
To: KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
cc: linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
Andrew Morton <akpm@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Frederic Weisbecker <fweisbec@...il.com>,
Theodore Tso <tytso@....edu>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
"H. Peter Anvin" <hpa@...or.com>,
Mathieu Desnoyers <compudj@...stal.dyndns.org>,
Lai Jiangshan <laijs@...fujitsu.com>,
"Martin J. Bligh" <mbligh@...igh.org>,
"Frank Ch. Eigler" <fche@...hat.com>,
Larry Woodman <lwoodman@...hat.com>,
Jason Baron <jbaron@...hat.com>,
Tom Zanussi <tzanussi@...il.com>,
Masami Hiramatsu <mhiramat@...hat.com>,
Christoph Hellwig <hch@...radead.org>,
Jiaying Zhang <jiayingz@...gle.com>,
Steven Rostedt <srostedt@...hat.com>
Subject: Re: [PATCH 4/7] tracing: new format for specialized trace points
On Tue, 10 Mar 2009, KOSAKI Motohiro wrote:
> Hi
>
> I have few comment.
>
>
> > diff --git a/kernel/trace/trace_event_types.h b/kernel/trace/trace_event_types.h
> > index d94179a..5cca4c9 100644
> > --- a/kernel/trace/trace_event_types.h
> > +++ b/kernel/trace/trace_event_types.h
> > @@ -106,9 +106,10 @@ TRACE_EVENT_FORMAT(print, TRACE_PRINT, print_entry, ignore,
> > TRACE_STRUCT(
> > TRACE_FIELD(unsigned long, ip, ip)
> > TRACE_FIELD(unsigned int, depth, depth)
> > + TRACE_FIELD(char *, fmt, fmt)
> > TRACE_FIELD_ZERO_CHAR(buf)
> > ),
> > - TP_RAW_FMT("%08lx (%d) %s")
> > + TP_RAW_FMT("%08lx (%d) fmt:%p %s")
> > );
>
> I'm not sure about what mean /sys/kernel/debug/tracing/events/ftrace/print/format..
>
> FWIW, I think the internal representation of trace_printk() is
>
> struct print_entry {
> struct trace_entry ent;
> unsigned long ip;
> int depth;
> const char *fmt;
> u32 buf[];
> };
>
> then, buf doesn't gurantee NUL-termination, right?
Right but the record has a size value, and a buf[] means that that record
will take up the rest of the record.
The trace_printk is special since it is now a binary format. I'm going
to export the pointers and fmts next. That way a binary reader can see
what the fmt maps to.
>
>
> > @@ -665,21 +534,6 @@ event_create_dir(struct ftrace_event_call *call, struct dentry *d_events)
> > "'%s/enable' entry\n", call->name);
> > }
> >
> > - /* Only let type be writable, if we can change it */
> > - entry = debugfs_create_file("type",
> > - call->raw_init ? 0644 : 0444,
> > - call->dir, call,
> > - &ftrace_type_fops);
> > - if (!entry)
> > - pr_warning("Could not create debugfs "
> > - "'%s/type' entry\n", call->name);
> > -
> > - entry = debugfs_create_file("available_types", 0444, call->dir, call,
> > - &ftrace_available_types_fops);
> > - if (!entry)
> > - pr_warning("Could not create debugfs "
> > - "'%s/available_types' entry\n", call->name);
> > -
> > /* A trace may not want to export its format */
> > if (!call->show_format)
> > return 0;
>
> this code mean, the output type of event use debug/tracing/trace_options now, right?
No. The above code is the fact that the new format no longer gives an
option for a trace point to be a "pretty print printf" format or a "raw"
format. The new TRACE_EVENT macro is powerful enough to not need the two
options. You can now have your cake and eat it too.
>
>
> > @@ -704,13 +558,6 @@ static __init int event_trace_init(void)
> > if (!d_tracer)
> > return 0;
> >
> > - entry = debugfs_create_file("available_events", 0444, d_tracer,
> > - (void *)&show_event_seq_ops,
> > - &ftrace_avail_fops);
> > - if (!entry)
> > - pr_warning("Could not create debugfs "
> > - "'available_events' entry\n");
> > -
> > entry = debugfs_create_file("set_event", 0644, d_tracer,
> > (void *)&show_set_event_seq_ops,
> > &ftrace_set_event_fops);
>
> Why do you remove available_events file?
Arg! Good catch! That was me getting too happy with the knife.
I'll add that back.
>
>
> > diff --git a/kernel/trace/trace_events_stage_2.h b/kernel/trace/trace_events_stage_2.h
> > index 8e2e0f5..d91bf4c 100644
> > --- a/kernel/trace/trace_events_stage_2.h
> > +++ b/kernel/trace/trace_events_stage_2.h
> > @@ -32,23 +32,14 @@
> > * in binary.
> > */
> >
> > -#undef TRACE_STRUCT
> > -#define TRACE_STRUCT(args...) args
> > +#undef __entry
> > +#define __entry field
> >
> > -#undef TRACE_FIELD
> > -#define TRACE_FIELD(type, item, assign) \
> > - field->item,
> > +#undef TP_printk
> > +#define TP_printk(fmt, args...) fmt "\n", args
> >
> > -#undef TRACE_FIELD_SPECIAL
> > -#define TRACE_FIELD_SPECIAL(type_item, item, cmd) \
> > - field->item,
> > -
> > -
> > -#undef TP_RAW_FMT
> > -#define TP_RAW_FMT(args...) args
> > -
> > -#undef TRACE_EVENT_FORMAT
> > -#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt) \
> > +#undef TRACE_EVENT
> > +#define TRACE_EVENT(call, proto, args, tstruct, print, assign) \
> > enum print_line_t \
> > ftrace_raw_output_##call(struct trace_iterator *iter, int flags) \
> > { \
> > @@ -66,14 +57,76 @@ ftrace_raw_output_##call(struct trace_iterator *iter, int flags) \
> > \
> > field = (typeof(field))entry; \
> > \
> > - ret = trace_seq_printf(s, tpfmt "%s", tstruct "\n"); \
> > + ret = trace_seq_printf(s, print); \
> > if (!ret) \
> > return TRACE_TYPE_PARTIAL_LINE; \
> > \
> > return TRACE_TYPE_HANDLED; \
> > }
>
> the header comment need the same chnge too.
I'll change it once more, but if I find that I need to maintain two
versions, the comment and the header, I will simply cut out the entire
comment. I only did the comment because I thought it would be easier than
reading the macros. Not sure it is worth the trouble.
Thanks for the review,
-- Steve
>
>
> > #include <trace/trace_event_types.h>
> > diff --git a/kernel/trace/trace_events_stage_3.h b/kernel/trace/trace_events_stage_3.h
> > index 41b82b9..8e398d8 100644
> > --- a/kernel/trace/trace_events_stage_3.h
> > +++ b/kernel/trace/trace_events_stage_3.h
> > @@ -144,27 +144,15 @@ __attribute__((section("_ftrace_events"))) event_##call = { \
> > .unregfunc = ftrace_unreg_event_##call, \
> > }
> >
> > -#undef TRACE_FIELD
> > -#define TRACE_FIELD(type, item, assign)\
> > - entry->item = assign;
> > -
> > -#undef TRACE_FIELD
> > -#define TRACE_FIELD(type, item, assign)\
> > - entry->item = assign;
> > -
> > -#undef TP_CMD
> > -#define TP_CMD(cmd...) cmd
> > -
> > -#undef TRACE_ENTRY
> > -#define TRACE_ENTRY entry
> > +#undef TRACE_EVENT_FORMAT
> > +#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, raw) \
> > + TRACE_FORMAT(call, PARAMS(proto), PARAMS(args), PARAMS(fmt))
> >
> > -#undef TRACE_FIELD_SPECIAL
> > -#define TRACE_FIELD_SPECIAL(type_item, item, cmd) \
> > - cmd;
> > +#undef __entry
> > +#define __entry entry
> >
> > -#undef TRACE_EVENT_FORMAT
> > -#define TRACE_EVENT_FORMAT(call, proto, args, fmt, tstruct, tpfmt) \
> > -_TRACE_FORMAT(call, PARAMS(proto), PARAMS(args), PARAMS(fmt)) \
> > +#undef TRACE_EVENT
> > +#define TRACE_EVENT(call, proto, args, tstruct, print, assign) \
> > \
> > static struct ftrace_event_call event_##call; \
> > \
> > @@ -185,7 +173,7 @@ static void ftrace_raw_event_##call(proto) \
> > return; \
> > entry = ring_buffer_event_data(event); \
> > \
> > - tstruct; \
> > + assign; \
> > \
> > trace_current_buffer_unlock_commit(event, irq_flags, pc); \
> > } \
>
> ditto
>
> > @@ -226,10 +214,8 @@ __attribute__((__aligned__(4))) \
> > __attribute__((section("_ftrace_events"))) event_##call = { \
> > .name = #call, \
> > .system = __stringify(TRACE_SYSTEM), \
> > - .regfunc = ftrace_reg_event_##call, \
> > - .unregfunc = ftrace_unreg_event_##call, \
> > .raw_init = ftrace_raw_init_event_##call, \
> > - .raw_reg = ftrace_raw_reg_event_##call, \
> > - .raw_unreg = ftrace_raw_unreg_event_##call, \
> > + .regfunc = ftrace_raw_reg_event_##call, \
> > + .unregfunc = ftrace_raw_unreg_event_##call, \
> > .show_format = ftrace_format_##call, \
> > }
>
> ditto
>
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists