code review 6850081: runtime: implement runtime.SysUnused on FreeBSD

runtime: implement runtime.SysUnused on FreeBSD

madvise was missing so implement it in assembler. This change
needs to be extended to the other BSD variantes (Net and Open)

Without this change the scavenger will attempt to pass memory back
to the operating system when it has become idle, but the memory is
not returned and for long running Go processes the total memory used
can grow until OOM occurs.

I have only been able to test the code on FreeBSD AMD64. The ARM
platforms needs testing.

[jgc, 2012-11-20 20:52:05]

Hello golang-dev@googlegroups.com,

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

[mikio, 2012-11-21 00:33:14]

Perhaps CL description would be
"runtime: madvise and SysUnused for FreeBSD" like CL 5531073.

Can you also provide freebsd/386 patches?

And a nit.

diff -r 1315abc581ed src/pkg/runtime/thread_freebsd.c
--- a/src/pkg/runtime/thread_freebsd.c
+++ b/src/pkg/runtime/thread_freebsd.c
-       param.arg = m;
+       param.arg = (byte*)m;

https://codereview.appspot.com/6850081/diff/5001/src/pkg/runtime/defs_freebsd.go
File src/pkg/runtime/defs_freebsd.go (right):

https://codereview.appspot.com/6850081/diff/5001/src/pkg/runtime/defs_freebsd...
src/pkg/runtime/defs_freebsd.go:11: GOARCH=386 cgo -cdefs defs.go >386/defs.h
please fix above comments too.

GOARCH=amd64 go tool cgo -cdefs defs_freebsd.go >defs_freebsd_amd64.h
GOARCH=386 go tool cgo -cdefs defs_freebsd.go >defs_freebsd_386.h

https://codereview.appspot.com/6850081/diff/5001/src/pkg/runtime/defs_freebsd...
src/pkg/runtime/defs_freebsd.go:50: MADV_FREE = C.MADV_FREE
please move up after MAP definitions like other platform files.

https://codereview.appspot.com/6850081/diff/5001/src/pkg/runtime/defs_freebsd...
File src/pkg/runtime/defs_freebsd_amd64.h (right):

https://codereview.appspot.com/6850081/diff/5001/src/pkg/runtime/defs_freebsd...
src/pkg/runtime/defs_freebsd_amd64.h:1: // godefs -f -m64 defs.c
please regenerate this file with:
GOARCH=amd64 go tool cgo -cdefs defs_freebsd.go >defs_freebsd_amd64.h

[dfc, 2012-11-21 01:27:39]

Here is the freebsd/arm version, you can import it into your CL with

cd $GOROOT/src/pkg/runtime
hg import https://codereview.appspot.com/download/issue6847077_2001_3001.diff
--force --no-commit
hg file 6850081 sys_freebsd_arm.s

[dfc, 2012-11-21 01:30:02]

> please regenerate this file with:
> GOARCH=amd64 go tool cgo -cdefs defs_freebsd.go >defs_freebsd_amd64.h

I think it is ok to hand patch this one (and defs_freebsd_{386,arm}.h), follow
the pattern from the defs_darwin_*.h files.

I tried to regenerate the files yesterday and they produced a large diff (I was
running from FreeBSD-CURRENT). I think it is ok to hand patch and review this
change, then Mikio or I can regenerate the defs_ files in a followup change.

[dfc, 2012-11-21 01:45:08]

https://codereview.appspot.com/6854077 should work for freebsd/386.

Just a note about the description. Without this change the scavenger will not
return blocks to the OS, but that should not have any impact on the operation of
the GC. From it's point of view, the heap is always fully mapped (just some
parts may have been hinted to the OS that they aren't in use).

To say that other way, regardless of this change, if you heap is growing without
bounds, that might be a different bug.

[jgc, 2012-11-22 10:55:04]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, dave@cheney.net (cc:
golang-dev@googlegroups.com),

Please take another look.

[dfc, 2012-11-22 11:05:02]

LGTM. Thank you.

Hopefully Mikio or Minux can check freebsd/386 and freebsd/arm.

John, while you're waiting for these checks and a few more LGTMs, can you please
complete the CLA procedure described here http://golang.org/doc/contribute.html.

[jgc_jgc.org, 2012-11-22 11:10:46]

On Thu, Nov 22, 2012 at 11:05 AM, <dave@cheney.net> wrote:

> LGTM. Thank you.
>
> Hopefully Mikio or Minux can check freebsd/386 and freebsd/arm.
>
> John, while you're waiting for these checks and a few more LGTMs, can
> you please complete the CLA procedure described here
>
http://golang.org/doc/**contribute.html<http://golang.org/doc/contribute.html>
> .
>
>
https://codereview.appspot.**com/6850081/<https://codereview.appspot.com/6850...
>

Done. I have signed the appropriate agreement electronically.

John.

[mikio, 2012-11-23 05:38:54]

Please change the CL description to:
runtime: implement madvise and SysUnused for FreeBSD.

It passes on freebsd/386, thank you.
I'll leave for minux or dfc.

