Issue 9873045: code review 9873045: runtime: make GC stack scanning understand whether the ... (Closed)
Descriptionruntime: make GC stack scanning understand whether the function has started or not
It is required for preemptive scheduler, because the top frame function in a preempted
goroutine has not yet started (has not decremented SP), and so requires special handling.
Currently it happens only with runtime.goexit, but it is handled specially in several places (traceback, GC).
That special handling can be removed later.
Patch Set 1 #Patch Set 2 : diff -r 7bd52af9db1c https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r 7bd52af9db1c https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 9
Patch Set 4 : diff -r e3f2b59f2404 https://dvyukov%40google.com@code.google.com/p/go/ #
MessagesTotal messages: 16
Hello golang-dev@googlegroups.com (cc: cshapiro@google.com, iant@golang.org), I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
Sign in to reply to this message.
I have not tested it on arm.
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:86: started = pc > f->entry && f->frame >= sizeof(uintptr); I am not sure what "f->frame >= sizeof(uintptr)" means here.
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:1415: // scan only args in this case. This case is already handled below... https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:1485: // The goroutine has not started. However, its incoming ...right here. Is this somehow not working for you?
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:86: started = pc > f->entry && f->frame >= sizeof(uintptr); I tripped over this code a while ago. I believe tries to determine if the function has executed beyond the prologue.
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:1415: // scan only args in this case. On 2013/05/30 19:03:39, cshapiro1 wrote: > This case is already handled below... I mean the *function*, not the *goroutine*. With preemptive scheduler there will be other functions that "not started" (preempted before SUB SP). Current code does not handle that, "fp = sp + f->frame" is incorrect for them. I think that current handling of goexit is incorrect as well. You correctly handle not started goroutines below, but bottom frame of a running goroutine is goexit as well. For that goexit frame of a running goroutine you offset fp and scan locals, both things are incorrect. Generic handling of "not started" function will allow to eliminate some corner cases as well, e.g. that special handling of not started goroutines below can be removed, and similar handling in gentraceback. https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:86: started = pc > f->entry && f->frame >= sizeof(uintptr); On 2013/05/30 19:07:24, cshapiro1 wrote: > I tripped over this code a while ago. I believe tries to determine if the > function has executed beyond the prologue. f->frame is static constant, it can't possibly do that...
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:86: started = pc > f->entry && f->frame >= sizeof(uintptr); On 2013/05/30 09:49:54, dvyukov wrote: > I am not sure what "f->frame >= sizeof(uintptr)" means here. I think this is to avoid doing the wrong thing for functions with no frame information at all, for which f->frame will be 0. In that case this code assumes that the frame size is simply sizeof(uintptr), for the return address. That's a reasonable guess for the adjustment to fp. But you shouldn't be using it to set the started variable.
Sign in to reply to this message.
PTAL https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x86.c File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/traceback_x8... src/pkg/runtime/traceback_x86.c:86: started = pc > f->entry && f->frame >= sizeof(uintptr); On 2013/06/01 00:34:02, iant wrote: > On 2013/05/30 09:49:54, dvyukov wrote: > > I am not sure what "f->frame >= sizeof(uintptr)" means here. > > I think this is to avoid doing the wrong thing for functions with no frame > information at all, for which f->frame will be 0. In that case this code > assumes that the frame size is simply sizeof(uintptr), for the return address. > That's a reasonable guess for the adjustment to fp. But you shouldn't be using > it to set the started variable. Aha! I've fixed the code and added the comment.
Sign in to reply to this message.
LGTM Wait for cshapiro.
Sign in to reply to this message.
https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c#newco... src/pkg/runtime/mgc0.c:1415: // scan only args in this case. On 2013/05/31 05:51:39, dvyukov wrote: > I mean the *function*, not the *goroutine*. > With preemptive scheduler there will be other functions that "not started" > (preempted before SUB SP). Current code does not handle that, "fp = sp + > f->frame" is incorrect for them. Okay, I understand the difference. I am somewhat unhappy with adding more complexity to gentraceback and having this spill over into addframeroots. The big loop in gentraceback handles two significant cases. The first is having a good values for the pc and sp. Unwinding is easy. The second is having pc and sp values which serve as an estimate. Unwinding requires some effort. The latter case happens at most once. The former case happens sometimes for the first iteration and always for any remaining iterations. Similarly, the new started case is false at most once and at the topmost frame. I would prefer to have only one opportunity for it to fire. I understand that the unstarted goroutine case cannot directly accomodate this situation but a one off handling around line 1508 is preferable to me. Overally, gentraceback needs to be cleaned up but this is not the change to do it in. > I think that current handling of goexit is incorrect as well. You correctly > handle not started goroutines below, but bottom frame of a running goroutine is > goexit as well. For that goexit frame of a running goroutine you offset fp and > scan locals, both things are incorrect. There are 0 locals in runtime.goexit but frame is reported as 16. This seems correct as runtime.goexit does a call. The top 8 bytes will be scanned from sp covering the argument to runtime.mcall and the 8 bytes below it will be ignored. I am not sure how the fp is relevant as it is not used when scanning the locals. What am I missing? > Generic handling of "not started" function will allow to eliminate some corner > cases as well, e.g. that special handling of not started goroutines below can be > removed, and similar handling in gentraceback. It would be nice to fold this all together. I believe the not started goroutine case is partially obsolete. The unknown arguments case can no longer occur. I would be happy to see this folded into code around line 1508.
Sign in to reply to this message.
On 2013/06/04 01:17:15, cshapiro1 wrote: > https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c > File src/pkg/runtime/mgc0.c (right): > > https://codereview.appspot.com/9873045/diff/5001/src/pkg/runtime/mgc0.c#newco... > src/pkg/runtime/mgc0.c:1415: // scan only args in this case. > On 2013/05/31 05:51:39, dvyukov wrote: > > I mean the *function*, not the *goroutine*. > > With preemptive scheduler there will be other functions that "not started" > > (preempted before SUB SP). Current code does not handle that, "fp = sp + > > f->frame" is incorrect for them. > > Okay, I understand the difference. I am somewhat unhappy with adding more > complexity to gentraceback and having this spill over into addframeroots. > > The big loop in gentraceback handles two significant cases. The first is having > a good values for the pc and sp. Unwinding is easy. The second is having pc > and sp values which serve as an estimate. Unwinding requires some effort. The > latter case happens at most once. The former case happens sometimes for the > first iteration and always for any remaining iterations. > > Similarly, the new started case is false at most once and at the topmost frame. > I would prefer to have only one opportunity for it to fire. I understand that > the unstarted goroutine case cannot directly accomodate this situation but a one > off handling around line 1508 is preferable to me. It can happen twice -- for the top frame and for the bottom frame (goexit). If we handle the top non-started function specially in addstackroots, it will dup another piece of hardcode code from gentraceback. And that code will be platform-dependent, the current handling of non-started goroutines references thechar. Moreover, gentraceback will call the callback for the top non-started function anyway (or you propose to not call it?), so it will require some handling the callback anyway. I think it's better to move in the opposite direction -- remove special handling of non-started goroutines from addstackroots, and instead handle them on the general basis in the gentraceback callback. However, this requires https://codereview.appspot.com/9868044/, but Russ seem to be against it. OK, can you write the alternative CL that does what you suggest? Probably I just do not understand what you suggest and how it will look. My next step was intended to be: -started = pc > f->entry; +started = pc > f->entry && (!topframe || !gp->preempted); > Overally, gentraceback needs to be cleaned up but this is not the change to do > it in. > > > I think that current handling of goexit is incorrect as well. You correctly > > handle not started goroutines below, but bottom frame of a running goroutine > is > > goexit as well. For that goexit frame of a running goroutine you offset fp and > > scan locals, both things are incorrect. > > There are 0 locals in runtime.goexit but frame is reported as 16. This seems > correct as runtime.goexit does a call. The top 8 bytes will be scanned from sp > covering the argument to runtime.mcall and the 8 bytes below it will be > ignored. I am not sure how the fp is relevant as it is not used when scanning > the locals. > > What am I missing? I am not saying that it does not work now. But it looks to me that it's only by coincidence. E.g. if we add some locals to goexit(), the callback will scan them, while it must not (it's not even started). > > Generic handling of "not started" function will allow to eliminate some corner > > cases as well, e.g. that special handling of not started goroutines below can > be > > removed, and similar handling in gentraceback. > > It would be nice to fold this all together. I believe the not started goroutine > case is partially obsolete. The unknown arguments case can no longer occur. I > would be happy to see this folded into code around line 1508.
Sign in to reply to this message.
On Tue, Jun 4, 2013 at 1:39 AM, <dvyukov@google.com> wrote: > I think it's better to move in the opposite direction -- remove special > handling of non-started goroutines from addstackroots, and instead > handle them on the general basis in the gentraceback callback. However, > this requires https://codereview.appspot.**com/9868044/<https://codereview.appspot.com/9868..., > but Russ seem to > be against it. > Okay. In the (hopefully) near future we will have one pointer map for each call. That includes the call generated by the linker to morestack in the function prologue. With that information we should not have the special case for unstarted goroutines or function calls when scanning the stack. OK, can you write the alternative CL that does what you suggest? > Probably I just do not understand what you suggest and how it will look. > I am going to be gone for 2 weeks starting tomorrow. We seem to have an understanding that this change is not idea, but it if you need it to move forward in the mean time it is LGTM by me. Maybe a TODO would be enough for now. I think we can revisit this once the GC information is available. > I am not saying that it does not work now. But it looks to me that it's > only by coincidence. E.g. if we add some locals to goexit(), the > callback will scan them, while it must not (it's not even started). > I see. This will also be acommodated by better pointer maps. The PC for the goexit routine is at the start of its prologue. Only the arguments to goexit can be live.
Sign in to reply to this message.
This CL will need to be revised if Dmitriy cleans up my CL tweaking the goroutine representation. In that CL there is no longer a concept of a non-started goroutine. There are just functions on the stack that haven't yet bumped the SP to allocate a frame (those are identified by pc == f->entry). It should be very easy to adjust the code in that CL. I'm happy to do that review while Carl is away. When Carl comes back perhaps we'll have fewer special cases in general. Russ
Sign in to reply to this message.
On Wed, Jun 5, 2013 at 2:37 PM, Russ Cox <rsc@golang.org> wrote: > This CL will need to be revised if Dmitriy cleans up my CL tweaking the > goroutine representation. In that CL there is no longer a concept of a > non-started goroutine. There are just functions on the stack that haven't > yet bumped the SP to allocate a frame (those are identified by pc == > f->entry). It should be very easy to adjust the code in that CL. I'm happy > to do that review while Carl is away. When Carl comes back perhaps we'll > have fewer special cases in general. That all sounds good to me. As an aside, is the pc==f->entry correspondence correct on the ARM? I remember having some issues with that a while ago. I might be completely wrong but it should be easy to check as part of the aforementioned change.
Sign in to reply to this message.
Yes, pc==f->entry is the right test even on ARM. However, on the ARM, on entry to a function, there is no pushed return PC, so the arguments begin at the stack pointer, not one word later as on the x86.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
