From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-17.4 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5B756C4363D for ; Thu, 24 Sep 2020 20:05:39 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 7065F235FD for ; Thu, 24 Sep 2020 20:05:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="ZfQhqdjE" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7065F235FD Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B30126B005C; Thu, 24 Sep 2020 16:05:37 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AE12B6B005D; Thu, 24 Sep 2020 16:05:37 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9D0046B0062; Thu, 24 Sep 2020 16:05:37 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0128.hostedemail.com [216.40.44.128]) by kanga.kvack.org (Postfix) with ESMTP id 845586B005C for ; Thu, 24 Sep 2020 16:05:37 -0400 (EDT) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 20AEB180AD802 for ; Thu, 24 Sep 2020 20:05:37 +0000 (UTC) X-FDA: 77299035114.14.point85_0903ed427161 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin14.hostedemail.com (Postfix) with ESMTP id 4D4B918229996 for ; Thu, 24 Sep 2020 20:05:32 +0000 (UTC) X-HE-Tag: point85_0903ed427161 X-Filterd-Recvd-Size: 9562 Received: from mail-pg1-f195.google.com (mail-pg1-f195.google.com [209.85.215.195]) by imf12.hostedemail.com (Postfix) with ESMTP for ; Thu, 24 Sep 2020 20:05:31 +0000 (UTC) Received: by mail-pg1-f195.google.com with SMTP id d13so385523pgl.6 for ; Thu, 24 Sep 2020 13:05:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=CEKITIo/fE6uNxy1Lcs9VsvYxy+SP2eUzht/P3sZLuo=; b=ZfQhqdjEK+3LztQ/ENGVGHbtPR6QLu/iTI2FvudqJjkfCyhvNU2hV2AncQWTtkono3 O39I9un0oCb5MG1o0ONewH5rDqnql/R6O6QEt0/CH2vOHQRu6ZOhRbRXNIBmm7tLq5p9 YeLMDPuFbXou9MlcfeVNUkB+cMpuGPYZxEZGfoGX0960mINdyGOcyj+zplo57oFay8PK MYtHLU/6Irq5KXjtPywITOjAUSGFBj212iqcQSw6O0RxYqq0p/JLbt9PGpVajLZRbiJh rM2erPouVkweSheXaGB4RLIuA1dNoFSJ0vBcKCYVPgidfieOMAeSjme/c4d/l5hI7Un5 XVlQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=CEKITIo/fE6uNxy1Lcs9VsvYxy+SP2eUzht/P3sZLuo=; b=QUsPHygdPfv4p5TetJ5z1ALJNiTyhwvwtr+929BLQifRY0KdT3jiBsHM1qW6SBeelO IazhkkawQjGRu8ZoEQFl4ME2A8DVhKcy5JyW5nsuNAadukV1YEPDcq8BbGZgGQFeJgQT gnxE72bTRx49vs2LUtJ2HHq6W3MB0nK72EBhfFyWjnBdl9zqJWLGjX/GU/G9mUOO6Rfy Tuszeau6zBLX6MmHIgFOJvvy+qo9SzyNtb5ex9jJaNxy9OaQIqNyU10qVjApHVAXVoLf Gv2K4/ER3Je0fsoEaVke5mgZAuc7zaYKpVE+PP1gf2zqEgz2r1NWQMmNvP+Ko26Sy1YS g0gg== X-Gm-Message-State: AOAM533m4b8hRKOWEaDpaw/d4mr7Iqo/42BvdgHpYUtg+XtnryW+u1CF tEGOWyy75ao2boM16pyqAOT2/hbiB1auOo+1ZHFW5Q== X-Google-Smtp-Source: ABdhPJzNUrQcMPFuCTSy3bJ0vrcfkhqbCZUH5VOGBFRcs8Xog5ppfqpt7q5T6l+70U1WCGJJAdvYF8Bb3vEYwicQ7OI= X-Received: by 2002:a63:1e0c:: with SMTP id e12mr605763pge.346.1600977930500; Thu, 24 Sep 2020 13:05:30 -0700 (PDT) MIME-Version: 1.0 References: <20200924170928.466191266@goodmis.org> <20200924171846.993048030@goodmis.org> <2006335081.68212.1600969345189.JavaMail.zimbra@efficios.com> <20200924143025.58dc3c1f@gandalf.local.home> <166273261.68446.1600974510284.JavaMail.zimbra@efficios.com> In-Reply-To: <166273261.68446.1600974510284.JavaMail.zimbra@efficios.com> From: Axel Rasmussen Date: Thu, 24 Sep 2020 13:04:53 -0700 Message-ID: Subject: Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header To: Mathieu Desnoyers Cc: rostedt , linux-kernel , Yafang Shao , Andrew Morton , Vlastimil Babka , Michel Lespinasse , Daniel Jordan , Davidlohr Bueso , linux-mm , Ingo Molnar , Joonsoo Kim Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Sep 24, 2020 at 12:08 PM Mathieu Desnoyers 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 wrote: > > > >> > Signed-off-by: Steven Rostedt (VMware) > >> > --- > >> > 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__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