Message-ID: <20220322110438.25c2a760@gandalf.local.home>
Date:   Tue, 22 Mar 2022 11:04:38 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Stephen Rothwell <sfr@...b.auug.org.au>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Next Mailing List <linux-next@...r.kernel.org>,
        mhiramat@...nel.org, ast@...nel.org, hjl.tools@...il.com,
        rick.p.edgecombe@...el.com, rppt@...nel.org,
        linux-toolchains@...r.kernel.org, Andrew.Cooper3@...rix.com,
        ndesaulniers@...gle.com
Subject: Re: linux-next: build warnings after merge of the tip tree

On Tue, 22 Mar 2022 15:35:54 +0100
Peter Zijlstra <peterz@...radead.org> wrote:

> On Tue, Mar 22, 2022 at 09:12:42AM -0400, Steven Rostedt wrote:
> 
> > > Suppose:
> > > 
> > > notrace func_B()
> > > {
> > > 	...
> > > }
> > > 
> > > func_A()
> > > {
> > > 	...
> > > 	return func_B();
> > > }
> > > 
> > > then inhibiting tail calls would end up looking like:  
> > 
> > If we inhibit tail calls, then we do not need to make func_B notrace.  
> 
> Dude, you're arguing in circles :-( the notrace was a given.

Why is it notrace? Sorry to be dense, but it's not a given to me.

> 
> > > func_A:
> > > 	call __fentry__
> > > 	...
> > > 	call func_B
> > > 	call __fexit__
> > > 	ret
> > > 
> > > Then A is fully traced, B is invisible, as per spec. What is the
> > > problem?  
> > 
> > The above is fine, but then func_B is not a tail call and can also be
> > traced.  
> 
> Again, B is notrace as a given. This was all about how to deal with
> notrace functions.


No, it was about how to deal with notrace functions that were tail calls.
That was what I had an issue with. Because the solution you proposed had
func_A depend on func_B executing its call __fexit__ which it would not, if
it was not traced.

> 
> I suggested inhibiting tail-call to notrace, you said no. You now seem to
> agree that solves it.

I said inhibiting tail-calls was a solution, but only inhibiting it to
notrace would probably have a significant performance impact.

I thought you were talking about adding notrace to tail calls, not the
other way around. Maybe that is our confusion in this conversation.

> 
> > > The problem you initially had, of doing a tail-call into a notrace, was
> > > that the __fexit__ call went missing, because notrace will obviously not
> > > have that. But that's avoided by inhibiting all tail-calls between
> > > notrace and !notrace functions (note that notrace must also not
> > > tail-call !notrace).  
> > 
> > I'm confused by the above. Why can't a notrace tail call a !notrace?
> > If we tail call to a
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > then the fentry and fexit show a perfectly valid trace of func_B.  
> 
> Bah; I thought I had a case this morning, but now I can't seem to recall
> :/

That happens to all of us ;-)

> 
> > > Your worry seems to stem about loosing visiblilty of !notrace functions,
> > > but AFAICT that doesn't happen.  
> > 
> > My worry is:
> > 
> > func_A:
> > 	call __fentry__
> > 	...
> > 	jmp func_B
> > 
> > Where do we do the call __fexit__ ?  
> 
> In B (or wherever if B again does a tail-call).

But there's no guarantee that the call __fexit__ will not be a nop in
func_B. Remember, these are all turned on and off. If we just trace func_A
and not func_B, we will never have a call __fexit__ for func_A.

> 
> > That was the original concern, and I think the proposed solutions have
> > convoluted our thoughts about what we are trying to fix. So let's go back
> > to the beginning, and see how to deal with it.
> > 
> > That is, we have:
> > 
> > func_C:
> > 	call __fenty__
> > 	...
> > 	call func_A:
> > 	...
> > 	call func_B:
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > func_A:
> > 	call __fentry__
> > 	...  
> 	call __ftail__
> > 	jmp func_B
> > 
> > func_B:
> > 	call __fentry__
> > 	...
> > 	call __fexit__
> > 	ret
> > 
> > Where the above is C calling A and B as normal functions, A calling B as a
> > tail call and B just being a normal function called by both A and C (and
> > many other functions).  
> 
> We need the __ftail__ thing to mark the trace-stack entry of func_A as
> complete, then any future __fexit__ will be able to pop all completed
> entries.
> 
> In recap:
> 
> 	__fentry__ -- push on trace-stack
> 	__ftail__  -- mark top-most entry complete
> 	__fexit__  -- mark top-most entry complete;
> 	              pop all completed entries

Again, this would require that the tail-calls are also being traced.

> 
> inhibit tail-calls to notrace.

Just inhibiting tail-calls to notrace would work without any of the above.
But my fear is that will cause a noticeable performance impact.

> 
> > And note, I do not want to limit function tracing (which does not rely on
> > __fexit__) just because we can't figure out how to handle __fexit__.  
> 
> I'm not following. Regular function tracing needs none of this.

The regular function tracing does not need this. Only function graph
tracing. I was thinking you were *adding* notrace to tail calls and such,
which is what I was against. But apparently that is not what you were
saying.

> 
> It's function graph tracing, kretprobes and whatever else this rethook
> stuff is about that needs this because return trampolines will stop
> working somewhere in the not too distant future.

Another crazy solution is to have:

func_A:
	call __fentry__
	...
tail:	jmp 1f 
	call 1f
	call __fexit__
	ret
1:	jmp func_B


where the compiler tells us about "tail:" and that we know that func_A ends
with a tail call, and if we want to trace the end of func_A we convert that
jmp 1f into a nop. And then we call the func_B and it's return comes back
to where we call __fexit__ and then return normally.

-- Steve

Powered by blists - more mailing lists