Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/link: better support for non-4K page size systems #10180

Closed
minux opened this issue Mar 16, 2015 · 19 comments
Closed

cmd/link: better support for non-4K page size systems #10180

minux opened this issue Mar 16, 2015 · 19 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@minux
Copy link
Member

minux commented Mar 16, 2015

At least three problems here:
1. cmd/ld INITRND needs to be at least as large as the page size, otherwise
we might trigger kernel bugs. See https://groups.google.com/forum/#!topic/golang-nuts/29RraQ-t3gc
for a case.

However, just using a larger INITRND value will make the binary bigger because
of the zero padding between segments.

I think we can have a way to have a much larger INITRND without increasing
the size of the binary by leaving a VM hole between the segments.
(e.g. text segment ends at N*INITRND + x, then we make the next segment's
load address start at (N+1)*INITRND + x + 1, so there is no hole in the file, but
different segments end up on different pages (if INITRND >= page size).

2. _PhysPageSize in runtime needs to be a variable and auto detected
at process start up. This might incur a performance overhead, but as the
proliferation of non-4K page size systems (power64, arm, arm64), we should
make the change to continue the Go tradition that it just works without extra
tuning. At least, we should make cmd/dist detect page size and tune the
const in runtime.

3. syscall.Getpagesize should return the value in the runtime. Right now,
it's hardcoded on all systems.

@minux minux added this to the Go1.5 milestone Mar 16, 2015
@davecheney
Copy link
Contributor

Detecting the page size inside cmd/dist sounds reasonable.

Dumb question time, what are the downsides of just assuming 64k pages
everywhere?
On 17 Mar 2015 07:15, "Minux Ma" notifications@github.com wrote:

At least three problems here:

  1. cmd/ld INITRND needs to be at least as large as the page size, otherwise
    we might trigger kernel bugs. See
    https://groups.google.com/forum/#!topic/golang-nuts/29RraQ-t3gc
    for a case.

I think we can have a way to have a much larger INITRND without increasing
the size of the binary by leaving a VM hole between the segments.
(e.g. text segment ends at N
_INITRND + x, then we make the next segment start at (N+1)_INITRND + x +
1, so there is no hole in the file, but we've made
sure different segments end up on different page (if INITRND > page size).

_PhysPageSize in runtime needs to be a variable and auto detected
at process start up. This might incur a performance overhead, but as
the
proliferation of non-4K page size systems (power64, arm, arm64), we
should
make the change to continue the Go tradition that it just works
without extra
tuning. At least, we should make cmd/dist detect page size and tune the
const in runtime.
2.

syscall.Getpagesize should return the value in the runtime. Right now,
it's hardcoded on all systems.


Reply to this email directly or view it on GitHub
#10180.

@randall77
Copy link
Contributor

I don't see any problem with _PhysPageSize being a variable, it is not used anywhere performance critical (except in the new memory barriers, but we can probably switch those to _PageSize if it matters).

The more difficult thing is to pick the right segment alignment in the binary. Because we cross compile you can't use any autodetection scheme. Picking the maximum alignment of all of our platforms might be fine.

@davecheney
Copy link
Contributor

On 17 Mar 2015 09:17, "Keith Randall" notifications@github.com wrote:

I don't see any problem with _PhysPageSize being a variable, it is not
used anywhere performance critical (except in the new memory barriers, but
we can probably switch those to _PageSize if it matters).

That was the one location that I saw that would be a performance concern if
it were not a constant expression.

We can get the system page size at startup from auxv or a syscall.

The more difficult thing is to pick the right segment alignment in the
binary. Because we cross compile you can't use any autodetection scheme.
Picking the maximum alignment of all of our platforms might be fine.

I'd like that to be the case.


Reply to this email directly or view it on GitHub.

@mwhudson
Copy link
Contributor

I think assuming the page size of the build and execution machines is a bad idea, even without thinking about cross compiling. OTOH, I can't see a downside to leaving plenty of VM space between different segments. What does binutils do?

davecheney added a commit that referenced this issue Mar 17, 2015
Updates #10180

Temporarily disable this test on ppc64 systems as all our builders use 64k page size.

We need a portable way to get the page size of the host so we can correctly size the mmap hole.

Change-Id: Ibd36ebe2f54cf75a44667e2070c385f0daaca481
Reviewed-on: https://go-review.googlesource.com/7652
Reviewed-by: Andrew Gerrand <adg@golang.org>
@ianlancetaylor
Copy link
Contributor

There are two linkers in the GNU binutils. They both support options -z common-page-size and -z max-page-size to set, at link time, the usual page size and the maximum page size. The default values for these are processor specific. These all feed into the algorithm that determines the layout of the segments in memory. Basically, each segment is aligned to the maximum page size. The linkers try to save a page on disk by offsetting the start of the data segment so that the last page of the text segment and the first page of the data segment can be on the same page of the executable (obviously that page gets mapped twice in memory, once executable, once writable). When trying to save this page, they use the usual page size, not the maximum page size.

On amd64, for gold, the default maximum and usual page size are both 0x10000. That is, gold does not support huge pages by default. For the GNU linker, the default maximum page size is The usual page size is 0x200000 and the default usual page size is 0x1000.

I'm sure you're glad you asked.

For these kinds of numbers, I agree that the amount of VM space wasted by large segment alignments is not very important (although there used to be an interesting bug in glibc when it was able to map a small shared library into the gap between the text and data segments of the executable). For Go, I think the linker should align the segments to the maximum page size that is really used on the processor. I don't see why this would make the binary bigger, at least I don't see why it would be bigger on ELF systems.

@rsc rsc removed the repo-main label Apr 14, 2015
@minux minux self-assigned this Apr 24, 2015
@rsc
Copy link
Contributor

rsc commented Jun 8, 2015

I'd strongly prefer not to put more autodetection in cmd/dist. Every step we take in that direction makes cross-compilation harder.

The main thing that needs to be done is that the linker needs to be made clearer about the difference between virtual alignment requirements and file offset alignment requirements. That is, the systems with 64 kB pages do not require the file data mapped into those pages to be 64 kB aligned within the file. They only require something smaller, probably 4 kB. With that distinction made, we can adjust the virtual address alignment upward without affecting binary size. We should do that.

Changing _PhysPageSize in the runtime and syscall.Getpagesize are both trivial, especially in comparison.

But it's now too late to be doing any of this for Go 1.5.

@rsc rsc modified the milestones: Unplanned, Go1.5 Jun 8, 2015
@rsc rsc changed the title build: better support for non-4K page size systems cmd/link: better support for non-4K page size systems Jun 8, 2015
@yonderblue
Copy link

With 1.4 we are changing arch_arm.h to set the page size and then passing into -R into ldflags, what will we need to do for 1.5 when on custom page sizes?

@ianlancetaylor
Copy link
Contributor

@Gaillard Please ask questions like that on the mailing list, not on an issue. Thanks. (I don't know the answer to your question--I'm not really sure what you are asking.)

@gopherbot
Copy link

CL https://golang.org/cl/14421 mentions this issue.

@aclements aclements modified the milestones: Go1.8, Unplanned Jul 19, 2016
gopherbot pushed a commit that referenced this issue Sep 6, 2016
Currently the physical page size assumed by the runtime is hard-coded.
On Linux the runtime at least fetches the OS page size during init and
sanity checks against the hard-coded value, but they may still differ.
On other OSes we wouldn't even notice.

Add support on all OSes to fetch the actual OS physical page size
during runtime init and lift the sanity check of PhysPageSize from the
Linux init code to general malloc init. Currently this is the only use
of the retrieved page size, but we'll add more shortly.

Updates #12480 and #10180.

Change-Id: I065f2834bc97c71d3208edc17fd990ec9058b6da
Reviewed-on: https://go-review.googlesource.com/25050
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot pushed a commit that referenced this issue Sep 6, 2016
Now that the runtime fetches the true physical page size from the OS,
make the physical page size used by heap growth a variable instead of
a constant. This isn't used in any performance-critical paths, so it
shouldn't be an issue.

sys.PhysPageSize is also renamed to sys.DefaultPhysPageSize to make it
clear that it's not necessarily the true page size. There are no uses
of this constant any more, but we'll keep it around for now.

Updates #12480 and #10180.

Change-Id: I6c23b9df860db309c38c8287a703c53817754f03
Reviewed-on: https://go-review.googlesource.com/25022
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
gopherbot pushed a commit that referenced this issue Sep 6, 2016
syscall.Getpagesize currently returns hard-coded page sizes on all
architectures (some of which are probably always wrong, and some of
which are definitely not always right). The runtime now has this
information, queried from the OS during runtime init, so make
syscall.Getpagesize return the page size that the runtime knows.

Updates #10180.

Change-Id: I4daa6fbc61a2193eb8fa9e7878960971205ac346
Reviewed-on: https://go-review.googlesource.com/25051
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 7, 2016
@quentinmit quentinmit assigned aclements and unassigned minux Oct 7, 2016
@aclements
Copy link
Member

quentinmit assigned aclements

The runtime should have no trouble with non-4K pages at this point. If there are any remaining issues, they're in the linker (IMO, they're really in the kernel's ELF loader).

I'm not quite sure who "owns" the linker at this point. I'm reassigning to David Crawshaw, but feel free to redirect as appropriate.

@vielmetti
Copy link

This issues affects the overlay2 filesystem on Docker, manifesting itself as moby/moby#27384

@aclements
Copy link
Member

This issues affects the overlay2 filesystem on Docker, manifesting itself as moby/moby#27384

If the assessment in the docker issue is right, this should be fixed on Go tip.

@rsc
Copy link
Contributor

rsc commented Oct 25, 2016

Per @aclements, the runtime is safe for larger page systems.

Looking at archinit in src/cmd/link/internal/*/obj.go, all the 64-bit non-x86 systems seem to already default FlagRound (formerly INITRND) to 64 kB.

So it sounds like both the runtime and the linker are fixed. I'm not worried about the binary size of an extra (average) 32 kB in this case due to rounding, so we can skip the INITRND VM tricks that Minux suggested in the original report.

I think we're all set on this issue. Closing. Reopen if I'm wrong, but please also explain why. Thanks.

@rsc rsc closed this as completed Oct 25, 2016
tmm1 pushed a commit to fancybits/go that referenced this issue Nov 9, 2016
Currently the physical page size assumed by the runtime is hard-coded.
On Linux the runtime at least fetches the OS page size during init and
sanity checks against the hard-coded value, but they may still differ.
On other OSes we wouldn't even notice.

Add support on all OSes to fetch the actual OS physical page size
during runtime init and lift the sanity check of PhysPageSize from the
Linux init code to general malloc init. Currently this is the only use
of the retrieved page size, but we'll add more shortly.

Updates golang#12480 and golang#10180.

Change-Id: I065f2834bc97c71d3208edc17fd990ec9058b6da
Reviewed-on: https://go-review.googlesource.com/25050
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
tmm1 pushed a commit to fancybits/go that referenced this issue Nov 9, 2016
syscall.Getpagesize currently returns hard-coded page sizes on all
architectures (some of which are probably always wrong, and some of
which are definitely not always right). The runtime now has this
information, queried from the OS during runtime init, so make
syscall.Getpagesize return the page size that the runtime knows.

Updates golang#10180.

Change-Id: I4daa6fbc61a2193eb8fa9e7878960971205ac346
Reviewed-on: https://go-review.googlesource.com/25051
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
tmm1 pushed a commit to fancybits/go that referenced this issue Nov 9, 2016
Now that the runtime fetches the true physical page size from the OS,
make the physical page size used by heap growth a variable instead of
a constant. This isn't used in any performance-critical paths, so it
shouldn't be an issue.

sys.PhysPageSize is also renamed to sys.DefaultPhysPageSize to make it
clear that it's not necessarily the true page size. There are no uses
of this constant any more, but we'll keep it around for now.

Updates golang#12480 and golang#10180.

Change-Id: I6c23b9df860db309c38c8287a703c53817754f03
Reviewed-on: https://go-review.googlesource.com/25022
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rick Hudson <rlh@golang.org>
@vielmetti
Copy link

Just confirming that a test build with Go 1.8 beta 1 on arm64 (armv8, aarch64) passes all tests.

@oshimaya
Copy link

Is this fix only for 64 bit?
The default pagesize is 8K in the current netbsd/arm (32bit, EABI).
So if the round size set by linker is 4K by default, this binary is not be loaded .
I'm very happy if the default value will be increased for netbsd/arm.
(The binaries of OS created by gnu-binutils is 64K)

@binarycrusader
Copy link
Contributor

I'd like to add that our almost-compete sparc64 port also has no issues once we backported the pagesize related fixes in 1.8 to 1.7. As such, I'd tend to guess that there are no general issues left with pagesize -- only platform-specific ones.

@vielmetti
Copy link

@binarycrusader - was the backport from 1.8 to 1.7 easy? (It looked like it might be.)

I've put together this tiny test at https://github.com/vielmetti/go-pagesize-test with the assistance of code from @bradfitz - hoping to backport this to aarch64 as well since the Ubuntu 16.04 LTS go for aarch64 is 1.6 vintage and exhibits this defect.

@tmm1
Copy link
Contributor

tmm1 commented Feb 2, 2017

The backport is quite straightforward with a few cherry-picks. I did it here: fancybits/go@release-branch.go1.7...arm-pagesize-64k

@gopherbot
Copy link

Change https://golang.org/cl/62111 mentions this issue: unix: defer Getpagesize() to runtime

gopherbot pushed a commit to golang/sys that referenced this issue Sep 7, 2017
In general, page size is not a function of the archetecture. This was
addressed in the Go standard library here:
https://go-review.googlesource.com/25051

This change simply defers to the standard library "syscall" package,
which in turn defers to the runtime. This helps in addressing golang/go#10180 and
also fixes a bug on ppc64.

Currently, we return 65536 as the page size on ppc64, but the kernel
supports 4k and 64k sizes, see here:
http://elixir.free-electrons.com/linux/v4.13/source/arch/powerpc/include/asm/page.h#L24

Now that various page size calculations are not needed, various
components are now dead code and can also be removed. This CL reverts:
https://go-review.googlesource.com/14483
and part of:
https://go-review.googlesource.com/30755

Change-Id: I9d7a2d96359054e0dca9c985b026c8072b2eeaf3
Reviewed-on: https://go-review.googlesource.com/62111
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
micanzhang pushed a commit to micanzhang/sys that referenced this issue Sep 12, 2017
In general, page size is not a function of the archetecture. This was
addressed in the Go standard library here:
https://go-review.googlesource.com/25051

This change simply defers to the standard library "syscall" package,
which in turn defers to the runtime. This helps in addressing golang/go#10180 and
also fixes a bug on ppc64.

Currently, we return 65536 as the page size on ppc64, but the kernel
supports 4k and 64k sizes, see here:
http://elixir.free-electrons.com/linux/v4.13/source/arch/powerpc/include/asm/page.h#L24

Now that various page size calculations are not needed, various
components are now dead code and can also be removed. This CL reverts:
https://go-review.googlesource.com/14483
and part of:
https://go-review.googlesource.com/30755

Change-Id: I9d7a2d96359054e0dca9c985b026c8072b2eeaf3
Reviewed-on: https://go-review.googlesource.com/62111
Reviewed-by: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Sep 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests