|
|
Descriptionruntime: changes to g->atomicstatus (nee status) to support concurrent GC
Every change to g->atomicstatus is now done atomically so that we can
ensure that all gs pass through a gc safepoint on demand. This allows
the GC to move from one phase to the next safely. In some phases the
stack will be scanned. This CL only deals with the infrastructure that
allows g->atomicstatus to go from one state to another. Future CLs
will deal with scanning and monitoring what phase the GC is in.
The major change was to moving to using a Gscan bit to indicate that
the status is in a scan state. The only bug fix was in oldstack where
I wasn't moving to a Gcopystack state in order to block scanning until
the new stack was in place. The proc.go file is waiting for an atomic
load instruction.
Patch Set 1 #Patch Set 2 : diff -r 203e5e3455a0cc7ea42c537eb7c9f60d1dc44c32 https://code.google.com/p/go/ #Patch Set 3 : diff -r 203e5e3455a0cc7ea42c537eb7c9f60d1dc44c32 https://code.google.com/p/go/ #
Total comments: 144
Patch Set 4 : diff -r 47de34bbcca11cd65f683dc7ab0d1c6f088d25f6 https://code.google.com/p/go/ #Patch Set 5 : diff -r 47de34bbcca11cd65f683dc7ab0d1c6f088d25f6 https://code.google.com/p/go/ #
Total comments: 59
Patch Set 6 : diff -r 601ad8b625532388ff9dee86409bf32c1f2f624a https://code.google.com/p/go/ #Patch Set 7 : diff -r 53cc52c4a1dec1ea8ad862af822d1478b24f3d52 https://code.google.com/p/go/ #Patch Set 8 : diff -r 53cc52c4a1dec1ea8ad862af822d1478b24f3d52 https://code.google.com/p/go/ #
MessagesTotal messages: 24
Hello golang-codereviews@googlegroups.com (cc: dvyukov@google.com, khr@golang.org, rsc@golang.org), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
There are lots of inconsistent formatting, unrelated format changes and debug output. I've marked some of them, but not all. Please go through the code and fix the rest. 1. Go runtime does not print debug output when running. 2. All printf messages must be prefixed with "runtime: " 3. Revert unrelated whitespace changes 4. No {} around single-line if/for/while 5. No initializers in var decls 6. Function opening { is on separate line 7. "if(" and "for(" without space in between https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:354: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); all printf's must be prefixed with "runtime: ", so that user can distinguish what comes from runtime and what comes from his own code https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:430: uint32 status = runtime·readgstatus(gp); we don't use variable initializers functions look like: void foo(...) { var decls empty line code } https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:543: isscanstatus(uint32 status) { { on next line https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:581: runtime·printf("casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", prefix with "runtime: " https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:584: runtime·throw("casfromgscanstatus: gp->status is not in scan state. "); remove ". " at the end of string https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:590: runtime·castogscanstatus (G *gp, uint32 oldval, uint32 newval) { { on next line https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:594: if (oldval==Grunnable && newval==Gscanrunnable) { drop {} around single line if/for/while https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:638: while (!success) { no space between while( https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:641: while (isscanstatus(status)) { no space between while( https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:645: runtime·printf("casgstatus::647 status stuck in scan state gp (%p)\n", remove debug output https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1145: retry: revert https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:137: Gscanidle, // When scanning completes make it Gidle this is unneeded as Gidle is unused https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:316: revert https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:348: runtime·casgstatus(gp, Grunning, Gwaiting); if oldstatus == Gscanrunning, then the cas will fail also, you need a loop here, because the status can change from Grunning to Gscanrunning concurrently https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:350: runtime·casgstatus(gp, Gsyscall, Gwaiting); you can't set Gwaiting status here because the concurrent scanner can cas(Gwaiting, Gscanwaiting) concurrently and start scanning stack concurrently with oldstack, which will explode https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:352: runtime·printf("oldstack:642 status(%d) is not Grunning or Gsyscall\n", oldstatus); s/printf/throw/ what is 642? https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:372: // gp->status = oldstatus; remove https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:373: if (oldstatus == Gscanrunning || oldstatus == Grunning) { remove {} around one-line if/for/while https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:374: runtime·casgstatus(gp, Gwaiting, Grunning); remove trailing tab https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:876: // if not we know we came from an overflow. what is the difference between 'stack guard' and 'overflow'? newstack is always called by the stack overflow prologue check https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: while (! runtime·casgstatus(gp, oldstatus, Gwaiting)) { I don't understand what we are waiting for here. This goroutine runs, so it can'be concurrently scanned. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:994: runtime·printf("proc.c:1021:newstack gp(%p)->status(%d)not Gwaiting, oldstatus(%d)\n", remove https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1012: runtime·printf("proc.c:1037:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", remove https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1041: runtime·printf("proc.c:1069:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", gp, runtime·readgstatus(gp), oldstatus); remove https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1122: runtime·printf("proc.c:1037:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", gp, runtime·readgstatus(gp), oldstatus); remove
Sign in to reply to this message.
I would not rename status to atomicstatus. It would be a mistake to prefix all vars that ever participate in an atomic op with "atomic". We have no single precedent today.
Sign in to reply to this message.
I suggested the rename, and I think it is important. It is not import to put atomic into the name. What is important is renaming the field so that if 'hg sync' merges this CL with some other changes, there will not be new non-atomic references to the field that are introduced and silently merged into the tree. Those would be bugs. Originally we used _status but atomicstatus seems a little clearer. It doesn't matter what it is, it just can't be 'status'.
Sign in to reply to this message.
https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.c File src/pkg/runtime/heapdump.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.... src/pkg/runtime/heapdump.c:459: runtime·printf("unexpected G.status %d\n", runtime·readgstatus(gp)); There's no clear guarantee that the status printed here will match the unexpected status read earlier. Use a local variable instead? Similar comments apply to several places in this CL.
Sign in to reply to this message.
lots of comments but overall looks good. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:761: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", The (x) for showing a value is unusual in the runtime prints. We typically use =x or just ' x'. Also it's a bit strange to have the leading space here. runtime.printf("gp=%p goid=%D status=%d\n", gp, gp->goid, runtime.readgstatus(gp)); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:1367: // Ok, we're doing it! Stop everybody else you deleted a blank line above this one. please put it back. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:339: gpstatus = runtime·readgstatus(gp); move up to top and switch(gpstatus) https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:340: if((gpstatus == Gwaiting || gpstatus == Gscanwaiting || gpstatus == Gsyscall || gpstatus == Gscansyscall) && gp->waitsince != 0) if(((gpstatus&~Gscan) == Gwaiting || (gpstatus&~Gscan) == Gsyscall) && gp->waitsince != 0) https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:354: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); make this runtime.dumpgstatus and then delete this copy in favor of the one in the other file. or maybe it just doesn't need to be here at all, because this information is usually more compactly printed on the same line as a more complete error. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:433: if( (status != Gwaiting) && (status != Gscanwaiting) ) { global search for "if (" and "for (" and "while (" and "( " and " )" and drop the spaces in all of those. also drop the inner ( ) here - they are unnecessary https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:544: if (status == Gscanidle) return (status&Gscan) != 0; or maybe drop entirely. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:547: return (status == Gscanrunnable || status == Gscanenqueue || drop ( ) - unnecessary https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:568: if (oldval==Gscanrunnable && newval==Grunnable) { Can avoid much of the duplicated code by using a switch. The switch also makes clear that Gscanenqueue is different. switch(oldval) { case Gscanrunnable: case Gscanwaiting: case Gscanrunning: case Gscansyscall: if(newval == (oldval&~Gscan)) success = runtime.cas(&gp->atomicstatus, oldval, newval); break; case Gscanenqueue: if(newval == Gwaiting) success = runtime.cas(&gp->atomicstatus, oldval, newval); break; } https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:581: runtime·printf("casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", runtime.printf("runtime: casfromgscanstatus gp=%p goid=%D status=%d old=%d new=%d\n", gp, gp->goid, runtime.readgstatus(gp), oldval, newval); runtime.throw("casfromgscanstatus"); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:593: // Check new value matches old values sans scan. switch(oldval) { case Grunnable: case Gwaiting: case Gsyscall: if(newval == (oldval|Gscan)) return runtime.cas(&gp->atomicstatus, oldval, newval); break; case Grunning: if(newval == Gscanrunning || newval == Gscanenqueue) return runtime.cas(&gp->atomicstatus, oldval, newval); break; } runtime.printf("runtime: castogscanstatus old=%d new=%d\n", old, new); runtime.throw("castogscanstatus"); return false; // not reached https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:630: if (isscanstatus(oldval)) I think it is a mistake to overload false this way. I don't see any callers that really make use of this, and they can always do the loop waiting for oldval themselves if they need it. Let's require that callers pass non-scan status to casgstatus and throw if they don't. if((oldval&Gscan) || (newval&Gscan) || oldval == newval) { runtime.printf("runtime: casgstatus old=%d new=%d\n", oldval, newval); runtime.throw("casgstatus"); } https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:638: while (!success) { Since success is always true, it is unnecessary. for(;;) { status = runtime.readgstatus(gp); while(status&Gscan) status = runtime.readgstatus(gp); if(runtime.cas(&gp->atomicstatus, oldval, newval)) return true; } Also, I am a little confused: should we also be checking that, once status is not a scan status, status == oldval? https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:652: // Allows Gidle to to go Gsyscall during newextram. I remember talking to you about this comment but I don't understand what it is describing in this code. That is, this is true but why is it HERE? https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:659: // of GC all the routines can be reliable stopped. This is not always the case reliably https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1214: if(runtime·atomicload(&runtime·sched.nmspinning) + revert changes here https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1275: if((status != Grunnable) && (status != Gscanrunnable)) { delete inner ( ) - unnecessary https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1332: runtime·casgstatus(gp, Grunnable, Grunning); // If gp->status is scanrunnable casgstatus will wait for scan to complete. I think the reader should know what casgstatus does. Delete this comment from all calls in this file. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1581: uint32 status; blank line after this one. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1650: if ((status != Grunning) && (status != Gscanrunning)) { if((status&~Gscan) != Grunning) actually isn't this whole block unnecessary? casgstatus will do this check. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1671: uint32 status; This looks like debugging code, and goexit0 will do it too. So delete changes here. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1688: status = runtime·readgstatus(gp); I think you can delete all of this. runtime.casgstatus(gp, Grunning, Gdead); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1864: // We need to cas the status and scan before resuming... doesn't casgstatus do the loop for you? runtime.casgstatus(g, Gsyscall, Grunning); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1944: // TODO write test to force this code to be executed This code is executed all the time. The code that was problematic was oldstatus not being restored in oldstack. The comment belongs there if anywhere, but I'd just delete it. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2151: runtime·casgstatus(newg, Gidle, Gdead); Why does the GC scanner need this? The code below makes it look like newg comes back from gfget with status set to Gidle. Why is it okay in that case but not this one? https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2171: status = runtime·readgstatus(newg); use Suggested Pattern (see next file) or if we can resolve the previous comment, do: runtime.casgstatus(newg, Gidle, Grunnable); and then the debugging prints can go away. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2589: (uintptr)sp < gp->stackguard - StackGuard || gp->stackbase < (uintptr)sp || revert spacing changes. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2600: if(mp->ncgo > 0 && mp->curg != nil && mp->curg->syscallpc != 0 && mp->curg->syscallsp != 0) { revert spacing changes https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2839: if(s == Gwaiting || s == Gscanwaiting) switch(s&~Gscan) { case Gwaiting: grunning++; break; case Grunnable: case Grunning: case Gsyscall: runtime.unlock... } https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2887: (runtime·sched.gcwaiting || runtime·atomicload(&runtime·sched.npidle) == runtime·gomaxprocs)) { // TODO: fast atomic revert spacing changes https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2988: if(p->runqhead == p->runqtail && runtime·atomicload(&runtime·sched.nmspinning) + runtime·atomicload(&runtime·sched.npidle) > 0 && pd->syscallwhen + 10*1000*1000 > now) revert spacing changes. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:3191: if((mp = runtime·sched.midle) != nil){ reinsert blank line above this one https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go File src/pkg/runtime/proc.go (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:9: // "sync/atomic" delete https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:41: //return atomic.LoadUint32(&gp.atomicstatus) // TODO: add bootstrap code to provide. I will make this work better today. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:136: // the following encode that the GC is scanning the stack and what to do when it is done I suggest Gdead, Gcopystack, Genqueue, // not used directly; for Gscanenqueue only Gscan = 0x1000, // GC is scanning; can mask bit to find underlying state Gscanrunnable = Gscan+Grunnable, Gscanrunning = Gscan+Grunning, Gscansyscall = Gscan+Gsyscall, Gscanwaiting = Gscan+Gwaiting, This will simplify a lot of the places that check for the two variants of a status. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:347: if (oldstatus == Gscanrunning || oldstatus == Grunning) Suggested Pattern: oldstatus = runtime.readgstatus(gp); if((oldstatus&~Gscan) != Grunning && (oldstatus&~Gscan) != Gsyscall) { runtime.printf("runtime: oldstack status=%d\n", oldstatus); runtime.throw("oldstack"); } oldstatus &= ~Gscan; runtime.casgstatus(gp, oldstatus, Gwaiting); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:348: runtime·casgstatus(gp, Grunning, Gwaiting); On 2014/08/26 09:04:33, dvyukov wrote: > if oldstatus == Gscanrunning, then the cas will fail > also, you need a loop here, because the status can change from Grunning to > Gscanrunning concurrently casgstatus takes care of waiting for the transition back from Gscanrunning to Grunning. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:350: runtime·casgstatus(gp, Gsyscall, Gwaiting); On 2014/08/26 09:04:32, dvyukov wrote: > you can't set Gwaiting status here > because the concurrent scanner can cas(Gwaiting, Gscanwaiting) concurrently and > start scanning stack concurrently with oldstack, which will explode There is no concurrent scanner right now. In the next CL we will change the state to Gcopystack around the stack flip below, and that will do the appropriate wait. Explosion averted. This is fine for now. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:372: // gp->status = oldstatus; replace entire new block with runtime.casgstatus(gp, Gwaiting, oldstatus); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:829: if (oldstatus == Gwaiting || oldstatus == Gscanwaiting) { use Suggested Pattern https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:843: if (oldstatus == Gwaiting) { runtime.casgstatus(gp, Gcopystack, oldstatus); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:875: // if g->preempt is set then we know we came from a stack guard. Delete this sentence and the next. It's not actually how we tell. We check for StackPreempt. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:877: // If it is an overflow then we need to prevent the GC from I think the comment form here down should probably be deleted. I don't understand it. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:904: // The possible states are Grunning. Gscanrunning, Gsyscall, Gscansyscall // The goroutine must be executing in order to call newstack, so the possible states are // Grunning and Gsyscall (and, due to GC, also Gscanrunning and Gscansyscall). The rest of the comment I think I would drop. I don't understand the second paragraph, and the third paragraph is incorrect. Newstack with Gsyscall is almost always BEFORE the system call, which can block for arbitrarily long. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: while (! runtime·casgstatus(gp, oldstatus, Gwaiting)) { use Suggested Pattern https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:991: //gp->status = oldstatus; replace the next 15 lines with runtime.casgstatus(gp, Gwaiting, oldstatus); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1009: // gp->status = oldstatus; replace next 5 lines with runtime.casgstatus(gp, Gwaiting, oldstatus); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1018: // A stack can be copied and in the transition between the two stack This comment is true but it's a strange place to put it. I would delete it from here. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1032: copystack(gp, nframes, newsize); here might be a good place. // Note that the concurrent GC might be scanning the stack as we try to replace it. // copystack takes care of the appropriate coordination with the stack scanner. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1039: if (oldstatus == Gsyscall || oldstatus == Grunning) { again, replace whole block with runtime.casgstatus(gp, Gwaiting, oldstatus); https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1120: if (oldstatus == Gsyscall || oldstatus == Grunning) { again
Sign in to reply to this message.
PTAL I'm leaving the torture test to run ove night but it has run 5 times so there aren't any obvious bugs. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.c File src/pkg/runtime/heapdump.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/heapdump.... src/pkg/runtime/heapdump.c:459: runtime·printf("unexpected G.status %d\n", runtime·readgstatus(gp)); On 2014/08/26 14:55:03, josharian wrote: > There's no clear guarantee that the status printed here will match the > unexpected status read earlier. Use a local variable instead? > > Similar comments apply to several places in this CL. Acknowledged and changed. That said we are in a STW at this point so any change in the status indicates a broken system. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:761: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", On 2014/08/26 15:27:11, rsc wrote: > The (x) for showing a value is unusual in the runtime prints. We typically use > =x or just ' x'. > Also it's a bit strange to have the leading space here. > > runtime.printf("gp=%p goid=%D status=%d\n", gp, gp->goid, > runtime.readgstatus(gp)); Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:1367: // Ok, we're doing it! Stop everybody else On 2014/08/26 15:27:11, rsc wrote: > you deleted a blank line above this one. please put it back. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:339: gpstatus = runtime·readgstatus(gp); On 2014/08/26 15:27:15, rsc wrote: > move up to top and switch(gpstatus) Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:340: if((gpstatus == Gwaiting || gpstatus == Gscanwaiting || gpstatus == Gsyscall || gpstatus == Gscansyscall) && gp->waitsince != 0) On 2014/08/26 15:27:13, rsc wrote: > if(((gpstatus&~Gscan) == Gwaiting || (gpstatus&~Gscan) == Gsyscall) && > gp->waitsince != 0) Never warmed to the idea of "scan" bit, it seems a bit too clever. It is a bit brittle because we have Gcopystack which doesn't have a scan version and Gscanenqueue which doesn't have non-scan version. That said these facts might be an indication of a need to revisit the use of these two values. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:354: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); On 2014/08/26 09:04:32, dvyukov wrote: > all printf's must be prefixed with "runtime: ", so that user can distinguish > what comes from runtime and what comes from his own code Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:354: runtime·printf(" gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); On 2014/08/26 15:27:11, rsc wrote: > make this runtime.dumpgstatus and then delete this copy in favor of the one in > the other file. > or maybe it just doesn't need to be here at all, because this information is > usually more compactly printed on the same line as a more complete error. I removed it from mgc0.c and left this as a static, just in this file. I want to keep it as a function so I can beef up diagnostics without having to scatter them throughout the file. In particular for debugging I want a history of gp status but I don't think such diagnostics belong in a CL. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:430: uint32 status = runtime·readgstatus(gp); On 2014/08/26 09:04:32, dvyukov wrote: > we don't use variable initializers > functions look like: > > void > foo(...) > { > var decls > empty line > code > } Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:433: if( (status != Gwaiting) && (status != Gscanwaiting) ) { On 2014/08/26 15:27:14, rsc wrote: > global search for "if (" and "for (" and "while (" and "( " and " )" and drop > the spaces in all of those. > > also drop the inner ( ) here - they are unnecessary Done but I resisted fixing lines I did not introduce. I could if you want me to. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:543: isscanstatus(uint32 status) { On 2014/08/26 09:04:32, dvyukov wrote: > { on next line Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:544: if (status == Gscanidle) On 2014/08/26 15:27:15, rsc wrote: > return (status&Gscan) != 0; > or maybe drop entirely. Let's have a meta discussion about Gscan before I make these changes. I did change runtime.h to introduce Gscan and I lined up all the enums so making the change won't be painful. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:547: return (status == Gscanrunnable || status == Gscanenqueue || On 2014/08/26 15:27:15, rsc wrote: > drop ( ) - unnecessary Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:581: runtime·printf("casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", On 2014/08/26 09:04:32, dvyukov wrote: > prefix with "runtime: " Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:581: runtime·printf("casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", On 2014/08/26 15:27:12, rsc wrote: > runtime.printf("runtime: casfromgscanstatus gp=%p goid=%D status=%d old=%d > new=%d\n", gp, gp->goid, runtime.readgstatus(gp), oldval, newval); > runtime.throw("casfromgscanstatus"); See comment above that argues for centralizing dumpgstatus to enable better diagnostics. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:584: runtime·throw("casfromgscanstatus: gp->status is not in scan state. "); On 2014/08/26 09:04:32, dvyukov wrote: > remove ". " at the end of string Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:590: runtime·castogscanstatus (G *gp, uint32 oldval, uint32 newval) { On 2014/08/26 09:04:32, dvyukov wrote: > { on next line Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:593: // Check new value matches old values sans scan. On 2014/08/26 15:27:11, rsc wrote: > switch(oldval) { > case Grunnable: > case Gwaiting: > case Gsyscall: > if(newval == (oldval|Gscan)) > return runtime.cas(&gp->atomicstatus, oldval, newval); > break; > case Grunning: > if(newval == Gscanrunning || newval == Gscanenqueue) > return runtime.cas(&gp->atomicstatus, oldval, newval); > break; > } > > runtime.printf("runtime: castogscanstatus old=%d new=%d\n", old, new); > runtime.throw("castogscanstatus"); > return false; // not reached > Acknowledged. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:594: if (oldval==Grunnable && newval==Gscanrunnable) { On 2014/08/26 09:04:31, dvyukov wrote: > drop {} around single line if/for/while Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:638: while (!success) { On 2014/08/26 09:04:32, dvyukov wrote: > no space between while( Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:641: while (isscanstatus(status)) { On 2014/08/26 09:04:32, dvyukov wrote: > no space between while( Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:645: runtime·printf("casgstatus::647 status stuck in scan state gp (%p)\n", On 2014/08/26 09:04:32, dvyukov wrote: > remove debug output Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:652: // Allows Gidle to to go Gsyscall during newextram. On 2014/08/26 15:27:15, rsc wrote: > I remember talking to you about this comment but I don't understand what it is > describing in this code. That is, this is true but why is it HERE? Acknowledged. The comment does not belong here it is an orphan from removed diagnostics. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:659: // of GC all the routines can be reliable stopped. This is not always the case On 2014/08/26 15:27:11, rsc wrote: > reliably Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1145: retry: On 2014/08/26 09:04:32, dvyukov wrote: > revert Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1214: if(runtime·atomicload(&runtime·sched.nmspinning) + On 2014/08/26 15:27:14, rsc wrote: > revert changes here Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1275: if((status != Grunnable) && (status != Gscanrunnable)) { On 2014/08/26 15:27:14, rsc wrote: > delete inner ( ) - unnecessary Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1332: runtime·casgstatus(gp, Grunnable, Grunning); // If gp->status is scanrunnable casgstatus will wait for scan to complete. On 2014/08/26 15:27:13, rsc wrote: > I think the reader should know what casgstatus does. Delete this comment from > all calls in this file. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1581: uint32 status; On 2014/08/26 15:27:13, rsc wrote: > blank line after this one. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1650: if ((status != Grunning) && (status != Gscanrunning)) { On 2014/08/26 15:27:11, rsc wrote: > if((status&~Gscan) != Grunning) > actually isn't this whole block unnecessary? casgstatus will do this check. casgstatus does not do the check that this is Grunning, perhaps you are thinking about castogscanstatus. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1688: status = runtime·readgstatus(gp); On 2014/08/26 15:27:13, rsc wrote: > I think you can delete all of this. > > runtime.casgstatus(gp, Grunning, Gdead); Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1864: // We need to cas the status and scan before resuming... On 2014/08/26 15:27:13, rsc wrote: > doesn't casgstatus do the loop for you? > > runtime.casgstatus(g, Gsyscall, Grunning); Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1944: // TODO write test to force this code to be executed On 2014/08/26 15:27:13, rsc wrote: > This code is executed all the time. The code that was problematic was oldstatus > not being restored in oldstack. The comment belongs there if anywhere, but I'd > just delete it. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2151: runtime·casgstatus(newg, Gidle, Gdead); On 2014/08/26 15:27:11, rsc wrote: > Why does the GC scanner need this? The code below makes it look like newg comes > back from gfget with status set to Gidle. Why is it okay in that case but not > this one? The code below is to conservative (ie wrong). gfget always returns Gdead. There is a check in gfput that the status is Gdead before it is made available to gfget. Likewise allgadd checks to be sure it is Gdead. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2589: (uintptr)sp < gp->stackguard - StackGuard || gp->stackbase < (uintptr)sp || On 2014/08/26 15:27:14, rsc wrote: > revert spacing changes. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2600: if(mp->ncgo > 0 && mp->curg != nil && mp->curg->syscallpc != 0 && mp->curg->syscallsp != 0) { On 2014/08/26 15:27:13, rsc wrote: > revert spacing changes Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2887: (runtime·sched.gcwaiting || runtime·atomicload(&runtime·sched.npidle) == runtime·gomaxprocs)) { // TODO: fast atomic On 2014/08/26 15:27:14, rsc wrote: > revert spacing changes Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2988: if(p->runqhead == p->runqtail && runtime·atomicload(&runtime·sched.nmspinning) + runtime·atomicload(&runtime·sched.npidle) > 0 && pd->syscallwhen + 10*1000*1000 > now) On 2014/08/26 15:27:14, rsc wrote: > revert spacing changes. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:3191: if((mp = runtime·sched.midle) != nil){ On 2014/08/26 15:27:11, rsc wrote: > reinsert blank line above this one Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go File src/pkg/runtime/proc.go (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:9: // "sync/atomic" On 2014/08/26 15:27:15, rsc wrote: > delete Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:41: //return atomic.LoadUint32(&gp.atomicstatus) // TODO: add bootstrap code to provide. On 2014/08/26 15:27:15, rsc wrote: > I will make this work better today. Acknowledged. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:137: Gscanidle, // When scanning completes make it Gidle On 2014/08/26 09:04:32, dvyukov wrote: > this is unneeded as Gidle is unused Turns out that Gidle is used. It is 0 and a newly allocated g come back in a Gidle state. That said Gscanidle is not used so I'll comment it out. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:316: On 2014/08/26 09:04:33, dvyukov wrote: > revert Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:347: if (oldstatus == Gscanrunning || oldstatus == Grunning) On 2014/08/26 15:27:15, rsc wrote: > Suggested Pattern: > > oldstatus = runtime.readgstatus(gp); > if((oldstatus&~Gscan) != Grunning && (oldstatus&~Gscan) != Gsyscall) { > runtime.printf("runtime: oldstack status=%d\n", oldstatus); > runtime.throw("oldstack"); > } > oldstatus &= ~Gscan; > runtime.casgstatus(gp, oldstatus, Gwaiting); > Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:348: runtime·casgstatus(gp, Grunning, Gwaiting); On 2014/08/26 09:04:33, dvyukov wrote: > if oldstatus == Gscanrunning, then the cas will fail > also, you need a loop here, because the status can change from Grunning to > Gscanrunning concurrently The loop is in casgstatus. The only way casgstatus will return false is if it is passed a Gscanmumble as an oldvalue. proc.c:639: while(!runtime·cas(&gp->atomicstatus, oldval, newval)){ https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:350: runtime·casgstatus(gp, Gsyscall, Gwaiting); On 2014/08/26 09:04:32, dvyukov wrote: > you can't set Gwaiting status here > because the concurrent scanner can cas(Gwaiting, Gscanwaiting) concurrently and > start scanning stack concurrently with oldstack, which will explode Yes this has to go to Gcopystack and not Gwaiting as is done in newstack. Changed. In anycase, oldstack is on its way out so no harm no foul. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:352: runtime·printf("oldstack:642 status(%d) is not Grunning or Gsyscall\n", oldstatus); On 2014/08/26 09:04:32, dvyukov wrote: > s/printf/throw/ > what is 642? At one point it was a line number so greping for oldstack:642 would uniquely match up this line with the error message. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:372: // gp->status = oldstatus; On 2014/08/26 15:27:16, rsc wrote: > replace entire new block with > > runtime.casgstatus(gp, Gwaiting, oldstatus); Yes but it is not Gcopystack... https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:373: if (oldstatus == Gscanrunning || oldstatus == Grunning) { On 2014/08/26 09:04:32, dvyukov wrote: > remove {} around one-line if/for/while Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:374: runtime·casgstatus(gp, Gwaiting, Grunning); On 2014/08/26 09:04:32, dvyukov wrote: > remove trailing tab Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:829: if (oldstatus == Gwaiting || oldstatus == Gscanwaiting) { On 2014/08/26 15:27:16, rsc wrote: > use Suggested Pattern Acknowledged. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:875: // if g->preempt is set then we know we came from a stack guard. On 2014/08/26 15:27:15, rsc wrote: > Delete this sentence and the next. > It's not actually how we tell. We check for StackPreempt. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:876: // if not we know we came from an overflow. On 2014/08/26 09:04:32, dvyukov wrote: > what is the difference between 'stack guard' and 'overflow'? > newstack is always called by the stack overflow prologue check Comment removed. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:877: // If it is an overflow then we need to prevent the GC from On 2014/08/26 15:27:15, rsc wrote: > I think the comment form here down should probably be deleted. I don't > understand it. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:904: // The possible states are Grunning. Gscanrunning, Gsyscall, Gscansyscall On 2014/08/26 15:27:16, rsc wrote: > // The goroutine must be executing in order to call newstack, so the possible > states are > // Grunning and Gsyscall (and, due to GC, also Gscanrunning and Gscansyscall). > > The rest of the comment I think I would drop. I don't understand the second > paragraph, and the third paragraph is incorrect. Newstack with Gsyscall is > almost always BEFORE the system call, which can block for arbitrarily long. > I deleted this as suggested. It appears to be comments from when I was first (mis)understanding what was going on. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: while (! runtime·casgstatus(gp, oldstatus, Gwaiting)) { On 2014/08/26 09:04:32, dvyukov wrote: > I don't understand what we are waiting for here. This goroutine runs, so it > can'be concurrently scanned. At some level this was a blind change based on the original assignment to gp->status. That said if it is in Gsyscall then it could be moved to Gscansyscall by the GC and scanned. This casgstatus will chase it out of that state and into Gsyscall and then into Gwaiting. The loop also ensures that oldstatus will not be Gscansyscal of Gscanrunning. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: while (! runtime·casgstatus(gp, oldstatus, Gwaiting)) { On 2014/08/26 15:27:16, rsc wrote: > use Suggested Pattern Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:991: //gp->status = oldstatus; On 2014/08/26 15:27:15, rsc wrote: > replace the next 15 lines with > > runtime.casgstatus(gp, Gwaiting, oldstatus); Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:994: runtime·printf("proc.c:1021:newstack gp(%p)->status(%d)not Gwaiting, oldstatus(%d)\n", On 2014/08/26 09:04:33, dvyukov wrote: > remove Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1009: // gp->status = oldstatus; On 2014/08/26 15:27:16, rsc wrote: > replace next 5 lines with > > runtime.casgstatus(gp, Gwaiting, oldstatus); Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1012: runtime·printf("proc.c:1037:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", On 2014/08/26 09:04:32, dvyukov wrote: > remove Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1018: // A stack can be copied and in the transition between the two stack On 2014/08/26 15:27:16, rsc wrote: > This comment is true but it's a strange place to put it. > I would delete it from here. Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1032: copystack(gp, nframes, newsize); On 2014/08/26 15:27:16, rsc wrote: > here might be a good place. > > // Note that the concurrent GC might be scanning the stack as we try to replace > it. > // copystack takes care of the appropriate coordination with the stack scanner. > Better wording. I will use it instead of the comment from above. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1039: if (oldstatus == Gsyscall || oldstatus == Grunning) { On 2014/08/26 15:27:16, rsc wrote: > again, replace whole block with > > runtime.casgstatus(gp, Gwaiting, oldstatus); > Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1041: runtime·printf("proc.c:1069:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", gp, runtime·readgstatus(gp), oldstatus); On 2014/08/26 09:04:32, dvyukov wrote: > remove Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1120: if (oldstatus == Gsyscall || oldstatus == Grunning) { On 2014/08/26 15:27:15, rsc wrote: > again Done. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:1122: runtime·printf("proc.c:1037:newstack gp(%p)->status(%d) oldstatus(%d)not Gsyscall or Grunning\n", gp, runtime·readgstatus(gp), oldstatus); On 2014/08/26 09:04:33, dvyukov wrote: > remove Done.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
Sign in to reply to this message.
LGTM fix comments as you see fit and submit. https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/40001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:652: // Allows Gidle to to go Gsyscall during newextram. On 2014/08/26 23:49:11, rlh wrote: > On 2014/08/26 15:27:15, rsc wrote: > > I remember talking to you about this comment but I don't understand what it is > > describing in this code. That is, this is true but why is it HERE? > > Acknowledged. > The comment does not belong here it is an orphan from removed diagnostics. Should it be removed? https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:744: runtime·printf("runtime: gp=%p, goid=%D, gp->status=%d\n", gp, gp->goid, runtime·readgstatus(gp)); change two space after second comma to one. same on next print. (actually it's more common to not have commas at all, but definitely remove the doubled space.) https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:355: runtime·printf("runtime: gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); use = instead of (), remove doubled space https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:548: if (status == Gscan) "if(" https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:550: return ((status&Gscan) == Gscan); remove outer ( ) https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:585: runtime·printf("runtime: casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", replace () with = https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:629: if(isscanstatus(oldval) || isscanstatus(newval) || (oldval == newval)) { remove () around last clause https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1567: status = runtime·readgstatus(g); Why is this loop necessary? Doesn't park_m end up doing this loop inside casgstatus anyway? https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2791: default: no need for 'default: break;' in switches fwiw if you do need a default, we conventionally place them at the top of the switch as the standard place to look for them. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:878: while ((oldstatus&Gscan) == Gscan) I don't understand why this is needed. oldstatus = runtime.readstatus(gp) & ~Gscan; seems like it should be enough. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... src/pkg/runtime/traceback_arm.c:332: if(status == Gsyscall || status == Gscansyscall) { if((status&~Gscan) == Gsyscall) https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... File src/pkg/runtime/traceback_x86.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... src/pkg/runtime/traceback_x86.c:407: if(status == Gsyscall || status == Gscansyscall){ same
Sign in to reply to this message.
please wait for me as well, I would like to understand how it will work https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:291: uint32 atomicstatus; remove trailing tab
Sign in to reply to this message.
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:627: runtime·casgstatus (G *gp, uint32 oldval, uint32 newval) and I thought why I can't find this function in the previous patchset remove space in "casgstatus (" https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:300: bool preemptscan; // preempted g does scan for GC Why do you need this if you have Gscanrunning? I expect that GC will set status to Gscanrunning, and then on first preemption the goroutine will scan own stack. So Gscanrunning will serve as "scan your stack" signal. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:699: void runtime·casgstatus (G*, uint32, uint32); remove space in "casgstatus ("
Sign in to reply to this message.
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:566: runtime·casfromgscanstatus (G *gp, uint32 oldval, uint32 newval) remove space before ( https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:595: runtime·castogscanstatus (G *gp, uint32 oldval, uint32 newval) remove space before ( https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:616: // casgstatus will return false if the oldvalue is a Gscan status so if you this part of the comment looks incorrect https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:634: while(!runtime·cas(&gp->atomicstatus, oldval, newval)){ add space between "){" https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:638: // Allows Gidle to to go Gsyscall during newextram. s/to to/to/ https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1568: while(status == Gscanrunning){ gopark does not have this loop, one of them is incorrect https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.go File src/pkg/runtime/proc.go (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:39: return gp.atomicstatus we have goatomicload function https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:848: // If the GC is trying to stop this g then it will set preemptscan to true. this function does not look at preemptscan, so why is it needed? why Gscanrunning/Gscansyscall is not enough? https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:878: while ((oldstatus&Gscan) == Gscan) why is this needed? GC can't scan stack of a running goroutine, so it will never reset Gscan https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:889: runtime·casgstatus(gp, oldstatus, Gwaiting); // oldstatus is not in a Gscan status oldstack and copystack set status to Gcopystack, newstack sets status to Gwaiting I don't understand the system Isn't one of oldstack/newstack wrong? https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:925: // If we notice that this is preemption and the status is Grunning do the scan... the code below does not do the scan and generally it must not scan, because generally GC is not running the comment looks confusing https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: remove this empty line the above comment is not general, it's about this particular 'if' https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:960: // Note that the concurrent GC might be scanning the stack as we try to replace it. Why is it possible? How GC can scan stack of a running goroutine? How can it make sense of stack of a concurrently mutated stack?..
Sign in to reply to this message.
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:605: if(newval == Gscanrunning || newval == Gscanenqueue) when the caller will pass Gscanrunning and when Gscanenqueue?
Sign in to reply to this message.
How are running goroutines scanned? What are transitions between Grunning, Gscanrunning and Gcopystack?
Sign in to reply to this message.
Dmitry, the concurrent scan is not in this CL. Please lgtm this and then we can start review on the next one which is more interesting. Thanks. On Wednesday, August 27, 2014, <dvyukov@google.com> wrote: > How are running goroutines scanned? What are transitions between > Grunning, Gscanrunning and Gcopystack? > > https://codereview.appspot.com/132960044/ >
Sign in to reply to this message.
The active spin waits while scanning look like the wrong direction to me. Goroutines must not wait, they must act according to current value of status. E.g. when a goroutine G1 wants to change status of goroutine G2 from Gwaiting to Grunnable (when e.g. waking a goroutine on chan); if it discovers that G2 is in Gscanwaiting, it needs to CAS it to Gscanenqueue and return. When scanning finishes, the scanner sees that the status is Gscanenqueue, changes it to Grunnable and queues G2 into runqueue. No waiting. The same applies to all other transitions. For example, when a goroutine returns from syscall but discovers that its own status is Gscansyscall, it needs to CAS it to Gscanenqueue and park the thread. The waiting unnecessary burns cycles and can lead to significant latency increase when GOMAXPROCS=2, because you think that the other proc is running user code, but it's actually just waiting for the first proc. Nobody is running user code. In significant portion of cases atomic CAS is not needed and produces false sense that statuses are always chaotically altered by a plurality of threads. This is not true. For example, Gscanenqueue is a "final" status, only scanner can update status from Gscanenqueue. The same applied to Grunning, since scanner can do nothing useful with a Grunning goroutine it does not need to alter its status; when the goroutine will finally call into scheduler, it scans its stack and non-atomically changes status to Grunnable/Gwaiting. Also CAS is not required if GC is not running, similar to write barriers.
Sign in to reply to this message.
On 2014/08/27 14:07:52, rsc wrote: > Dmitry, the concurrent scan is not in this CL. Please lgtm this and then we > can start review on the next one which is more interesting. Thanks. This casgstatus looks like the wrong way to approach it. See the previous longer reply. The design doc does not describe enough details to understand where it is moving. And I afraid it will move too far when I will understand the whole picture. > On Wednesday, August 27, 2014, <mailto:dvyukov@google.com> wrote: > > > How are running goroutines scanned? What are transitions between > > Grunning, Gscanrunning and Gcopystack? > > > > https://codereview.appspot.com/132960044/ > >
Sign in to reply to this message.
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:602: return runtime·cas(&gp->atomicstatus, oldval, newval); A goroutine is Gsyscall status still runs and can grow/shrink and mutate own stack. Are you sure scanning of such stack is safe? Why?
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dvyukov@google.com, josharian@gmail.com, rsc@golang.org (cc: golang-codereviews@googlegroups.com, khr@golang.org), Please take another look.
Sign in to reply to this message.
LGTM Dmitriy, we can continue this discussion in the next CL or offline. It's a good discussion to have. But we also need to make progress here. Rick, please submit.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=9fafd63bc3c5 *** runtime: changes to g->atomicstatus (nee status) to support concurrent GC Every change to g->atomicstatus is now done atomically so that we can ensure that all gs pass through a gc safepoint on demand. This allows the GC to move from one phase to the next safely. In some phases the stack will be scanned. This CL only deals with the infrastructure that allows g->atomicstatus to go from one state to another. Future CLs will deal with scanning and monitoring what phase the GC is in. The major change was to moving to using a Gscan bit to indicate that the status is in a scan state. The only bug fix was in oldstack where I wasn't moving to a Gcopystack state in order to block scanning until the new stack was in place. The proc.go file is waiting for an atomic load instruction. LGTM=rsc R=golang-codereviews, dvyukov, josharian, rsc CC=golang-codereviews, khr https://codereview.appspot.com/132960044 https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/mgc0.c File src/pkg/runtime/mgc0.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/mgc0.c#ne... src/pkg/runtime/mgc0.c:744: runtime·printf("runtime: gp=%p, goid=%D, gp->status=%d\n", gp, gp->goid, runtime·readgstatus(gp)); On 2014/08/27 11:43:50, rsc wrote: > change two space after second comma to one. > same on next print. > (actually it's more common to not have commas at all, but definitely remove the > doubled space.) Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:355: runtime·printf("runtime: gp (%p), goid %D, gp->status(%d) \n", gp, gp->goid, runtime·readgstatus(gp)); On 2014/08/27 11:43:50, rsc wrote: > use = instead of (), remove doubled space Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:548: if (status == Gscan) On 2014/08/27 11:43:50, rsc wrote: > "if(" Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:550: return ((status&Gscan) == Gscan); On 2014/08/27 11:43:50, rsc wrote: > remove outer ( ) Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:566: runtime·casfromgscanstatus (G *gp, uint32 oldval, uint32 newval) On 2014/08/27 13:16:02, dvyukov wrote: > remove space before ( Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:585: runtime·printf("runtime: casfromgscanstatus failed gp(%p), oldval(%d), newval(%d) \n", On 2014/08/27 11:43:50, rsc wrote: > replace () with = Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:595: runtime·castogscanstatus (G *gp, uint32 oldval, uint32 newval) On 2014/08/27 13:16:02, dvyukov wrote: > remove space before ( Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:605: if(newval == Gscanrunning || newval == Gscanenqueue) On 2014/08/27 13:37:18, dvyukov wrote: > when the caller will pass Gscanrunning and when Gscanenqueue? Gscanrunning when trying to set up stack trap, Gscanenqueue when the go routine should be put on the runq when the scan is complete. More detail in the next CL once this one is submitted. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:616: // casgstatus will return false if the oldvalue is a Gscan status so if you On 2014/08/27 13:16:02, dvyukov wrote: > this part of the comment looks incorrect Good catch, the comment is just wrong... https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:627: runtime·casgstatus (G *gp, uint32 oldval, uint32 newval) On 2014/08/27 12:11:12, dvyukov wrote: > and I thought why I can't find this function in the previous patchset > remove space in "casgstatus (" Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:629: if(isscanstatus(oldval) || isscanstatus(newval) || (oldval == newval)) { On 2014/08/27 11:43:50, rsc wrote: > remove () around last clause Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:634: while(!runtime·cas(&gp->atomicstatus, oldval, newval)){ On 2014/08/27 13:16:02, dvyukov wrote: > add space between "){" What is the rule??? When is there a space and when isn't there? AN why a space after a close paren but not before an open paren? https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:638: // Allows Gidle to to go Gsyscall during newextram. On 2014/08/27 13:16:03, dvyukov wrote: > s/to to/to/ Comment removed. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1567: status = runtime·readgstatus(g); On 2014/08/27 11:43:50, rsc wrote: > Why is this loop necessary? Doesn't park_m end up doing this loop inside > casgstatus anyway? I was preserving the original check and throw. Moving the check to the casgstatus is probably the right thing to do. Deleting this which makes it a lot simpler. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:1568: while(status == Gscanrunning){ On 2014/08/27 13:16:02, dvyukov wrote: > gopark does not have this loop, one of them is incorrect Loop removed. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:2791: default: On 2014/08/27 11:43:50, rsc wrote: > no need for 'default: break;' in switches > fwiw if you do need a default, we conventionally place them at the top of the > switch as the standard place to look for them. I understand and agree but a switch without a default feels venial ... https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.go File src/pkg/runtime/proc.go (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.go#n... src/pkg/runtime/proc.go:39: return gp.atomicstatus On 2014/08/27 13:16:03, dvyukov wrote: > we have goatomicload function There are (or were as of yesterday) some bootstrap issues. It is being worked on. I'll change it over in the next CL. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h File src/pkg/runtime/runtime.h (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:291: uint32 atomicstatus; On 2014/08/27 11:54:01, dvyukov wrote: > remove trailing tab Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:300: bool preemptscan; // preempted g does scan for GC On 2014/08/27 12:11:12, dvyukov wrote: > Why do you need this if you have Gscanrunning? I expect that GC will set status > to Gscanrunning, and then on first preemption the goroutine will scan own stack. > So Gscanrunning will serve as "scan your stack" signal. All of this will be clear with the next CL. Briefly Gscanrunning is used to set up the stack trap. Once in the stack trap the g observe the preemptscan and will move to Gscanwaiting and do the stack scan. All will be revealed with the next cl. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/runtime.h... src/pkg/runtime/runtime.h:699: void runtime·casgstatus (G*, uint32, uint32); On 2014/08/27 12:11:12, dvyukov wrote: > remove space in "casgstatus (" Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:848: // If the GC is trying to stop this g then it will set preemptscan to true. On 2014/08/27 13:16:03, dvyukov wrote: > this function does not look at preemptscan, so why is it needed? why > Gscanrunning/Gscansyscall is not enough? Gscanrunning is used to protect the code that sets up the trap. The scanning is done in the actual trap by the g not by the GC. The next CL (later today hopefully) will have more details. That said I do understand that calling it Gscanrunning when I really mean Gscanrunningtrap is less than helpful. I'll consider changing the name in the next CL. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:878: while ((oldstatus&Gscan) == Gscan) On 2014/08/27 11:43:50, rsc wrote: > I don't understand why this is needed. > > oldstatus = runtime.readstatus(gp) & ~Gscan; > seems like it should be enough. Agreed, changed. Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:878: while ((oldstatus&Gscan) == Gscan) On 2014/08/27 13:16:03, dvyukov wrote: > why is this needed? > GC can't scan stack of a running goroutine, so it will never reset Gscan The GC can be setting up a stack trap if it is Gscanrunning or actually doing the scan if it is Gscansyscall. The casgstatus below will block this routine until the GC is done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:889: runtime·casgstatus(gp, oldstatus, Gwaiting); // oldstatus is not in a Gscan status On 2014/08/27 13:16:03, dvyukov wrote: > oldstack and copystack set status to Gcopystack, newstack sets status to > Gwaiting > I don't understand the system > Isn't one of oldstack/newstack wrong? Possibly but this is really an issue for the next CL. Hang on a little bit longer.... https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:925: // If we notice that this is preemption and the status is Grunning do the scan... On 2014/08/27 13:16:03, dvyukov wrote: > the code below does not do the scan > and generally it must not scan, because generally GC is not running > the comment looks confusing Comment deleted. The code to do the scan will be in the next CL. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:936: On 2014/08/27 13:16:03, dvyukov wrote: > remove this empty line > the above comment is not general, it's about this particular 'if' Done. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/stack.c#n... src/pkg/runtime/stack.c:960: // Note that the concurrent GC might be scanning the stack as we try to replace it. On 2014/08/27 13:16:03, dvyukov wrote: > Why is it possible? How GC can scan stack of a running goroutine? How can it > make sense of stack of a concurrently mutated stack?.. It doesn't, the scan will be done by the g in this routine. The trap is set by the GC, the scan is done by the Grunning g. It is done by the GC if it can move the Gsyscall to Gscansyscall. The g will wait for the GC to finish before it moves from Gscansyscall to Grunning. https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... File src/pkg/runtime/traceback_arm.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/traceback... src/pkg/runtime/traceback_arm.c:332: if(status == Gsyscall || status == Gscansyscall) { On 2014/08/27 11:43:50, rsc wrote: > if((status&~Gscan) == Gsyscall) Done.
Sign in to reply to this message.
Message was sent while issue was closed.
https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/132960044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:634: while(!runtime·cas(&gp->atomicstatus, oldval, newval)){ On 2014/08/27 15:15:52, rlh wrote: > On 2014/08/27 13:16:02, dvyukov wrote: > > add space between "){" > > What is the rule??? When is there a space and when isn't there? AN why a space > after a close paren but not before an open paren? The space is before the {, not after the ). Soon we will do this all in Go and gofmt can fix these and we can stop talking about them. Until then, I am just very sorry.
Sign in to reply to this message.
Message was sent while issue was closed.
This CL appears to have broken the windows-amd64 builder. See http://build.golang.org/log/595ba451c599fa972e952bcc77a67ed1d16596b9
Sign in to reply to this message.
|