code review 159990043: runtime: make pprof a little nicer

runtime: make pprof a little nicer

Update issue 8942

This does not fully address issue 8942 but it does make
the profiles much more useful, until that issue can be
fixed completely.

[rsc, 2014-10-16 18:05:41]

Hello r (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[dvyukov, 2014-10-16 18:20:29]

LGTM

https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c
File src/runtime/proc.c (right):

https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c#newcod...
src/runtime/proc.c:2539: sp = (uint8*)gp->sched.sp;
this code is roughly as subtle as the code above that got 70-line comment
we can end up with e.g. sp=0 here, if it's cleared before g is restored in gogo
on in back jump in mcall
i've looked at gogo/mcall, and it seems that current code sequences are ok

[rsc, 2014-10-16 18:26:01]

https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c
File src/runtime/proc.c (right):

https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c#newcod...
src/runtime/proc.c:2539: sp = (uint8*)gp->sched.sp;
On 2014/10/16 18:20:28, dvyukov wrote:
> this code is roughly as subtle as the code above that got 70-line comment
> we can end up with e.g. sp=0 here, if it's cleared before g is restored in
gogo
> on in back jump in mcall
> i've looked at gogo/mcall, and it seems that current code sequences are ok

I don't understand. If we are running on g0 then curg must have a valid, saved
pc/sp. We always (and must) do a full curg state save before starting to switch
onto g0. Are you worried about the transition back? It seems like that should be
okay too - it even if we see gp == mp->g0 because it is in the middle of
reloading the state, the saved state is still valid.

I don't see anywhere that we clear gp->sched.sp. What am I missing?

Thanks.

[dvyukov, 2014-10-16 18:29:53]

On 2014/10/16 18:26:01, rsc wrote:
> https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c
> File src/runtime/proc.c (right):
> 
>
https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c#newcod...
> src/runtime/proc.c:2539: sp = (uint8*)gp->sched.sp;
> On 2014/10/16 18:20:28, dvyukov wrote:
> > this code is roughly as subtle as the code above that got 70-line comment
> > we can end up with e.g. sp=0 here, if it's cleared before g is restored in
> gogo
> > on in back jump in mcall
> > i've looked at gogo/mcall, and it seems that current code sequences are ok
> 
> I don't understand. If we are running on g0 then curg must have a valid, saved
> pc/sp. We always (and must) do a full curg state save before starting to
switch
> onto g0. Are you worried about the transition back? It seems like that should
be
> okay too - it even if we see gp == mp->g0 because it is in the middle of
> reloading the state, the saved state is still valid.
> 
> I don't see anywhere that we clear gp->sched.sp. What am I missing?


Here is onM switch back on amd64:

	// switch back to g
	get_tls(CX)
	MOVQ	g(CX), AX
	MOVQ	g_m(AX), BX
	MOVQ	m_curg(BX), AX
	MOVQ	AX, g(CX)
	MOVQ	(g_sched+gobuf_sp)(AX), SP
	MOVQ	$0, (g_sched+gobuf_sp)(AX)
	RET

If we manage to do "MOVQ	$0, (g_sched+gobuf_sp)(AX)" before"MOVQ	AX, g(CX)"
(which is just 2 lines above), new traceback will crash as far as I see.

Perf dashboard will stress cpu profiler well enough.

[rsc, 2014-10-16 18:43:14]

On 2014/10/16 18:29:53, dvyukov wrote:
> On 2014/10/16 18:26:01, rsc wrote:
> > https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c
> > File src/runtime/proc.c (right):
> > 
> >
>
https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c#newcod...
> > src/runtime/proc.c:2539: sp = (uint8*)gp->sched.sp;
> > On 2014/10/16 18:20:28, dvyukov wrote:
> > > this code is roughly as subtle as the code above that got 70-line comment
> > > we can end up with e.g. sp=0 here, if it's cleared before g is restored in
> > gogo
> > > on in back jump in mcall
> > > i've looked at gogo/mcall, and it seems that current code sequences are ok
> > 
> > I don't understand. If we are running on g0 then curg must have a valid,
saved
> > pc/sp. We always (and must) do a full curg state save before starting to
> switch
> > onto g0. Are you worried about the transition back? It seems like that
should
> be
> > okay too - it even if we see gp == mp->g0 because it is in the middle of
> > reloading the state, the saved state is still valid.
> > 
> > I don't see anywhere that we clear gp->sched.sp. What am I missing?
> 
> 
> Here is onM switch back on amd64:
> 
> 	// switch back to g
> 	get_tls(CX)
> 	MOVQ	g(CX), AX
> 	MOVQ	g_m(AX), BX
> 	MOVQ	m_curg(BX), AX
> 	MOVQ	AX, g(CX)
> 	MOVQ	(g_sched+gobuf_sp)(AX), SP
> 	MOVQ	$0, (g_sched+gobuf_sp)(AX)
> 	RET
> 
> If we manage to do "MOVQ	$0, (g_sched+gobuf_sp)(AX)" before"MOVQ	AX, g(CX)"
> (which is just 2 lines above), new traceback will crash as far as I see.

Thanks. That's another instance of 'don't edit the state you're running on', and
since it follows the rule, there is no problem. That general rule should mean
that if we're on g0, then curg->sched is okay.
The big comment is about whether the actual PC/SP registers are okay to use,
which is a different story entirely.

[rsc, 2014-10-16 18:45:00]

*** Submitted as https://code.google.com/p/go/source/detail?r=421fadcef39a ***

runtime: make pprof a little nicer

Update issue 8942

This does not fully address issue 8942 but it does make
the profiles much more useful, until that issue can be
fixed completely.

LGTM=dvyukov
R=r, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/159990043

[gobot, 2014-10-16 18:49:49]

This CL appears to have broken the freebsd-amd64 builder.
See http://build.golang.org/log/acf42712fca1d6d87f7365e5d3270bd6053241b7

[r, 2014-10-16 19:14:38]

LGTM

[bradfitz, 2014-10-16 19:57:14]

CL description says "make better". How?
 On Oct 16, 2014 8:05 PM, <rsc@golang.org> wrote:

> Reviewers: r,
>
> Message:
> Hello r (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: make pprof a little nicer
>
> Update issue 8942
>
> This does not fully address issue 8942 but it does make
> the profiles much more useful, until that issue can be
> fixed completely.
>
> Please review this at https://codereview.appspot.com/159990043/
>
> Affected files (+13, -2 lines):
>   M src/runtime/proc.c
>
>
> Index: src/runtime/proc.c
> ===================================================================
> --- a/src/runtime/proc.c
> +++ b/src/runtime/proc.c
> @@ -2436,7 +2436,7 @@
>  void
>  runtime·sigprof(uint8 *pc, uint8 *sp, uint8 *lr, G *gp, M *mp)
>  {
> -       int32 n;
> +       int32 n, off;
>         bool traceback;
>         // Do not use global m in this function, use mp instead.
>         // On windows one m is sending reports about all the g's, so m
> means a wrong thing.
> @@ -2530,9 +2530,20 @@
>            ((uint8*)runtime·gogo <= pc && pc < (uint8*)runtime·gogo +
> RuntimeGogoBytes))
>                 traceback = false;
>
> +       off = 0;
> +       if(gp == mp->g0 && mp->curg != nil) {
> +               stk[0] = (uintptr)pc;
> +               off = 1;
> +               gp = mp->curg;
> +               pc = (uint8*)gp->sched.pc;
> +               sp = (uint8*)gp->sched.sp;
> +               lr = 0;
> +               traceback = true;
> +       }
> +
>         n = 0;
>         if(traceback)
> -               n = runtime·gentraceback((uintptr)pc, (uintptr)sp,
> (uintptr)lr, gp, 0, stk, nelem(stk), nil, nil, false);
> +               n = runtime·gentraceback((uintptr)pc, (uintptr)sp,
> (uintptr)lr, gp, 0, stk+off, nelem(stk)-off, nil, nil, false);
>         if(!traceback || n <= 0) {
>                 // Normal traceback is impossible or has failed.
>                 // See if it falls into several common cases.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[rsc, 2014-10-16 23:49:23]

On Thu, Oct 16, 2014 at 3:57 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> CL description says "make better". How?
>
malloc routines have callers again.

[dvyukov, 2014-10-17 06:56:22]

On Thu, Oct 16, 2014 at 10:43 PM,  <rsc@golang.org> wrote:
> On 2014/10/16 18:29:53, dvyukov wrote:
>>
>> On 2014/10/16 18:26:01, rsc wrote:
>> >
>
> https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c
>>
>> > File src/runtime/proc.c (right):
>> >
>> >
>
>
>
https://codereview.appspot.com/159990043/diff/40001/src/runtime/proc.c#newcod...
>>
>> > src/runtime/proc.c:2539: sp = (uint8*)gp->sched.sp;
>> > On 2014/10/16 18:20:28, dvyukov wrote:
>> > > this code is roughly as subtle as the code above that got 70-line
>
> comment
>>
>> > > we can end up with e.g. sp=0 here, if it's cleared before g is
>
> restored in
>>
>> > gogo
>> > > on in back jump in mcall
>> > > i've looked at gogo/mcall, and it seems that current code
>
> sequences are ok
>>
>> >
>> > I don't understand. If we are running on g0 then curg must have a
>
> valid, saved
>>
>> > pc/sp. We always (and must) do a full curg state save before
>
> starting to
>>
>> switch
>> > onto g0. Are you worried about the transition back? It seems like
>
> that should
>>
>> be
>> > okay too - it even if we see gp == mp->g0 because it is in the
>
> middle of
>>
>> > reloading the state, the saved state is still valid.
>> >
>> > I don't see anywhere that we clear gp->sched.sp. What am I missing?
>
>
>
>> Here is onM switch back on amd64:
>
>
>>         // switch back to g
>>         get_tls(CX)
>>         MOVQ    g(CX), AX
>>         MOVQ    g_m(AX), BX
>>         MOVQ    m_curg(BX), AX
>>         MOVQ    AX, g(CX)
>>         MOVQ    (g_sched+gobuf_sp)(AX), SP
>>         MOVQ    $0, (g_sched+gobuf_sp)(AX)
>>         RET
>
>
>> If we manage to do "MOVQ        $0, (g_sched+gobuf_sp)(AX)" before"MOVQ
>> AX,
>
> g(CX)"
>>
>> (which is just 2 lines above), new traceback will crash as far as I
>
> see.
>
> Thanks. That's another instance of 'don't edit the state you're running
> on', and since it follows the rule, there is no problem. That general
> rule should mean that if we're on g0, then curg->sched is okay.
> The big comment is about whether the actual PC/SP registers are okay to
> use, which is a different story entirely.


Windows builders are not happy:
http://build.golang.org/perf
They seem to just hang.

[rsc, 2014-10-17 11:14:41]

On Fri, Oct 17, 2014 at 2:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:

> Windows builders are not happy:
> http://build.golang.org/perf
> They seem to just hang.
>

true but that started 2 CLs before this one. there's a separate thread on
golang-dev. brad said he thinks maybe they got broken into.

[dvyukov, 2014-10-17 11:19:53]

On Fri, Oct 17, 2014 at 3:14 PM, Russ Cox <rsc@golang.org> wrote:
> On Fri, Oct 17, 2014 at 2:56 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> Windows builders are not happy:
>> http://build.golang.org/perf
>> They seem to just hang.
>
>
> true but that started 2 CLs before this one.

no, that started on this commit


> there's a separate thread on
> golang-dev. brad said he thinks maybe they got broken into.
>