From: Axel Rasmussen <axelrasmussen@google.com>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: rostedt <rostedt@goodmis.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
Yafang Shao <laoar.shao@gmail.com>,
Andrew Morton <akpm@linux-foundation.org>,
Vlastimil Babka <vbabka@suse.cz>,
Michel Lespinasse <walken@google.com>,
Daniel Jordan <daniel.m.jordan@oracle.com>,
Davidlohr Bueso <dbueso@suse.de>, linux-mm <linux-mm@kvack.org>,
Ingo Molnar <mingo@kernel.org>,
Joonsoo Kim <iamjoonsoo.kim@lge.com>
Subject: Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Date: Thu, 24 Sep 2020 13:04:53 -0700 [thread overview]
Message-ID: <CAJHvVch3QTdArP24rNsWKkNpsCHyA7TT7s9M0=KS9+59ADjGgA@mail.gmail.com> (raw)
In-Reply-To: <166273261.68446.1600974510284.JavaMail.zimbra@efficios.com>
On Thu, Sep 24, 2020 at 12:08 PM Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>
>
> ----- On Sep 24, 2020, at 2:30 PM, rostedt rostedt@goodmis.org wrote:
>
> > On Thu, 24 Sep 2020 13:42:25 -0400 (EDT)
> > Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> >
> >> > Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.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
next prev parent reply other threads:[~2020-09-24 20:13 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-24 17:09 [PATCH 0/2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
2020-09-24 17:09 ` [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
2020-09-24 17:42 ` Mathieu Desnoyers
2020-09-24 18:19 ` Axel Rasmussen
2020-09-24 18:27 ` Mathieu Desnoyers
2020-09-24 18:30 ` Steven Rostedt
2020-09-24 19:08 ` Mathieu Desnoyers
2020-09-24 19:35 ` Steven Rostedt
2020-09-24 19:40 ` Steven Rostedt
2020-09-24 20:25 ` Mathieu Desnoyers
2020-09-24 20:05 ` Mathieu Desnoyers
2020-09-24 20:13 ` Steven Rostedt
2020-09-24 20:27 ` Mathieu Desnoyers
2020-09-24 20:33 ` Steven Rostedt
2020-09-25 14:41 ` Mathieu Desnoyers
2020-09-25 15:14 ` Steven Rostedt
2020-09-25 15:30 ` Mathieu Desnoyers
2020-09-25 16:26 ` Steven Rostedt
2020-09-25 17:05 ` Mathieu Desnoyers
2020-09-24 20:04 ` Axel Rasmussen [this message]
2020-09-24 17:09 ` [PATCH 2/2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper Steven Rostedt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJHvVch3QTdArP24rNsWKkNpsCHyA7TT7s9M0=KS9+59ADjGgA@mail.gmail.com' \
--to=axelrasmussen@google.com \
--cc=akpm@linux-foundation.org \
--cc=daniel.m.jordan@oracle.com \
--cc=dbueso@suse.de \
--cc=iamjoonsoo.kim@lge.com \
--cc=laoar.shao@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=mingo@kernel.org \
--cc=rostedt@goodmis.org \
--cc=vbabka@suse.cz \
--cc=walken@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).