runtime: refactor and fix stack management code

There are at least 3 bugs:
1. g->stacksize accounting is broken during copystack/shrinkstack
2. stktop->free is not properly maintained during copystack/shrinkstack
3. stktop->free logic is broken:
        we can have stktop->free==FixedStack,
        and we will free it into stack cache,
        but it actually comes from heap as the result of non-copying segment shrink
This shows as at least spurious races on race builders (maybe something else as well I don't know).

The idea behind the refactoring is to consolidate stacksize and
segment origin logic in stackalloc/stackfree.

Fixes #7490.

LGTM=rsc, khr
R=golang-codereviews, rsc, khr
CC=golang-codereviews
https://golang.org/cl/72440043

diff --git a/src/pkg/runtime/panic.c b/src/pkg/runtime/panic.c
index a580e9f31014735f366e023e2f1430687f446b0e..29bf7de27f23858b5b4a6dbba20d0407eb99082d 100644 (file)
--- a/src/pkg/runtime/panic.c
+++ b/src/pkg/runtime/panic.c
@@ -339,10 +339,7 @@ runtime·unwindstack(G *gp, byte *sp)
                gp->stackbase = top->stackbase;
                gp->stackguard = top->stackguard;
                gp->stackguard0 = gp->stackguard;
-               if(top->free != 0) {
-                       gp->stacksize -= top->free;
-                       runtime·stackfree(stk, top->free);
-               }
+               runtime·stackfree(gp, stk, top);
        }
 
        if(sp != nil && (sp < (byte*)gp->stackguard - StackGuard || (byte*)gp->stackbase < sp)) {

diff --git a/src/pkg/runtime/proc.c b/src/pkg/runtime/proc.c
index a99e56dde227c14c5c2dd58a1b198fdd6324dbca..bf55912783d5c91bc7867a32e19f364e0499b05f 100644 (file)
--- a/src/pkg/runtime/proc.c
+++ b/src/pkg/runtime/proc.c
@@ -1727,7 +1727,13 @@ syscall·runtime_AfterFork(void)
 static void
 mstackalloc(G *gp)
 {
-       gp->param = runtime·stackalloc((uintptr)gp->param);
+       G *newg;
+       uintptr size;
+
+       newg = (G*)gp->param;
+       size = newg->stacksize;
+       newg->stacksize = 0;
+       gp->param = runtime·stackalloc(newg, size);
        runtime·gogo(&gp->sched);
 }
 
@@ -1747,20 +1753,19 @@ runtime·malg(int32 stacksize)
        if(stacksize >= 0) {
                if(g == m->g0) {
                        // running on scheduler stack already.
-                       stk = runtime·stackalloc(StackSystem + stacksize);
+                       stk = runtime·stackalloc(newg, StackSystem + stacksize);
                } else {
                        // have to call stackalloc on scheduler stack.
-                       g->param = (void*)(StackSystem + stacksize);
+                       newg->stacksize = StackSystem + stacksize;
+                       g->param = newg;
                        runtime·mcall(mstackalloc);
                        stk = g->param;
                        g->param = nil;
                }
-               newg->stacksize = StackSystem + stacksize;
                newg->stack0 = (uintptr)stk;
                newg->stackguard = (uintptr)stk + StackGuard;
                newg->stackguard0 = newg->stackguard;
                newg->stackbase = (uintptr)stk + StackSystem + stacksize - sizeof(Stktop);
-               runtime·memclr((byte*)newg->stackbase, sizeof(Stktop));
        }
        return newg;
 }
