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=unavailable 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 8C813C4727E for ; Thu, 24 Sep 2020 18:20:13 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 22EAD2311B for ; Thu, 24 Sep 2020 18:20:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="GdfyMCbq" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 22EAD2311B 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 676C96B0062; Thu, 24 Sep 2020 14:20:12 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 64DC96B0068; Thu, 24 Sep 2020 14:20:12 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 563086B006C; Thu, 24 Sep 2020 14:20:12 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0035.hostedemail.com [216.40.44.35]) by kanga.kvack.org (Postfix) with ESMTP id 34F236B0062 for ; Thu, 24 Sep 2020 14:20:12 -0400 (EDT) Received: from smtpin17.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id EE01D180AD81A for ; Thu, 24 Sep 2020 18:20:11 +0000 (UTC) X-FDA: 77298769422.17.glass54_151667a27161 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin17.hostedemail.com (Postfix) with ESMTP id C490A180D0180 for ; Thu, 24 Sep 2020 18:20:11 +0000 (UTC) X-HE-Tag: glass54_151667a27161 X-Filterd-Recvd-Size: 8752 Received: from mail-pj1-f67.google.com (mail-pj1-f67.google.com [209.85.216.67]) by imf07.hostedemail.com (Postfix) with ESMTP for ; Thu, 24 Sep 2020 18:20:10 +0000 (UTC) Received: by mail-pj1-f67.google.com with SMTP id t7so164476pjd.3 for ; Thu, 24 Sep 2020 11:20:10 -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=eQMwhMR95n+3FqBYI5WeAox8ddPXZxaoyC5f5KTzv1Q=; b=GdfyMCbqMOKTkZTGUricXEzpXRPXMc0br4yflxhC7hdlpcAcJ41FcQce+qdccc3z87 MMHOf+8H5Qr7RybcJieBQ5EQNHlLU5mUBdxeE64mi7MqDPzFTD8FlUGOrNkDsD3zyonP cZvcfl8xL5uDWI7gIgAXwh2E3zr9TiPG7nH676ZY8oREoV9x/hm2xZY621hV4+O5Obyn FzkZiPfL3s8lCz0lJhzkW2b7vZnlvGlJmmvTq3UFIh29qUQ4q7K0h7b0kWCmUVcE3j22 8ctZwQVXWEnL7SHF/Pk+KGEEY4RL75w0Lg0VL8ZNiJbHODxZsUa+28YyqqxNSOWrwW7p 5Wnw== 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=eQMwhMR95n+3FqBYI5WeAox8ddPXZxaoyC5f5KTzv1Q=; b=ISJ0Fk9DkMRQd8KqPGGLG9CzfJgmJfeHIQOj+fjytTZMjIIDe+0cnxOLIc/fwn74rc RvZ/enxCcSKH83piz5xnqM1d7flf7qFL+Znqc8vORNzBZHngaDQlDpGNvpywxHrw2/f/ 0O2+uKm3VYhrsHu9GFUo0H80mBex5iRQgf0mK8gJ5WL6dm1dvQ5D5dcuPFJKUd1b00C7 xMQvltOxCIRPmAvQp6uYlFYzB0y/w4zcugWbv+JtoVPrHLuh1qe0rHFpJNKW4eWRKNNZ HsQR6st4/KVu2Ca4pcEIcYIZ5RiKGY+VPaS5bmrvx7uyuzAvme6OgkMqGgVXavsGYpnx 5Ulg== X-Gm-Message-State: AOAM532PoT0jKGDILruvCeN9Lf2Rfeod34l/haVNWTobcOOe2XeOHudQ ndaFpbpLpSTRPVN48ZjVcDcqkzdnFCR/F4nsYaUTHg== X-Google-Smtp-Source: ABdhPJxGK7ykG4+qfUScNswCNWsCaTVZXUZJkcNZiTFJ0sltfxqVFMju3BXl5ivw2bkup5SuTddERmoUfauwxLhCPrg= X-Received: by 2002:a17:90a:c17:: with SMTP id 23mr182252pjs.127.1600971609587; Thu, 24 Sep 2020 11:20:09 -0700 (PDT) MIME-Version: 1.0 References: <20200924170928.466191266@goodmis.org> <20200924171846.993048030@goodmis.org> <2006335081.68212.1600969345189.JavaMail.zimbra@efficios.com> In-Reply-To: <2006335081.68212.1600969345189.JavaMail.zimbra@efficios.com> From: Axel Rasmussen Date: Thu, 24 Sep 2020 11:19:33 -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 10:42 AM Mathieu Desnoyers wrote: > > ----- On Sep 24, 2020, at 1:09 PM, rostedt rostedt@goodmis.org wrote: > > > From: "Steven Rostedt (VMware)" > > > > As tracepoints are discouraged from being added in a header because it can > > cause side effects if other tracepoints are in headers, the common > > workaround is to add a function call that calls a wrapper function in a > > C file that then calls the tracepoint. But as function calls add overhead, > > this function should only be called when the tracepoint in question is > > enabled. To get around the overhead, a static_branch can be used that only > > gets set when the tracepoint is enabled, and then inside the block of the > > static branch can contain the call to the tracepoint wrapper. > > > > Add a tracepoint_enabled(tp) macro that gets passed the name of the > > tracepoint, and this becomes a static_branch that is enabled when the > > tracepoint is enabled and is a nop when the tracepoint is disabled. > > > > 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. > > 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. Perhaps anecdotally, I've found that the situation Steven described occurs not just because of the TRACE_EVENT infrastructure. We also run into this problem when adding tracepoints under any "very core" APIs, i.e. anything that is transiently included from linux/tracepoint.h. For example, I ran into this issue while adding tracepoints under the linux/mmap_lock.h API, because that header is somehow transiently included by linux/tracepoint.h (sorry, I don't have the exact transient include path on hand; I can dig it up if it would be useful). > > > + > > +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. > > Thanks, > > Mathieu > > > + do_trace_foo_bar_wrapper(args); > > + [..] > > + } > > diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h > > index b29950a19205..ca2f1f77f6f8 100644 > > --- a/include/linux/tracepoint-defs.h > > +++ b/include/linux/tracepoint-defs.h > > @@ -48,4 +48,37 @@ struct bpf_raw_event_map { > > u32 writable_size; > > } __aligned(32); > > > > +/* > > + * If a tracepoint needs to be called from a header file, it is not > > + * recommended to call it directly, as tracepoints in header files > > + * may cause side-effects. Instead, use trace_enabled() to test > > + * if the tracepoint is enabled, then if it is, call a wrapper > > + * function defined in a C file that will then call the tracepoint. > > + * > > + * For "trace_foo()", you would need to create a wrapper function > > + * in a C file to call trace_foo(): > > + * void trace_bar(args) { trace_foo(args); } > > + * Then in the header file, declare the tracepoint: > > + * DECLARE_TRACEPOINT(foo); > > + * And call your wrapper: > > + * static inline void some_inlined_function() { > > + * [..] > > + * if (tracepoint_enabled(foo)) > > + * trace_bar(args); > > + * [..] > > + * } > > + * > > + * Note: tracepoint_enabled(foo) is equivalent to trace_foo_enabled() > > + * but is safe to have in headers, where trace_foo_enabled() is not. > > + */ > > +#define DECLARE_TRACEPOINT(tp) \ > > + extern struct tracepoint __tracepoint_##tp > > + > > +#ifdef CONFIG_TRACEPOINTS > > +# define tracepoint_enabled(tp) \ > > + static_key_false(&(__tracepoint_##tp).key) > > +#else > > +# define tracepoint_enabled(tracepoint) false > > +#endif > > + > > #endif > > -- > > 2.28.0 > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com