code review 5634050: runtime: drop to 32 bit malloc if 64 bit will not work

runtime: drop to 32 bit malloc if 64 bit will not work

On 64 bit UML it is not possible to reserve memory at 0xF8<<32.
Detect when linux cannot use these high virtual memory addresses
and drop back to the 32 bit memory allocator.

[rsc, 2012-02-06 19:08:43]

This seems small and reasonable.  Minor comments below.
Please run hg mail 5634050 when you're ready for another review;
that will CC golang-dev.

Thanks.

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/malloc.goc#ne...
src/pkg/runtime/malloc.goc:364: if(sizeof(void*) == 8 && (uintptr)(h->bitmap) >=
(1ULL << 32))
drop unnecessary () around h->bitmap

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/mem_linux.c
File src/pkg/runtime/mem_linux.c (right):

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:12: #define USE64(v) (sizeof(void*) == 8 &&
(uintptr)(v) >= (1ULL<<32))
This is pushing logic from portable code into OS-specific code.
If it's not appropriate for SysReserve, SysMap to do anything,
the portable code should avoid calling them.

There should be no changes to this file.

[borman, 2012-02-06 19:32:05]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[borman, 2012-02-06 19:32:32]

I am not sure I can easily do this without editing mem_linux.c.

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/malloc.goc
File src/pkg/runtime/malloc.goc (right):

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/malloc.goc#ne...
src/pkg/runtime/malloc.goc:364: if(sizeof(void*) == 8 && (uintptr)(h->bitmap) >=
(1ULL << 32))
On 2012/02/06 19:08:43, rsc wrote:
> drop unnecessary () around h->bitmap

Done.

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/mem_linux.c
File src/pkg/runtime/mem_linux.c (right):

http://codereview.appspot.com/5634050/diff/2001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:12: #define USE64(v) (sizeof(void*) == 8 &&
(uintptr)(v) >= (1ULL<<32))
On 2012/02/06 19:08:43, rsc wrote:
> This is pushing logic from portable code into OS-specific code.
> If it's not appropriate for SysReserve, SysMap to do anything,
> the portable code should avoid calling them.
> 
> There should be no changes to this file.

I see two problems if we don't change this code.   First, this code masks the
failure until after the arena is set up (I don't think we want an mmap in the
portable code).  Second, this code assumes that on 64 bit architectures we will
always use the 64 bit algorithm.  This is not always true now.

I am open to any suggestion on how to resolve those two issues.

[rsc, 2012-02-06 19:37:01]

What calls the functions you changed in mem_linux.c?
Can the change go there instead (at the call sites)?

Russ

[borman, 2012-02-06 19:59:56]

On 2012/02/06 19:37:01, rsc wrote:
> What calls the functions you changed in mem_linux.c?
> Can the change go there instead (at the call sites)?
> 
> Russ

It is malloc.goc:296 that makes this call.  Trying to move this logic up seems
to require that malloc.goc use the mmap system call.  I think this change (I
removed the other checks, I think it should work without them) needs to stay
here.

[cw, 2012-02-06 20:27:16]

Why not try the region between 1<<32 and 0xf8<<32?  Is the complexity not worth
the few extra bits?

[borman, 2012-02-06 20:39:54]

On 2012/02/06 20:27:16, cw wrote:
> Why not try the region between 1<<32 and 0xf8<<32?  Is the complexity not
worth
> the few extra bits?

The issue appears to happen only with UML.  Anything more than 32 bits fail IIRC
so you will end up with the 32 bit allocator anyhow.

[rsc, 2012-02-06 20:43:39]

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c
File src/pkg/runtime/mem_linux.c (right):

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:76: // if we can reserve at least 64K and check the
assumption in SysMap.
// Only user-mode Linux (UML) rejects these requests.

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:77: if(sizeof(void*) == 8) {
if(sizeof(void*) == 8 && (uintptr)v >= 0xffffffffU) {

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:103: if(sizeof(void*) == 8) {
if(sizeof(void*) == 8 && (uintptr)v >= 0xffffffffU) {

[borman, 2012-02-06 21:11:36]

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c
File src/pkg/runtime/mem_linux.c (right):

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:76: // if we can reserve at least 64K and check the
assumption in SysMap.
On 2012/02/06 20:43:39, rsc wrote:
> // Only user-mode Linux (UML) rejects these requests.

Done.

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:77: if(sizeof(void*) == 8) {
On 2012/02/06 20:43:39, rsc wrote:
> if(sizeof(void*) == 8 && (uintptr)v >= 0xffffffffU) {

Done.

But shouldn't this be > and not >=?  Or does the 32 bit build complain
otherwise?

http://codereview.appspot.com/5634050/diff/6001/src/pkg/runtime/mem_linux.c#n...
src/pkg/runtime/mem_linux.c:103: if(sizeof(void*) == 8) {
On 2012/02/06 20:43:39, rsc wrote:
> if(sizeof(void*) == 8 && (uintptr)v >= 0xffffffffU) {

Done.

[rsc, 2012-02-06 22:58:30]

On Mon, Feb 6, 2012 at 16:11,  <borman@google.com> wrote:
> But shouldn't this be > and not >=?  Or does the 32 bit build complain
> otherwise?

It seems much safer to use >= and not leave room for complaint.
These are aligned addresses so it's not any different.

Thanks for making the changes; please hg mail 5634050.

Russ

[borman, 2012-02-06 23:29:45]

On 2012/02/06 22:58:30, rsc wrote:
> On Mon, Feb 6, 2012 at 16:11,  <mailto:borman@google.com> wrote:
> > But shouldn't this be > and not >=? &nbsp;Or does the 32 bit build complain
> > otherwise?
> 
> It seems much safer to use >= and not leave room for complaint.
> These are aligned addresses so it's not any different.
> 
> Thanks for making the changes; please hg mail 5634050.
> 
> Russ

Okay, I also fixed the same expression in malloc.goc.
Doing the hg mail now.

[borman, 2012-02-06 23:29:57]

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

Please take another look.

[cw, 2012-02-07 00:55:25]

LGTM

[borman, 2012-02-08 19:22:46]

ping -- just to make sure this does not fall through the cracks

[rsc, 2012-02-08 19:33:09]

LGTM

[rsc, 2012-02-08 19:39:19]

*** Submitted as http://code.google.com/p/go/source/detail?r=36c9c7810f14 ***

runtime: drop to 32 bit malloc if 64 bit will not work

On 64 bit UML it is not possible to reserve memory at 0xF8<<32.
Detect when linux cannot use these high virtual memory addresses
and drop back to the 32 bit memory allocator.

R=rsc, cw
CC=golang-dev
http://codereview.appspot.com/5634050

Committer: Russ Cox <rsc@golang.org>