@@ -1883,14 +1888,20 @@ static void
 gfput(P *p, G *gp)
 {
        uintptr stksize;
+       Stktop *top;
 
        if(gp->stackguard - StackGuard != gp->stack0)
                runtime·throw("invalid stack in gfput");
        stksize = gp->stackbase + sizeof(Stktop) - gp->stack0;
-       if(stksize != FixedStack) {
+       if(stksize != gp->stacksize) {
+               runtime·printf("runtime: bad stacksize, goroutine %D, remain=%d, last=%d\n",
+                       gp->goid, (int32)gp->stacksize, (int32)stksize);
+               runtime·throw("gfput: bad stacksize");
+       }
+       top = (Stktop*)gp->stackbase;
+       if(top->malloced) {
                // non-standard stack size - free it.
-               runtime·stackfree((void*)gp->stack0, stksize);
-               gp->stacksize = 0;
+               runtime·stackfree(gp, (void*)gp->stack0, top);
                gp->stack0 = 0;
                gp->stackguard = 0;
                gp->stackguard0 = 0;
@@ -1941,19 +1952,18 @@ retry:
                if(gp->stack0 == 0) {
                        // Stack was deallocated in gfput.  Allocate a new one.
                        if(g == m->g0) {
-                               stk = runtime·stackalloc(FixedStack);
+                               stk = runtime·stackalloc(gp, FixedStack);
                        } else {
-                               g->param = (void*)FixedStack;
+                               gp->stacksize = FixedStack;
+                               g->param = gp;
                                runtime·mcall(mstackalloc);
                                stk = g->param;
                                g->param = nil;
                        }
-                       gp->stacksize = FixedStack;
                        gp->stack0 = (uintptr)stk;
                        gp->stackbase = (uintptr)stk + FixedStack - sizeof(Stktop);
                        gp->stackguard = (uintptr)stk + StackGuard;
                        gp->stackguard0 = gp->stackguard;
-                       runtime·memclr((byte*)gp->stackbase, sizeof(Stktop));
                }
        }
        return gp;

diff --git a/src/pkg/runtime/runtime.h b/src/pkg/runtime/runtime.h
index 2db18003de289e017bb2e20f77d699609b2d2b72..4415f550d4de792c77a2c4f5af93ba5869e8d5a3 100644 (file)
--- a/src/pkg/runtime/runtime.h
+++ b/src/pkg/runtime/runtime.h
@@ -452,8 +452,8 @@ struct      Stktop
        uint32  panicwrap;
 
        uint8*  argp;   // pointer to arguments in old frame
-       uintptr free;   // if free>0, call stackfree using free as size
        bool    panic;  // is this frame the top of a panic?
+       bool    malloced;
 };
 struct SigTab
 {
@@ -880,8 +880,8 @@ int32       runtime·funcarglen(Func*, uintptr);
 int32  runtime·funcspdelta(Func*, uintptr);
 int8*  runtime·funcname(Func*);
 int32  runtime·pcdatavalue(Func*, int32, uintptr);
-void*  runtime·stackalloc(uint32);
-void   runtime·stackfree(void*, uintptr);
+void*  runtime·stackalloc(G*, uint32);
+void   runtime·stackfree(G*, void*, Stktop*);
 void   runtime·shrinkstack(G*);
 MCache*        runtime·allocmcache(void);
 void   runtime·freemcache(MCache*);

diff --git a/src/pkg/runtime/stack.c b/src/pkg/runtime/stack.c
index 4abdd7bdb5607bb1128cbdf3c32a4ec01404231e..d1ba2bfdb9bef86f0096d7567ba3ab80715819e2 100644 (file)
--- a/src/pkg/runtime/stack.c
+++ b/src/pkg/runtime/stack.c
@@ -84,10 +84,12 @@ stackcacherelease(void)
 }
 
 void*
-runtime·stackalloc(uint32 n)
+runtime·stackalloc(G *gp, uint32 n)
 {
        uint32 pos;
        void *v;
+       bool malloced;
+       Stktop *top;
 
        // Stackalloc must be called on scheduler stack, so that we
        // never try to grow the stack during the code that stackalloc runs.
@@ -99,6 +101,7 @@ runtime·stackalloc(uint32 n)
        if(StackDebug >= 1)
                runtime·printf("stackalloc %d\n", n);
 
+       gp->stacksize += n;
        if(StackFromSystem)
                return runtime·SysAlloc(ROUND(n, PageSize), &mstats.stacks_sys);
 
@@ -106,6 +109,7 @@ runtime·stackalloc(uint32 n)
        // but if we need a stack of a bigger size, we fall back on malloc
        // (assuming that inside malloc all the stack frames are small,
        // so that we do not deadlock).
+       malloced = true;
        if(n == FixedStack || m->mallocing) {
                if(n != FixedStack) {
                        runtime·printf("stackalloc: in malloc, size=%d want %d\n", FixedStack, n);
@@ -119,18 +123,26 @@ runtime·stackalloc(uint32 n)
                m->stackcachepos = pos;
                m->stackcachecnt--;
                m->stackinuse++;
-               return v;
-       }
-       return runtime·mallocgc(n, 0, FlagNoProfiling|FlagNoGC|FlagNoZero|FlagNoInvokeGC);
+               malloced = false;
+       } else
+               v = runtime·mallocgc(n, 0, FlagNoProfiling|FlagNoGC|FlagNoZero|FlagNoInvokeGC);
+
+       top = (Stktop*)((byte*)v+n-sizeof(Stktop));
+       runtime·memclr((byte*)top, sizeof(*top));
+       top->malloced = malloced;
+       return v;
 }
 
 void
-runtime·stackfree(void *v, uintptr n)
+runtime·stackfree(G *gp, void *v, Stktop *top)
 {
        uint32 pos;
+       uintptr n;
 
+       n = (uintptr)(top+1) - (uintptr)v;
        if(StackDebug >= 1)
                runtime·printf("stackfree %p %d\n", v, (int32)n);
+       gp->stacksize -= n;
        if(StackFromSystem) {
                if(StackFaultOnFree)
                        runtime·SysFault(v, n);
@@ -138,18 +150,19 @@ runtime·stackfree(void *v, uintptr n)
                        runtime·SysFree(v, n, &mstats.stacks_sys);
                return;
        }
-
-       if(n == FixedStack || m->mallocing || m->gcing) {
-               if(m->stackcachecnt == StackCacheSize)
-                       stackcacherelease();
-               pos = m->stackcachepos;
-               m->stackcache[pos] = v;
-               m->stackcachepos = (pos + 1) % StackCacheSize;
-               m->stackcachecnt++;
-               m->stackinuse--;
+       if(top->malloced) {
+               runtime·free(v);
                return;
        }
-       runtime·free(v);
+       if(n != FixedStack)
+               runtime·throw("stackfree: bad fixed size");
+       if(m->stackcachecnt == StackCacheSize)
+               stackcacherelease();
+       pos = m->stackcachepos;
+       m->stackcache[pos] = v;
+       m->stackcachepos = (pos + 1) % StackCacheSize;
+       m->stackcachecnt++;
+       m->stackinuse--;
 }
 
 // Called from runtime·lessstack when returning from a function which
@@ -202,11 +215,7 @@ runtime·oldstack(void)
        gp->stackguard = top->stackguard;
        gp->stackguard0 = gp->stackguard;
        gp->panicwrap = top->panicwrap;
-
-       if(top->free != 0) {
-               gp->stacksize -= top->free;
-               runtime·stackfree(old, top->free);
-       }
+       runtime·stackfree(gp, old, top);
 
        gp->status = oldstatus;
        runtime·gogo(&gp->sched);
@@ -498,6 +507,8 @@ copystack(G *gp, uintptr nframes, uintptr newsize)
        byte *oldstk, *oldbase, *newstk, *newbase;
        uintptr oldsize, used;
        AdjustInfo adjinfo;
+       Stktop *oldtop, *newtop;
+       bool malloced;
 
        if(gp->syscallstack != 0)
                runtime·throw("can't handle stack copy in syscall yet");
@@ -505,13 +516,17 @@ copystack(G *gp, uintptr nframes, uintptr newsize)
        oldbase = (byte*)gp->stackbase + sizeof(Stktop);
        oldsize = oldbase - oldstk;
        used = oldbase - (byte*)gp->sched.sp;
+       oldtop = (Stktop*)gp->stackbase;
 
        // allocate new stack
-       newstk = runtime·stackalloc(newsize);
+       newstk = runtime·stackalloc(gp, newsize);
        newbase = newstk + newsize;
+       newtop = (Stktop*)(newbase - sizeof(Stktop));
+       malloced = newtop->malloced;
 
        if(StackDebug >= 1)
                runtime·printf("copystack [%p %p]/%d -> [%p %p]/%d\n", oldstk, oldbase, (int32)oldsize, newstk, newbase, (int32)newsize);
+       USED(oldsize);
        
        // adjust pointers in the to-be-copied frames
        adjinfo.oldstk = oldstk;
@@ -523,11 +538,12 @@ copystack(G *gp, uintptr nframes, uintptr newsize)
        adjustctxt(gp, &adjinfo);
        adjustdefers(gp, &adjinfo);
        
-       // copy the stack to the new location
+       // copy the stack (including Stktop) to the new location
        runtime·memmove(newbase - used, oldbase - used, used);
+       newtop->malloced = malloced;
        
        // Swap out old stack for new one
-       gp->stackbase = (uintptr)newbase - sizeof(Stktop);
+       gp->stackbase = (uintptr)newtop;
        gp->stackguard = (uintptr)newstk + StackGuard;
        gp->stackguard0 = (uintptr)newstk + StackGuard; // NOTE: might clobber a preempt request
        if(gp->stack0 == (uintptr)oldstk)
@@ -535,7 +551,7 @@ copystack(G *gp, uintptr nframes, uintptr newsize)
        gp->sched.sp = (uintptr)(newbase - used);
 
        // free old stack
-       runtime·stackfree(oldstk, oldsize);
+       runtime·stackfree(gp, oldstk, oldtop);
 }
 
 // round x up to a power of 2.
@@ -566,7 +582,6 @@ runtime·newstack(void)
        G *gp;
        Gobuf label;
        bool newstackcall;
-       uintptr free;
 
        if(m->morebuf.g != m->curg) {
                runtime·printf("runtime: newstack called from g=%p\n"
@@ -669,14 +684,12 @@ runtime·newstack(void)
                framesize = StackMin;
        framesize += StackSystem;
        framesize = round2(framesize);
-       gp->stacksize += framesize;
+       stk = runtime·stackalloc(gp, framesize);
        if(gp->stacksize > runtime·maxstacksize) {
                runtime·printf("runtime: goroutine stack exceeds %D-byte limit\n", (uint64)runtime·maxstacksize);
                runtime·throw("stack overflow");
        }
-       stk = runtime·stackalloc(framesize);
        top = (Stktop*)(stk+framesize-sizeof(*top));
-       free = framesize;
 
        if(StackDebug >= 1) {
                runtime·printf("\t-> new stack [%p, %p]\n", stk, top);
@@ -687,7 +700,6 @@ runtime·newstack(void)
        top->gobuf = m->morebuf;
        top->argp = m->moreargp;
        top->argsize = argsize;
-       top->free = free;
        m->moreargp = nil;
        m->morebuf.pc = (uintptr)nil;
        m->morebuf.lr = (uintptr)nil;
@@ -805,6 +817,7 @@ runtime·shrinkstack(G *gp)
        gp->stackguard0 = (uintptr)oldstk + newsize + StackGuard;
        if(gp->stack0 == (uintptr)oldstk)
                gp->stack0 = (uintptr)oldstk + newsize;
+       gp->stacksize -= oldsize - newsize;
 
        // Free bottom half of the stack.  First, we trick malloc into thinking
        // we allocated the stack as two separate half-size allocs.  Then the

diff --git a/src/pkg/runtime/stack.h b/src/pkg/runtime/stack.h
index 5175b98080185293d0f06902ac703a1e6c034936..df965e158763322061d95071a67a1cfc8e69402c 100644 (file)
--- a/src/pkg/runtime/stack.h
+++ b/src/pkg/runtime/stack.h
@@ -102,7 +102,7 @@ enum {
        // The assumed size of the top-of-stack data block.
        // The actual size can be smaller than this but cannot be larger.
        // Checked in proc.c's runtime.malg.
-       StackTop = 96,
+       StackTop = 88,
 };
 
 // Goroutine preemption request.