https://codereview.appspot.com/6850081/diff/10002/src/pkg/runtime/defs_freebs...
File src/pkg/runtime/defs_freebsd.go (right):

https://codereview.appspot.com/6850081/diff/10002/src/pkg/runtime/defs_freebs...
src/pkg/runtime/defs_freebsd.go:41: MADV_FREE   = C.MADV_FREE
pls put a linebreak before MADV_FREE, thanks.

[dfc, 2012-11-23 06:17:48]

Mikio, I think we should regenerate these files in a follow up CL, are you able
to assist with FreeBSD/amd64 ?

On 23/11/2012, at 16:38, mikioh.mikioh@gmail.com wrote:

> Please change the CL description to:
> runtime: implement madvise and SysUnused for FreeBSD.
> 
> It passes on freebsd/386, thank you.
> I'll leave for minux or dfc.
> 
> 
>
https://codereview.appspot.com/6850081/diff/10002/src/pkg/runtime/defs_freebs...
> File src/pkg/runtime/defs_freebsd.go (right):
> 
>
https://codereview.appspot.com/6850081/diff/10002/src/pkg/runtime/defs_freebs...
> src/pkg/runtime/defs_freebsd.go:41: MADV_FREE   = C.MADV_FREE
> pls put a linebreak before MADV_FREE, thanks.
> 
> https://codereview.appspot.com/6850081/

[mikio, 2012-11-23 06:26:55]

On Fri, Nov 23, 2012 at 3:17 PM, Dave Cheney <dave@cheney.net> wrote:

> Mikio, I think we should regenerate these files in a follow up CL, are you
able to assist with FreeBSD/amd64 ?

Sure.

[jgc_jgc.org, 2012-11-23 09:31:08]

On Fri, Nov 23, 2012 at 5:38 AM, <mikioh.mikioh@gmail.com> wrote:

> Please change the CL description to:
> runtime: implement madvise and SysUnused for FreeBSD.
>

Done.


> It passes on freebsd/386, thank you.
> I'll leave for minux or dfc.
>

OK. So, is there anyone who can test this out on ARM?

John.

[jgc, 2012-11-23 09:31:59]

Hello golang-dev@googlegroups.com, mikioh.mikioh@gmail.com, dave@cheney.net,
jgc@jgc.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[dfc, 2012-11-23 09:38:02]

I can vouch for arm. If it is wrong I'll address it when we get a FreeBSD/arm
builder. That itself isn't proving to be easy, the last attempt I tried got to
multiuser, but had no networking to speak of. 

Ian/brad/adg, can you please verify the CLA process and do the commit. 

Cheers

Dave

On 23/11/2012, at 20:31, John Graham-t <jgc@jgc.org> wrote:
I
> On Fri, Nov 23, 2012 at 5:38 AM, <mikioh.mikioh@gmail.com> wrote:
>> Please change the CL description to:
>> runtime: implement madvise and SysUnused for FreeBSD.
> 
> Done.
>  
>> It passes on freebsd/386, thank you.
>> I'll leave for minux or dfc.
> 
> OK. So, is there anyone who can test this out on ARM?
> 
> John.
>  

[minux1, 2012-11-23 09:47:34]

I'm testing on emulated FreeBSD/arm.
Will take quite some time.

[mikio, 2012-11-23 10:07:31]

On Fri, Nov 23, 2012 at 3:17 PM, Dave Cheney <dave@cheney.net> wrote:
> Mikio, I think we should regenerate these files in a follow up CL, are you
able to assist with FreeBSD/amd64 ?

Here you go: https://codereview.appspot.com/6855080/

[minux1, 2012-11-23 11:26:15]

LGTM on FreeBSD/ARM.

[dfc, 2012-11-24 04:55:26]

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

runtime: implement runtime.SysUnused on FreeBSD

madvise was missing so implement it in assembler. This change
needs to be extended to the other BSD variantes (Net and Open)

Without this change the scavenger will attempt to pass memory back
to the operating system when it has become idle, but the memory is
not returned and for long running Go processes the total memory used
can grow until OOM occurs.

I have only been able to test the code on FreeBSD AMD64. The ARM
platforms needs testing.

R=golang-dev, mikioh.mikioh, dave, jgc, minux.ma
CC=golang-dev
http://codereview.appspot.com/6850081

Committer: Dave Cheney <dave@cheney.net>

[dfc, 2012-11-24 05:40:52]

Thanks John. If possible, please keep the mailing list in the loop as to the
success or further problems using FreeBSD/amd64 with large long running
programs.

[jgc, 2012-11-24 09:07:22]

I will. Once this is in the tip it will be deployed and tested. 

John.


On 24 Nov 2012, at 05:40, dave@cheney.net wrote:

> Thanks John. If possible, please keep the mailing list in the loop as to
> the success or further problems using FreeBSD/amd64 with large long
> running programs.
> 
> https://codereview.appspot.com/6850081/

[minux1, 2012-11-24 09:12:20]

On Sat, Nov 24, 2012 at 5:07 PM, John Graham-Cumming <jgrahamc@gmail.com>wrote:

> I will. Once this is in the tip it will be deployed and tested.
>
FYI, This CL is already submitted to the default branch.