Re: [GIT PULL 0/4] perf/annotate loop detection V2, fixes

[Arnaldo Carvalho de Melo]

Em Fri, Apr 27, 2012 at 10:12:45AM -0700, Linus Torvalds escreveu:
> On Fri, Apr 27, 2012 at 9:46 AM, Arnaldo Carvalho de Melo
> <acme@xxxxxxxxxxxxx> wrote:
> > Em Fri, Apr 27, 2012 at 09:35:58AM -0700, Linus Torvalds escreveu:
> >> Btw, don't get me wrong. I really like the changes. It's just that now
> >> that the asm is almost readable, the remaining stupid default decode
> >> format details just show up so much more clearly.
> >
> > Hey, I love the comments and suggestions, keep them coming when you feel
> > like doing it.
> 
> I found another problem, and I think this one is more fundamental.
> 
> The "loop detection" is completely and utterly broken.

Arjan noticed that too ;-)
 
> It seems to think that a backwards jump implies a loop. But that's not
> at all true.

Yeah, the jump has to be conditional.

I should have reworded the "loop detection" with "basic jump arrows" in
the first place.
 
> In fact, many backwards jumps are the *reverse* of loops. They are due
> to *cold* code, that is totally uninteresting, and that was done
> out-of-line. The backwards jump is not a loop at all, it's a jump back
> to the hot code. In fact, it's often a jump back to the *exit* of a
> function, when the cold code returns an error value (but the actual
> code to do the "return" part was generated earlier as part of the hot
> normal case code).

That makes me think if another thing to add to the TODO isn't to detect
those cold code blocks from hot ones, even without profiling, just by
looking at these well known patterns.

Profiling would just prove that when done.

Such cold blocks could start folded, so that what appeared on the screen
at first would be just the hot path.
 
> So making a big deal out of it as if it was a loop can be actively
> wrong and misleading.
> 
> (And yes, I'm looking at an example of that right now -
> __d_lookup_rcu() has this, for example)
> 
> Now, it's often nice to see the line to find the branch target
> (whether it's a loop or not), but you don't show them for forwards
> branches, you only show them for backwards branches, as if the
> backwards branches were somehow more important. But they really really
> aren't.

Yeah, there has to be a key to press over jumps to ask for it to point
to its target.

In addition another key to press anywhere to ask for the current loop,
if any, and this key, if pressed twice, should show a capped number of
loops, something like that.

- Arnaldo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/