Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header

From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rostedt <rostedt@goodmis.org>, paulmck <paulmck@kernel.org>,
	Michael Jeanson <mjeanson@efficios.com>
Cc: linux-kernel <linux-kernel@vger.kernel.org>,
	Yafang Shao <laoar.shao@gmail.com>,
	Axel Rasmussen <axelrasmussen@google.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: Fri, 25 Sep 2020 10:41:56 -0400 (EDT)	[thread overview]
Message-ID: <176393901.69671.1601044916547.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20200924163331.0080b943@oasis.local.home>

----- On Sep 24, 2020, at 4:33 PM, rostedt rostedt@goodmis.org wrote:

> On Thu, 24 Sep 2020 16:27:34 -0400 (EDT)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
>> I'd be a bit more specific: so far, the msr.h use-case requires to include
>> directly tracepoint-defs.h and use a tracepoint_enabled() macro defined there.
>> 
>> Other less "core" header use-cases could still include tracepoint.h, as long as
>> there is no circular dependency.
> 
> Well, I'll keep tracepoint-defs.h for the msr.h case, and I could see
> if tracepoint.h is good enough for the other cases.
> 
> But does it really matter, if we only need what is in
> tracepoint-defs.h?  Why add something that may cause issues in the
> future?

The trade-off here is tracing (on) speed and code size vs header instrumentation
coverage.

Adding the trampoline as is done in msr.h adds the overhead of an extra
function call when tracing is active. It also slightly increases the code
size. This is why we don't have that extra trampoline in the common case.

The main limitation with respect to tracepoint instrumentation coverage is
header dependencies of RCU read-side synchronization. Currently, tracepoint.h
uses rcu-sched and SRCU. Moving that synchronization into a trampoline
is one way to work-around circular dependency issues.

Note that I have plans to make tracepoint.h use Tasks Trace RCU as well,
so some probes can take pages faults (especially useful for sys enter/exit).
Michael Jeanson has been working on a prototype implementing this, and
he should be able to post a RFC patch publicly soon.

That being said, I suspect that Tasks Trace RCU has fewer header dependencies
than rcu-sched and SRCU. Maybe one idea worth considering is replacing
tracepoint's use of rcu-sched and SRCU by Tasks Trace RCU altogether, if the
latter has read-side performance close to rcu-sched. This could be another way
to minimize the amount of tracepoint.h header dependencies.

With the current dependencies of tracepoint.h, I would argue that we should
only do the trampoline work-around for cases where there is an unavoidable
circular dependency, like the case of msr.h. For other headers which don't
have circular dependency issues with tracepoint.h, we should use the usual
tracepoint instrumentation because not having the trampoline provides better
tracing (on) speed and reduces (slightly) code size.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com