Re: [PATCH] tracing: Trace instrumentation begin and end

From: Steven Rostedt <rostedt@goodmis.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Linux Trace Kernel <linux-trace-kernel@vger.kernel.org>,
	Masami Hiramatsu <mhiramat@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Josh Poimboeuf <jpoimboe@redhat.com>,
	Ross Zwisler <zwisler@google.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Miroslav Benes <mbenes@suse.cz>
Subject: Re: [PATCH] tracing: Trace instrumentation begin and end
Date: Wed, 22 Mar 2023 14:16:57 -0400	[thread overview]
Message-ID: <20230322141657.7d00a9bd@gandalf.local.home> (raw)
In-Reply-To: <87v8is94n6.ffs@tglx>

On Wed, 22 Mar 2023 16:39:41 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> But for figuring out how long a syscall, interrupt or exception takes
> there are exactly TWO tracepoints required, not 10 or 8. And it's bloody
> obvious where to place them, right?

Not always, and it's pretty much always architecture dependent. I was
looking for an architecture agnostic approach, as I imagine all archs will
be eventually using this.

> 
> >> instrumentation_begin()/end() is solely meant for objtool validation and
> >> nothing else.
> >> 
> >> There are clearly less horrible ways to retrieve the #PF duration, no?  
> >
> > It's not just for #PF, that was just one example. I use to use function
> > graph tracing max_depth_count=1 to verify NO_HZ_FULL to make sure there's
> > no entry into the kernel. That doesn't work anymore. Even compat syscalls
> > are not traced.  
> 
> That still works. noinstr did neither break syscall tracing nor any of
> the interrupt/exception tracepoints which can be used to validate the
> NOHZ full mechanics. Your fancy favourite script might not work anymore,
> but claiming that it can't be traced is just nonsense.

I don't remember fully, but there was something that was missing. It was
back in 2021, so I do not remember fully. That's when I first wrote this
patch. I only just redisovered it and wanted to share ;-) The only thing I
did differently since then was to add the page fault logic, because that
was something I am currently interested it.

Things could have changed since then. But if adding trace events for the
missing syscalls and around exceptions for timing purposes is OK, then I'm
happy to go that approach.

> 
> > I lost a kernel feature with the noinstr push and this is the closest that
> > comes to bringing it back.  
> 
> This is the closest _you_ came up with without thinking about it for a
> split second.
> 
> > And the more we add noinstr, the more the kernel becomes a black box
> > again.  
> 
> It does not. noinstr is a technical requirement to keep instrumentation
> out of code pathes which are not able to handle instrumentation. You
> know that very well, so please stop this theatrical handwaving and come
> back if you have sensible technical arguments.

I never said nor implied that it's not important. I'm just concerned that
we currently have no way to see when it's happening.

But I'll drop this patch and look to add specific trace events in specific
points to be able to get the timings that are useful.

Thanks,

-- Steve