Prev: HID: add multi-input quirk for NextWindow Touchscreen.
Next: [RFC 0/2] macvtap, second try
From: Steven Rostedt on 3 Dec 2009 09:20
On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> be included with CREATE_TRACE_EVENTS defined
>
> I've been workingon adding a few tracepoints to the network stack, and in so
> doing it was convienient for me to add a second include file to
> net/core/net-traces.c with TRACE_EVENTS defined in them. net-traces.c defines
> CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> duplicate definitions of __tpstrtab_<name>. I tracked down the bug to find that
> define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> run through define_trace.h (which is included from skb.h, included in
> net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> to start a new cycle with the next header.
>
> The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> This places us back in the proper state to define a new set of tracepoints in a
> subsequent header file. Not sure if theres a better way to handle this, but
> this worked well for me, allowing me to include multiple headers with
> TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
>

Nice, just some comments below.

> Signed-off-by: Neil Horman <nhorman(a)tuxdriver.com>
>
>
> linux/tracepoint.h | 18 ++++++++++++++++++
> trace/define_trace.h | 1 +
> 2 files changed, 19 insertions(+)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 2aac8a8..562d8ba 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -173,6 +173,24 @@ static inline void tracepoint_synchronize_unregister(void)
> * trace event headers under one "CREATE_TRACE_POINTS" the first include
> * will override the TRACE_EVENT and break the second include.
> */
> +#ifndef DECLARE_TRACE
> +#define DECLARE_TRACE(name, proto, args) \
> + extern struct tracepoint __tracepoint_##name; \
> + static inline void trace_##name(proto) \
> + { \
> + if (unlikely(__tracepoint_##name.state)) \
> + __DO_TRACE(&__tracepoint_##name, \
> + TP_PROTO(proto), TP_ARGS(args)); \
> + } \
> + static inline int register_trace_##name(void (*probe)(proto)) \
> + { \
> + return tracepoint_probe_register(#name, (void *)probe); \
> + } \
> + static inline int unregister_trace_##name(void (*probe)(proto)) \
> + { \
> + return tracepoint_probe_unregister(#name, (void *)probe);\
> + }
> +#endif
>

You duplicate the DECLARE_TRACE from above and place it into an
unprotected area. Why not move it instead of duplicating it. This is
just a bug waiting to happen if we ever need to modify DECLARE_TRACE.

If you change it to:

#if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)

#define DECLARE_TRACE(...) ....

#endif

Then you can remove the first definition of it. The first time
processing the file, when it hits the #ifndef DECLARE_TRACE in the
_LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
and then when it exits that conditional, it does.

-- Steve


> #ifndef TRACE_EVENT
> /*
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..a3e0095 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -64,6 +64,7 @@
> #undef TRACE_EVENT
> #undef TRACE_EVENT_FN
> #undef TRACE_HEADER_MULTI_READ
> +#undef DECLARE_TRACE
>
> /* Only undef what we defined in this file */
> #ifdef UNDEF_TRACE_INCLUDE_FILE

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 3 Dec 2009 14:50
On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:
> On Thu, Dec 03, 2009 at 09:15:26AM -0500, Steven Rostedt wrote:
> > On Wed, 2009-12-02 at 14:19 -0500, Neil Horman wrote:
> > > Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> > > be included with CREATE_TRACE_EVENTS defined
> > >
> > > I've been workingon adding a few tracepoints to the network stack, and in so
> > > doing it was convienient for me to add a second include file to
> > > net/core/net-traces.c with TRACE_EVENTS defined in them. net-traces.c defines
> > > CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> > > duplicate definitions of __tpstrtab_<name>. I tracked down the bug to find that
> > > define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> > > run through define_trace.h (which is included from skb.h, included in
> > > net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> > > to start a new cycle with the next header.
> > >
> > > The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> > > end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> > > This places us back in the proper state to define a new set of tracepoints in a
> > > subsequent header file. Not sure if theres a better way to handle this, but
> > > this worked well for me, allowing me to include multiple headers with
> > > TRACE_EVENT macros in a c file with CREATE_TRACE_POINTS defined.
> > >
> >
> > Nice, just some comments below.
> ><snip>
> >
> > You duplicate the DECLARE_TRACE from above and place it into an
> > unprotected area. Why not move it instead of duplicating it. This is
> > just a bug waiting to happen if we ever need to modify DECLARE_TRACE.
> >
> > If you change it to:
> >
> > #if !defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> >
> > #define DECLARE_TRACE(...) ....
> >
> > #endif
> >
> > Then you can remove the first definition of it. The first time
> > processing the file, when it hits the #ifndef DECLARE_TRACE in the
> > _LINUX_TRACEPOINT_H conditional, it still does not define DECLARE_TRACE,
> > and then when it exits that conditional, it does.
>
>
> Yeah, that makes sense to me. Although I think you mean if
> defined(CONFIG_TRACEPOINTS) rather than if !defined(CONFIG_TRACEPOINTS). The

Yep, that's what I meant ;-)

> case I'm moving to outside the unprotected area is the one that we use if
> tracepoints are enabled. I just tested this new patch, and it works well for
> me. Thanks!
>
> Neil
>
> Signed-off-by: Neil Horman <nhorman(a)tuxdriver.com>

OK, I'll give it some testing tonight.

-- Steve


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 7 Dec 2009 15:10
On Thu, 2009-12-03 at 14:35 -0500, Neil Horman wrote:

> Fix tracing infrastructure to allow multiple header files with TRACE_EVENTS to
> be included with CREATE_TRACE_EVENTS defined
>
> I've been workingon adding a few tracepoints to the network stack, and in so
> doing it was convienient for me to add a second include file to
> net/core/net-traces.c with TRACE_EVENTS defined in them. net-traces.c defines
> CREATE_TRACE_EVENTS, and during the build it was failing, complaining about
> duplicate definitions of __tpstrtab_<name>. I tracked down the bug to find that
> define_trace.h redefined DECLARE_TRACE to be DEFINE_TRACE, so after the first
> run through define_trace.h (which is included from skb.h, included in
> net-trace.c first), the DECLARE_TRACE macro was left with an improper definition
> to start a new cycle with the next header.
>
> The fix I came up with was to make sure that DECLARE_TRACE was undefined at the
> end of define_trace.h, and to add a conditional re-definition in tracepoint.h.
> This places us back in the proper state to define a new set of tracepoints in a
> subsequent header file.
>
> Signed-off-by: Neil Horman <nhorman(a)tuxdriver.com>
>

By itself, this patch breaks the build (for me). I take it that you
converted the napi.h file to use TRACE_EVENT. Currently it uses a
DECLARE_TRACE, which will break with this patch. We need a patch that
removes that DECLARE_TRACE before we can apply this patch.

To keep bisectablity, we either need to make one patch that converts the
napi to TRACE_EVENT and this patch, or we need to first remove the
napi.h DECLARE_TRACE, add this patch, and then one that adds
TRACE_EVENT.

-- Steve

> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 2aac8a8..311abbf 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -58,28 +58,6 @@ struct tracepoint {
> rcu_read_unlock_sched_notrace(); \
> } while (0)
>
> -/*
> - * Make sure the alignment of the structure in the __tracepoints section will
> - * not add unwanted padding between the beginning of the section and the
> - * structure. Force alignment to the same alignment as the section start.
> - */
> -#define DECLARE_TRACE(name, proto, args) \
> - extern struct tracepoint __tracepoint_##name; \
> - static inline void trace_##name(proto) \
> - { \
> - if (unlikely(__tracepoint_##name.state)) \
> - __DO_TRACE(&__tracepoint_##name, \
> - TP_PROTO(proto), TP_ARGS(args)); \
> - } \
> - static inline int register_trace_##name(void (*probe)(proto)) \
> - { \
> - return tracepoint_probe_register(#name, (void *)probe); \
> - } \
> - static inline int unregister_trace_##name(void (*probe)(proto)) \
> - { \
> - return tracepoint_probe_unregister(#name, (void *)probe);\
> - }
> -
>
> #define DEFINE_TRACE_FN(name, reg, unreg) \
> static const char __tpstrtab_##name[] \
> @@ -173,6 +151,29 @@ static inline void tracepoint_synchronize_unregister(void)
> * trace event headers under one "CREATE_TRACE_POINTS" the first include
> * will override the TRACE_EVENT and break the second include.
> */
> +#if defined(CONFIG_TRACEPOINTS) && !defined(DECLARE_TRACE)
> +/*
> + * Make sure the alignment of the structure in the __tracepoints section will
> + * not add unwanted padding between the beginning of the section and the
> + * structure. Force alignment to the same alignment as the section start.
> + */
> +#define DECLARE_TRACE(name, proto, args) \
> + extern struct tracepoint __tracepoint_##name; \
> + static inline void trace_##name(proto) \
> + { \
> + if (unlikely(__tracepoint_##name.state)) \
> + __DO_TRACE(&__tracepoint_##name, \
> + TP_PROTO(proto), TP_ARGS(args)); \
> + } \
> + static inline int register_trace_##name(void (*probe)(proto)) \
> + { \
> + return tracepoint_probe_register(#name, (void *)probe); \
> + } \
> + static inline int unregister_trace_##name(void (*probe)(proto)) \
> + { \
> + return tracepoint_probe_unregister(#name, (void *)probe);\
> + }
> +#endif
>
> #ifndef TRACE_EVENT
> /*
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 2a4b3bf..a3e0095 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -64,6 +64,7 @@
> #undef TRACE_EVENT
> #undef TRACE_EVENT_FN
> #undef TRACE_HEADER_MULTI_READ
> +#undef DECLARE_TRACE
>
> /* Only undef what we defined in this file */
> #ifdef UNDEF_TRACE_INCLUDE_FILE

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 7 Dec 2009 15:50
On Mon, 2009-12-07 at 15:47 -0500, Steven Rostedt wrote:

> >
> > Note this patch also converts the napi_poll tracepoint to a TRACE_EVENT. This
> > is done so that the TRACE_EVENT fix doesn't break the build, and because its
> > rather pointless I think given the current tracing infrastructure to not have
> > napi_poll be an independent event.
> >
> > Signed-off-by: Neil Horman <nhorman(a)tuxdriver.com>
>
> Thanks, I'll give it a test. But because this now touches networking
> code, I need an Acked-by from David Miller before I can pull it in.
>
> -- Steve
>
>
>
> >
> >
> > linux/tracepoint.h | 45 +++++++++++++++++++++++----------------------
> > trace/define_trace.h | 1 +
> > trace/events/napi.h | 25 ++++++++++++++++++++++---

Hmm, This really isn't networking code. But still, a patch that changes
the way a networking trace point works, I would rather have an ack.

Thanks,

-- Steve

> > 3 files changed, 46 insertions(+), 25 deletions(-)
> >
> > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> > index 2aac8a8..311abbf 100644
> > --- a/include/linux/tracepoint.h
> > +++ b/include/linux/tracepoint.h
> > @@ -58,28 +58,6 @@ struct tracepoint {
> > rcu_read_unlock_sched_notrace(); \
> > } while (0)
> >


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Steven Rostedt on 15 Dec 2009 23:20
On Tue, 2009-12-15 at 11:08 -0500, Neil Horman wrote:

Hi Neil,

I was playing with this, and I got really nasty errors in the trace
parsing tools. Then I noticed why:

> -DECLARE_TRACE(napi_poll,
> +TRACE_EVENT(napi_poll,
> +
> TP_PROTO(struct napi_struct *napi),
> - TP_ARGS(napi));
> +
> + TP_ARGS(napi),
> +
> + TP_STRUCT__entry(
> + __field( struct napi_struct *, napi)
> + ),
> +
> + TP_fast_assign(
> + __entry->napi = napi;
> + ),
> +
> + TP_printk("napi poll on napi struct %p for device %s",
> + __entry->napi, __entry->napi->dev->name)

You can't trust this! That "__entry" happens to reside on the ring
buffer. If for some reason the device goes away, this blows up when you
read the trace.

If you need to save the name of the device, then store it in the ring
buffer. You can do it with a dynamic array:

TP_STRUCT__entry(
__field( struct napi_struct *, napi)
__string( dev_name, napi->dev->name)
),

TP_fast_assign(
__entry->napi = napi;
__assign_str(dev_name, napi->dev->name);
),

TP_printk("napi poll on napi struct %p for device %s",
__entry->napi, __get_string(dev_name))

-- Steve

> +);
>
> #endif


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: HID: add multi-input quirk for NextWindow Touchscreen.
Next: [RFC 0/2] macvtap, second try