code review 3993044: 8l, runtime: place G and M pointers relative to _tos on...

8l, runtime: place G and M pointers relative to _tos on Plan 9, instead of hardcoded values for USTKTOP.

This should allow executing both on native Plan 9 and inside 9vx.

[paulzhol, 2011-01-19 20:22:58]

http://codereview.appspot.com/3993044/diff/1/src/pkg/runtime/plan9/386/sys.s
File src/pkg/runtime/plan9/386/sys.s (right):

http://codereview.appspot.com/3993044/diff/1/src/pkg/runtime/plan9/386/sys.s#...
src/pkg/runtime/plan9/386/sys.s:70: // more paranoia; check that stack splitting
code works
These are commented out because 9vx does not implement PUSHAL, POPAL
instructions.

[paulzhol, 2011-01-19 20:24:07]

Hello rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[rsc, 2011-02-01 20:55:41]

I apologize for not getting to these code reviews earlier.

Please take a look and see whether you can find a way
to incorporate the most recent garbage collector changes
in some reasonable way.  I am honestly not sure how
to implement the interface on Plan 9.  It is possible that
the answer is to use sbrk in SysReserve and then make
SysMap a no-op, depending on demand paging to avoid
charging the process for the memory until it actually
touches those pages.  But I don't remember enough to
know whether that will actually work.

Russ

[paulzhol, 2011-02-02 13:05:54]

Hi!

I've modified mem.c to round addresses to page boundaries, otherwise using the
old 8 byte rounding, mlookup() fails to find the address. 

I added SysMap() a no-op as you suggested, and SysReserve() as a wrapper to
SysAlloc() although I didn't see where they are called for non-64bit platforms.

Pavel

[rsc, 2011-02-02 20:32:46]

> I've modified mem.c to round addresses to page boundaries, otherwise
> using the old 8 byte rounding, mlookup() fails to find the address.
>
> I added SysMap() a no-op as you suggested, and SysReserve() as a wrapper
> to SysAlloc() although I didn't see where they are called for non-64bit
> platforms.

That's the next CL to be checked in (3975046).

Russ

[paulzhol, 2011-02-02 21:43:56]

> That's the next CL to be checked in (3975046).

I've applied it manually (I can't get hg clpatch to import due to
runtime/windows/mem.c). I can get away with running a hello.go which uses
println, but a modified sieve running with runtime.GOMAXPROCS(4) set is dying in
mgc0.c:228:
s = runtime·mheap.map[((uintptr)b>>PageShift) - k0];

the 386 Linux version is not doing to great either:

panic: invalid memory address or nil pointer dereference
throw: panic during gc

[signal 0xb code=0x1 addr=0x1c pc=0x804ce2e]

runtime.throw+0x3e
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/runtime.c:78
        runtime.throw(0x806e828, 0x806e69f)
runtime.panicstring+0x51
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/runtime.c:89
        runtime.panicstring(0x806e69f, 0x100000)
runtime.sigpanic+0x11b
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/linux/thread.c:291
        runtime.sigpanic()
scanblock+0x298
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/mgc0.c:228
        scanblock(0x806e000, 0x246a, 0x0)
mark+0x3d
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/mgc0.c:325
        mark()
runtime.gc+0x1db
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/mgc0.c:527
        runtime.gc(0x0, 0x40)
runtime.mallocgc+0x1d9
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/malloc.c:89
        runtime.mallocgc(0x40, 0x0, 0x1, 0x1, 0x0, ...)
runtime.mal+0x43
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/malloc.c:282
        runtime.mal(0x38, 0x0)
runtime.makechan_c+0x86
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/chan.c:103
        runtime.makechan_c(0x8057e7c, 0x0, 0x0)
runtime.makechan+0x39
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/chan.c:148
        runtime.makechan(0x8057e7c, 0x0, 0x0, 0x8051765, 0x806f984, ...)
main.Sieve+0x36 /home/yuval/Data/devel/go-test/sieve.go:31
        main.Sieve()
main.main+0x2b /home/yuval/Data/devel/go-test/sieve.go:44
        main.main()
runtime.mainstart+0xf
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/386/asm.s:85
        runtime.mainstart()
runtime.goexit
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/proc.c:149
        runtime.goexit()

probably not related but cgo fails on jasonrpc.

Pavel

[rsc, 2011-02-03 04:03:41]

On 2011/02/02 21:43:56, paulzhol wrote:
> > That's the next CL to be checked in (3975046).
> 
> I've applied it manually (I can't get hg clpatch to import due to
> runtime/windows/mem.c). I can get away with running a hello.go which uses
> println, but a modified sieve running with runtime.GOMAXPROCS(4) set is dying
in
> mgc0.c:228:
> s = runtime·mheap.map[((uintptr)b>>PageShift) - k0];

Fixed in the submitted CL.

[paulzhol, 2011-02-03 09:04:29]

On 2011/02/03 04:03:41, rsc wrote:
> Fixed in the submitted CL.

sieve is running again on native Plan 9:

glenda          229    0:00   0:02   792828K Semacqui sieve
glenda          230    0:00   0:01   792828K Semacqui sieve
glenda          231    0:00   0:02   792828K Semacqui sieve
glenda          232    0:00   0:01   792828K _write   sieve

but I'm getting a page fault on 9vx:

acid: cont()
199: page fault	runtime.MCache_Alloc+0x39	MOVL	0x0(AX),AX
Notes pending:
	sys: trap: page fault
acid: *PC
0x00002128
acid: regs()
PC	0x00002128 runtime.MCache_Alloc+0x39 
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/mcache.c:21
SP	0x0ffffe74 ECODE 0x00000000 EFLAG 0x00000000
CS	0x00000000 DS	 0x00000000 SS	0x00000000
GS	0x00000000 FS	 0x00000000 ES	0x00000000
TRAP	0x0000000e page fault
AX	0x3043a00c BX	0x3043a00c CX	0x0000000c DX	0x00000001
DI	0x3043a000 SI	0x00000008 BP	0x00000000
acid: src(*PC)
/home/yuval/Data/devel/custom-build/golang.heap/src/pkg/runtime/mcache.c:21
 16		MLink *first, *v;
 17		int32 n;
 18	
 19		// Allocate from list.
 20		l = &c->list[sizeclass];
>21		if(l->list == nil) {
 22			// Replenish using central lists.
 23			n = runtime·MCentral_AllocList(&runtime·mheap.central[sizeclass],
 24				runtime·class_to_transfercount[sizeclass], &first);
 25			l->list = first;
 26			l->nlist = n;

Do you think sigattach should be used instead of brk for SysReserve ?

Pavel

[rsc, 2011-02-03 19:33:42]

Let's get native Plan 9 working.

I never cease to be amazed that 9vx works as
well as it does, but broken on 9vx still has a
good chance of being 9vx's fault.

Russ

[paulzhol, 2011-02-03 19:46:38]

On 2011/02/03 19:33:42, rsc wrote:
> Let's get native Plan 9 working.
> 
> I never cease to be amazed that 9vx works as
> well as it does, but broken on 9vx still has a
> good chance of being 9vx's fault.

I'm using Ron Minnich' 9vx branch with fixes for seqmacquire/release. I'll ask
his help with this.

As a side note, what are the chances of http://codereview.appspot.com/3816043/
getting merged in ? Should I split it into 2 CL's ? is the os change too big,
maybe I could maintain it as a collection of ed scripts outside of the tree.

Pavel

[rsc, 2011-02-03 19:58:34]

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/386/sys.s
File src/pkg/runtime/plan9/386/sys.s (right):

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/386/sy...
src/pkg/runtime/plan9/386/sys.s:71: //	PUSHAL
Why is this commented out?

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c#...
src/pkg/runtime/plan9/mem.c:56: while((p = runtime·SysAlloc((byte*)v-(byte*)p))
< v)
I'm a little confused.
Why is this a while loop?
Why is there a final call below?
Why doesn't the loop refer to nbytes?

[rsc, 2011-02-03 19:59:40]

> As a side note, what are the chances of
> http://codereview.appspot.com/3816043/ getting merged in ? Should I
> split it into 2 CL's ? is the os change too big, maybe I could maintain
> it as a collection of ed scripts outside of the tree.

We're getting there.  If you can split the syscall stuff
into a different CL that would certainly speed it up
a little.

I want to wait until the new os.Process API is checked
in before we check in the Plan 9 changes to os, though.

Russ

[paulzhol, 2011-02-03 20:14:31]

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/386/sys.s
File src/pkg/runtime/plan9/386/sys.s (right):

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/386/sy...
src/pkg/runtime/plan9/386/sys.s:71: //	PUSHAL
On 2011/02/03 19:58:34, rsc wrote:
> Why is this commented out?

I was getting invalid opcode 60 e8 5e for PUSHAL on 9vx, before the garbage
collector changes.

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c#...
src/pkg/runtime/plan9/mem.c:56: while((p = runtime·SysAlloc((byte*)v-(byte*)p))
< v)
On 2011/02/03 19:58:34, rsc wrote:
> I'm a little confused.
> Why is this a while loop?
> Why is there a final call below?
> Why doesn't the loop refer to nbytes?

I try to get brk above the requested address, that's what the while loop is for.
Once I'm there, I allocate nbytes, this way I make sure there are at least
nbytes from v, and return v.

[paulzhol, 2011-02-03 22:54:57]

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c
File src/pkg/runtime/plan9/mem.c (right):

http://codereview.appspot.com/3993044/diff/18008/src/pkg/runtime/plan9/mem.c#...
src/pkg/runtime/plan9/mem.c:56: while((p = runtime·SysAlloc((byte*)v-(byte*)p))
< v)
err.. sorry, I don't know what I was thinking, will drop the while loop and
directly call SysAlloc once.

[paulzhol, 2011-02-10 21:32:36]

Recent allocator changes make v a hint for SysReserve, which in turn can just
call SysAlloc.

Hello and sieve both run on native Plan 9, still failing in runtime/mcache.c:21
on 9vx.

Pavel

[rsc, 2011-02-11 18:39:03]

LGTM

[rsc, 2011-02-11 18:39:07]

*** Submitted as 1d10645da15a ***

8l, runtime: place G and M pointers relative to _tos on Plan 9, instead of
hardcoded values for USTKTOP.

This should allow executing both on native Plan 9 and inside 9vx.

R=rsc
CC=golang-dev
http://codereview.appspot.com/3993044

Committer: Russ Cox <rsc@golang.org>