Re: [RFC patch 3/5] ftrace trace event add missing semicolumn

[Frederic Weisbecker]

On Wed, Jan 05, 2011 at 08:52:42AM -0500, Mathieu Desnoyers wrote:
> * Frederic Weisbecker (fweisbec@xxxxxxxxx) wrote:
> > Hardly, as the real cleanup requires dozens of patches to clean every trace
> > events.
> 
> I know, but it's hardly a large change in terms of lines of code, not
> complexity.

Given the big coverage of such change, requiring routing pieces to relevant
maintainers, care about conflicts, etc... As a reason I'd expect more than just
a semicolon in files.i that almost nobody looks at, except few tracing people
when things break. 

I guess the win is more hiding in the Lttng tree that needs this API requirement
change? It's perfectly fine. But you should come up with stronger arguments to
convince people that the change improves the mainline tracing. The same problem
happened with your __assign() changes lately.

That said I personally don't oppose about the change.


> > So what are these arrays of events you have in mind?
> 
> Currently, Ftrace is creating a "ftrace_define_fields_##call" function for each
> event to define the event fields, which consumes space. It also has the
> ".fields" list head to keep a dynamically generated list of event fields.
> 
> By allowing creation of arrays of events, we can do the following: turn the
> dynamically created list of event fields into a first-level const array listing
> event fields. We can use ARRAY_SIZE() on this array to know its size,
> statically. Then, in a following trace event stage, we can create another const
> array containing tuples of (pointers to the event-specific arrays, array size).
> 
> So we get all the same information Ftrace currently gets with much less code
> overall, much less read-write data, and less dynamic initialization code.
> 
> The following relevant code snippets does the trick. It's extracted from my
> LTTng git tree. Feel free to use any variation of it if you want.
> 
> Thanks,
> 
> Mathieu
> 
> /*
>  * Stage 1 of the trace events.
>  *
>  * Create event field type metadata section.
>  * Each event produce an array of fields.
>  */
> 
> #include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */
> 
> /* Named field types must be defined in lttng-types.h */
> 
> #undef __field
> #define __field(_type, _item)                                   \
>         { .name = #_item, .type = { .atype = atype_integer, .name = #_type} },
> 
> [... same for __array, __dynamic_array, __string, ...]
> 
> #undef TP_STRUCT__entry
> #define TP_STRUCT__entry(args...) args  /* Only one used in this phase */
> 
> #undef DECLARE_EVENT_CLASS
> #define DECLARE_EVENT_CLASS(_name, _proto, _args, _tstruct, _assign, _print) \
>         static const struct lttng_event_field __event_fields___##_name[] = { \
>                 _tstruct                                                     \
>         };
> 
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> 
> 
> /*
>  * Stage 2 of the trace events.
>  *
>  * Create an array of events.
>  */
> 
> /* Named field types must be defined in lttng-types.h */
> 
> #include "lttng-events-reset.h" /* Reset all macros within TRACE_EVENT */
> 
> #undef DEFINE_EVENT
> #define DEFINE_EVENT(_template, _name, _proto, _args)                          \
>                 {                                                              \
>                         .fields = __event_fields___##_template,                \
>                         .name = #_name,                                        \
>                         .nr_fields = ARRAY_SIZE(__event_fields___##_template), \
>                 },
> 
> #define TP_ID1(_token, _system) _token##_system
> #define TP_ID(_token, _system)  TP_ID1(_token, _system)
> 
> static const struct lttng_event_desc TP_ID(__event_desc___, TRACE_SYSTEM)[] = {
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> };
> 
> #undef TP_ID1
> #undef TP_ID

Looks good!

I might be missing corner things but it seems this would reduce the code
footprint (one function less) and turn more rw into ro datas.

So it seems to be a very valuable reason to change the semicolon requirement
all over the place.

If you come up with this feature along the massive semicolon requirement change,
we will probably happily apply the whole.

But coming with only the semicolon change is more like an empty shell.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/