code review 10126044: runtime: more flexible heap memory mapping on 64-bits

runtime: more flexible heap memory mapping on 64-bits
Fixes issue 5641.

[dvyukov, 2013-06-10 08:49:29]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2013-06-10 08:50:30]

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

Please take another look.

[dave_cheney.net, 2013-06-10 09:20:43]

s/mere/more/ in the title. 



On 10/06/2013, at 18:49, dvyukov@google.com wrote:

> Reviewers: golang-dev1,
> 
> Message:
> Hello golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
> 
> 
> Description:
> runtime: mere flexible heap memory mapping on 64-bits
> Fixes issue 5641.
> 
> Please review this at https://codereview.appspot.com/10126044/
> 
> Affected files:
>  A misc/cgo/testasan/main.go
>  M src/pkg/runtime/malloc.goc
>  M src/run.bash
> 
> 
> Index: misc/cgo/testasan/main.go
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/misc/cgo/testasan/main.go
> @@ -0,0 +1,51 @@
> +// Copyright 2013 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +package main
> +
> +/*
> +#include <sys/mman.h>
> +#include <pthread.h>
> +#include <unistd.h>
> +
> +void ctor(void) __attribute__((constructor));
> +static void* thread(void*);
> +
> +void ctor(void)
> +{
> +    // occupy memory where Go runtime would normally map heap
> +    mmap((void*)0x00c000000000, 64<<10, PROT_READ|PROT_WRITE,
MAP_PRIVATE|MAP_ANONYMOUS|MAP_FIXED, -1, 0);
> +
> +    // allocate 4K every 10us
> +    pthread_t t;
> +    pthread_create(&t, 0, thread, 0);
> +}
> +
> +void* thread(void *p)
> +{
> +    for(;;) {
> +        usleep(10000);
> +        mmap(0, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1,
0);
> +    }
> +    return 0;
> +}
> +*/
> +import "C"
> +
> +import (
> +    "fmt"
> +    "time"
> +)
> +
> +func main() {
> +    // say where the heap is
> +    fmt.Printf("%v\n", new(int))
> +
> +    // ensure that we can functionate normally
> +    var v [][]byte
> +    for i := 0; i < 1000; i++ {
> +        time.Sleep(10*time.Microsecond)
> +        v = append(v, make([]byte, 64<<10))
> +    }
> +}
> Index: src/pkg/runtime/malloc.goc
> ===================================================================
> --- a/src/pkg/runtime/malloc.goc
> +++ b/src/pkg/runtime/malloc.goc
> @@ -303,6 +303,7 @@
>    extern byte end[];
>    byte *want;
>    uintptr limit;
> +    int32 i;
> 
>    p = nil;
>    arena_size = 0;
> @@ -330,15 +331,17 @@
>        // 128 GB (MaxMem) should be big enough for now.
>        //
>        // The code will work with the reservation at any address, but ask
> -        // SysReserve to use 0x000000c000000000 if possible.
> +        // SysReserve to use 0x0000XXc000000000 if possible (XX=00...7f).
>        // Allocating a 128 GB region takes away 37 bits, and the amd64
>        // doesn't let us choose the top 17 bits, so that leaves the 11 bits
>        // in the middle of 0x00c0 for us to choose.  Choosing 0x00c0 means
>        // that the valid memory addresses will begin 0x00c0, 0x00c1, ...,
0x0x00df.
>        // In little-endian, that's c0 00, c1 00, ..., df 00. None of those are
valid
>        // UTF-8 sequences, and they are otherwise as far away from
> -        // ff (likely a common byte) as possible. An earlier attempt to use
0x11f8
> -        // caused out of memory errors on OS X during thread allocations.
> +        // ff (likely a common byte) as possible.  If that fail, we try other
0xXXc0
> +        // addresses.  An earlier attempt to use 0x11f8 caused out of memory
errors
> +        // on OS X during thread allocations.  0x00c0 causes conflicts with
> +        // AddressSanitizer which reserves all memory up to 0x0100.
>        // These choices are both for debuggability and to reduce the
>        // odds of the conservative garbage collector not collecting memory
>        // because some non-pointer block of memory had a bit pattern
> @@ -351,7 +354,15 @@
>        arena_size = MaxMem;
>        bitmap_size = arena_size / (sizeof(void*)*8/4);
>        spans_size = arena_size / PageSize * sizeof(runtime·mheap.spans[0]);
> -        p = runtime·SysReserve((void*)(0x00c0ULL<<32), bitmap_size +
spans_size + arena_size);
> +        for(i = -1; i <= 0x7f; i++) {
> +            if(i == -1)
> +                p = (void*)(0x00c0ULL<<32);
> +            else
> +                p = (void*)((uint64)i<<40 | 0x00c0ULL<<32);
> +            p = runtime·SysReserve(p, bitmap_size + spans_size +
arena_size);
> +            if(p != nil)
> +                break;
> +        }
>    }
>    if (p == nil) {
>        // On a 32-bit machine, we can't typically get away
> Index: src/run.bash
> ===================================================================
> --- a/src/run.bash
> +++ b/src/run.bash
> @@ -108,6 +108,12 @@
> ./test.bash
> ) || exit $?
> 
> +[ "$CGO_ENABLED" != 1 ] ||
> +[ "$GOHOSTOS-$GOARCH" != linux-amd64 ] ||
> +(xcd ../misc/cgo/testasan
> +go run main.go
> +) || exit $?
> +
> (xcd ../doc/progs
> time ./run
> ) || exit $?
> 
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[dvyukov, 2013-06-10 09:22:49]

