-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
Detecting the page size inside cmd/dist sounds reasonable. Dumb question time, what are the downsides of just assuming 64k pages
|
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. |
On 17 Mar 2015 09:17, "Keith Randall" notifications@github.com wrote:
That was the one location that I saw that would be a performance concern if We can get the system page size at startup from auxv or a syscall.
I'd like that to be the case.
|
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? |
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>
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. |
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. |
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? |
@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.) |
CL https://golang.org/cl/14421 mentions this issue. |
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>
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>
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>
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. |
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. |
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. |
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>
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>
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>
Just confirming that a test build with Go 1.8 beta 1 on arm64 (armv8, aarch64) passes all tests. |
Is this fix only for 64 bit? |
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. |
@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. |
The backport is quite straightforward with a few cherry-picks. I did it here: fancybits/go@release-branch.go1.7...arm-pagesize-64k |
Change https://golang.org/cl/62111 mentions this issue: |
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>
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>
At least three problems here:
1
. cmd/ld INITRND needs to be at least as large as the page size, otherwisewe 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'sload address start at
(N+1)*INITRND + x + 1
, so there is no hole in the file, butdifferent segments end up on different pages (if
INITRND >= page size
).2
. _PhysPageSize in runtime needs to be a variable and auto detectedat 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.
The text was updated successfully, but these errors were encountered: