Message-ID: <CAJHvVch3QTdArP24rNsWKkNpsCHyA7TT7s9M0=KS9+59ADjGgA@mail.gmail.com>
Date:   Thu, 24 Sep 2020 13:04:53 -0700
From:   Axel Rasmussen <axelrasmussen@...gle.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     rostedt <rostedt@...dmis.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Yafang Shao <laoar.shao@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        Michel Lespinasse <walken@...gle.com>,
        Daniel Jordan <daniel.m.jordan@...cle.com>,
        Davidlohr Bueso <dbueso@...e.de>,
        linux-mm <linux-mm@...ck.org>, Ingo Molnar <mingo@...nel.org>,
        Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is
 enabled in a header

On Thu, Sep 24, 2020 at 12:08 PM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
>
>
> ----- On Sep 24, 2020, at 2:30 PM, rostedt rostedt@...dmis.org wrote:
>
> > On Thu, 24 Sep 2020 13:42:25 -0400 (EDT)
> > Mathieu Desnoyers <mathieu.desnoyers@...icios.com> wrote:
> >
> >> > Signed-off-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
> >> > ---
> >> > Documentation/trace/tracepoints.rst | 25 ++++++++++++++++++++++
> >> > include/linux/tracepoint-defs.h     | 33 +++++++++++++++++++++++++++++
> >> > 2 files changed, 58 insertions(+)
> >> >
> >> > diff --git a/Documentation/trace/tracepoints.rst
> >> > b/Documentation/trace/tracepoints.rst
> >> > index 6e3ce3bf3593..833d39ee1c44 100644
> >> > --- a/Documentation/trace/tracepoints.rst
> >> > +++ b/Documentation/trace/tracepoints.rst
> >> > @@ -146,3 +146,28 @@ with jump labels and avoid conditional branches.
> >> >       define tracepoints. Check http://lwn.net/Articles/379903,
> >> >       http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362
> >> >       for a series of articles with more details.
> >> > +
> >> > +If you require calling a tracepoint from a header file, it is not
> >> > +recommended to call one directly or to use the trace_<tracepoint>_enabled()
> >> > +function call, as tracepoints in header files can have side effects if a
> >> > +header is included from a file that has CREATE_TRACE_POINTS set. Instead,
> >> > +include tracepoint-defs.h and use trace_enabled().
> >>
> >> Tracepoints per-se have no issues being used from header files. The TRACE_EVENT
> >> infrastructure seems to be the cause of this problem. We should fix trace events
> >> rather than require all users to use weird work-arounds thorough the kernel code
> >> base.
> >
> > That's like saying "let's solve include hell". Note, in the past there has
> > also been issues with the headers included also having issues including
> > other headers and cause a dependency loop.
>
> AFAIU, there are a few scenarios we care about here:
>
> 1) Includes done by tracepoint.h (directly and indirectly). Some additional
>    includes may have crept in and bloated tracepoint.h since its original
>    implementation. We should identify and fix those.

I was recalling that tracepoint.h transiently included linux/mm.h, but
that appears to not be the case. Either I'm misremembering, or I at
some point fixed the issue by switching to forward declarations
instead.

>
> 2) Includes done by trace event headers when CREATE_TRACE_POINTS is defined.
>    define_trace.h already takes care of #undef/#define CREATE_TRACE_POINTS to
>    prevent recursion, so it should not be an issue.
>
> 3) Includes done by a compile unit after #define CREATE_TRACE_POINTS, but which
>    are not meant to create trace point probes. For this case, requiring to
>    #undef CREATE_TRACE_POINTS after all the relevant headers are included should
>    solve it.
>
> So what additional issues am I missing here ?
>
> >
> > But the magic of trace events (for both perf and ftrace, sorry if you
> > refused to use it),
>
> I cannot not use it to this day because changes I needed to upstream in order
> to make it useful to me were rejected.
>
> > is that people who add tracepoints do not need to know
> > how tracepoints work. There's no adding of registering of them, or anything
> > else. The formats and everything they record appear in the tracefs file
> > system.
>
> That's all fine by me.
>
> >
> > How are your trace events created?  All manual, or do you have helper
> > macros.
>
> I suspect you mean LTTng's trace events. Those are created with helper macros
> (LTTNG_TRACEPOINT_EVENT) which have a few improvements over TRACE_EVENT, namely:
>
> - Ability to create arrays of events (because lots of semicolons are removed), removing
>   the need to dynamically register each event at init time,
> - Ability to do pre-filtering of events, before they hit the ring buffer, allowed by
>   combining TP_struct and TP_fast_assign into a single structured TP_FIELDS.
>
> LTTng also has an include pass which uses TRACE_EVENT from the kernel to perform
> tracepoint prototype signature validation of LTTNG_TRACEPOINT_EVENT against TRACE_EVENT
> prototypes.
>
> > Would these be safe if a bunch were included?
>
> Can you elaborate on this question ? I have a hard time figuring out what
> scenario(s) you are after.
>
> >
> >>
> >> I am not against the idea of a tracepoint_enabled(tp), but I am against the
> >> motivation behind this patch and the new tracepoint user requirements it
> >> documents.
> >
> > It removes the open coded code that has been in the kernel for the last 4
> > years.
>
> There are indeed many cases where a tracepoint_enabled() macro makes sense. In
> some situations we encounter in user-space for lttng-ust, there is need to
> prepare data before it is passed as tracepoint arguments. Having this "enabled"
> macros allows to only prepare the data when needed.
>
> >
> >>
> >> > +
> >> > +In a C file::
> >> > +
> >> > +  void do_trace_foo_bar_wrapper(args)
> >> > +  {
> >> > +          trace_foo_bar(args);
> >> > +  }
> >> > +
> >> > +In the header file::
> >> > +
> >> > +  DECLEARE_TRACEPOINT(foo_bar);
> >> > +
> >> > +  static inline void some_inline_function()
> >> > +  {
> >> > +          [..]
> >> > +          if (trace_enabled(foo_bar))
> >>
> >> Is it trace_enabled() or tracepoint_enabled() ? There is a mismatch
> >> between the commit message/code and the documentation.
> >
> > Yes, it should be tracepoint_enabled(). Thanks for catching that.
> >
> > Anyway, this shouldn't affect you in any way, as it's just adding wrappers
> > around locations that have been doing this for years.
> >
> > If you want, I can change the name to trace_event_enabled() and put the
> > code in trace_events-defs.h instead. Which would simply include
> > tracepoints-defs.h and have the exact same code.
>
> I'm ok with tracepoint_enabled(). However, I would have placed it in tracepoint.h
> rather than tracepoint-defs.h, and we should figure out why people complain that
> tracepoint.h is including headers too eagerly.
>
> Thanks,
>
> Mathieu
>
> >
> > -- Steve
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

Powered by blists - more mailing lists