On 2013/06/10 09:20:43, dfc wrote:
> s/mere/more/ in the title. 


Done

[DMorsing, 2013-06-10 09:36:22]

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:306: int32 i;
Use int64 here to avoid a cast.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:334: // SysReserve to use 0x0000XXc000000000 if
possible (XX=00...7f).
There are a couple of places in this comment where 00 hasn't been replaced with
XX.

The code is extremely subtle, so I'd like to keep this comment as up to date as
possible for any future code archaeology.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:337: // in the middle of 0x00c0 for us to choose. 
Choosing 0x00c0 means
Any sequence c0 xx where xx =< 7f is invalid UTF-8. Not sure if that's worth
pointing out.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:338: // that the valid memory addresses will begin
0x00c0, 0x00c1, ..., 0x0x00df.
double 0x on 0x0x00df

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:358: if(i == -1)
why the if here?

If i == 0, then "(void*)((uint64)i<<40 | 0x00c0ULL<<32);" would evaluate to
0x00c0ULL<<32

[dave_cheney.net, 2013-06-10 12:28:39]

https://codereview.appspot.com/10126044/diff/15001/src/run.bash
File src/run.bash (right):

https://codereview.appspot.com/10126044/diff/15001/src/run.bash#newcode112
src/run.bash:112: [ "$GOHOSTOS-$GOARCH" != linux-amd64 ] ||
not testing windows/amd64 or darwin/amd64 ?

[dvyukov, 2013-06-10 13:32:03]

On 2013/06/10 12:28:39, dfc wrote:
> https://codereview.appspot.com/10126044/diff/15001/src/run.bash
> File src/run.bash (right):
> 
> https://codereview.appspot.com/10126044/diff/15001/src/run.bash#newcode112
> src/run.bash:112: [ "$GOHOSTOS-$GOARCH" != linux-amd64 ] ||
> not testing windows/amd64 or darwin/amd64 ?

Darwin does not use MAP_FIXED, so at least on my machine it is not using 0x00c0
anyway. So the test won't actually test what it is intended to test.
Windows test will require radically different code for constructors (will it
require dll?), threads, memory mapping and sleep. So it will be a different
test.

[dvyukov, 2013-06-10 13:32:09]

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:306: int32 i;
On 2013/06/10 09:36:22, DMorsing wrote:
> Use int64 here to avoid a cast.

I am not sure here. Then it will involve signed shift that is not visible in the
code. "(uint64)i<<40" looks more bulletproof.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:334: // SysReserve to use 0x0000XXc000000000 if
possible (XX=00...7f).
On 2013/06/10 09:36:22, DMorsing wrote:
> There are a couple of places in this comment where 00 hasn't been replaced
with
> XX.
> 
> The code is extremely subtle, so I'd like to keep this comment as up to date
as
> possible for any future code archaeology.

This is intentional. That place describes what happens if we choose 0x00c0,
which we choose be default (as before). After that I've added "If that fail, we
try other 0xXXc0 addresses."

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:337: // in the middle of 0x00c0 for us to choose. 
Choosing 0x00c0 means
On 2013/06/10 09:36:22, DMorsing wrote:
> Any sequence c0 xx where xx =< 7f is invalid UTF-8. Not sure if that's worth
> pointing out.

Is that true? I don't know.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:338: // that the valid memory addresses will begin
0x00c0, 0x00c1, ..., 0x0x00df.
On 2013/06/10 09:36:22, DMorsing wrote:
> double 0x on 0x0x00df

Done.

https://codereview.appspot.com/10126044/diff/15001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:358: if(i == -1)
On 2013/06/10 09:36:22, DMorsing wrote:
> why the if here?
> 
> If i == 0, then "(void*)((uint64)i<<40 | 0x00c0ULL<<32);" would evaluate to
> 0x00c0ULL<<32

Some deployments use patched default address. It makes it possible to have
minimal diff. So the idea is that it starts from -N, then we can enumerate N
special cases in the necessary order. And only then try all 0xXXc0 in order.

[iant, 2013-06-10 17:00:00]

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go
File misc/cgo/testasan/main.go (right):

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:25: void* thread(void *p)
The declaration above is static, so the function should be static.

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:43: fmt.Printf("%v\n", new(int))
Probably this test shouldn't print anything in the normal case.  That makes the
run.bash output simpler and easier to interpret.

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:45: // ensure that we can functionate normally
Not sure what "functionate" means, do you mean "function"?

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:341: // ff (likely a common byte) as possible.  If
that fail, we try other 0xXXc0
s/fail/fails/

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:358: if(i == -1)
I saw your argument about why you use -1, but it makes this code too confusing. 
This loop should start at 0 and you should skip the -1 case.  And then I think
you should make i have type uint64.

[dvyukov, 2013-06-10 18:29:58]

PTAL

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go
File misc/cgo/testasan/main.go (right):

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:25: void* thread(void *p)
On 2013/06/10 17:00:00, iant wrote:
> The declaration above is static, so the function should be static.

Done.

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:43: fmt.Printf("%v\n", new(int))
On 2013/06/10 17:00:00, iant wrote:
> Probably this test shouldn't print anything in the normal case.  That makes
the
> run.bash output simpler and easier to interpret.

Done.

https://codereview.appspot.com/10126044/diff/20001/misc/cgo/testasan/main.go#...
misc/cgo/testasan/main.go:45: // ensure that we can functionate normally
On 2013/06/10 17:00:00, iant wrote:
> Not sure what "functionate" means, do you mean "function"?

Done.

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:341: // ff (likely a common byte) as possible.  If
that fail, we try other 0xXXc0
On 2013/06/10 17:00:00, iant wrote:
> s/fail/fails/

Done.

https://codereview.appspot.com/10126044/diff/20001/src/pkg/runtime/malloc.goc...
src/pkg/runtime/malloc.goc:358: if(i == -1)
On 2013/06/10 17:00:00, iant wrote:
> I saw your argument about why you use -1, but it makes this code too
confusing. 
> This loop should start at 0 and you should skip the -1 case.  And then I think
> you should make i have type uint64.

Done.

[iant, 2013-06-10 21:17:39]

LGTM

[dvyukov, 2013-06-12 14:47:29]

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

runtime: more flexible heap memory mapping on 64-bits
Fixes issue 5641.

R=golang-dev, dave, daniel.morsing, iant
CC=golang-dev, kcc
https://codereview.appspot.com/10126044