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

runtime/metrics: update documentation w.r.t. stack_sys #54396

Closed
aktau opened this issue Aug 11, 2022 · 2 comments
Closed

runtime/metrics: update documentation w.r.t. stack_sys #54396

aktau opened this issue Aug 11, 2022 · 2 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@aktau
Copy link
Contributor

aktau commented Aug 11, 2022

What version of Go are you using (go version)?

$ go version 
go1.20

Issue

The documentation related to stack memory in MemStats says:

        // StackInuse is bytes in stack spans.
	//
	// In-use stack spans have at least one stack in them. These
	// spans can only be used for other stacks of the same size.
	//
	// There is no StackIdle because unused stack spans are
	// returned to the heap (and hence counted toward HeapIdle).
	StackInuse [uint64](https://pkg.go.dev/builtin#uint64)

	// StackSys is bytes of stack memory obtained from the OS.
	//
	// StackSys is StackInuse, plus any memory obtained directly
	// from the OS for OS thread stacks (which should be minimal).
	StackSys [uint64](https://pkg.go.dev/builtin#uint64)

And, in runtime/metrics, there is also:

/memory/classes/heap/stacks:bytes
	Memory allocated from the heap that is reserved for stack
	space, whether or not it is currently in-use.

/memory/classes/os-stacks:bytes
	Stack memory allocated by the underlying operating system.

But this is at best confusing and at worst incorrect. Quoting @mknyszek from a recent conversation:

if a program is pure Go, we use newosproc0 to allocate the first stack. this accounts to stacks_sys, which funnels into StackSys. all other system stacks are allocated from the same pool as goroutine stacks (basically, StackInuse). 8192 bytes.
if a program is cgo, then we use _cgo_sys_thread_create for the first thread which is the platform C library thread creation. we use this for every new stack, too. we don't control the size of those stacks at all, and we also don't account for them AFAICT. this is how we ensure that calls into C are fine: all system stacks are basically provided by pthreads.

This means:

  1. The behaviour is different between pure Go and cgo programs, which should be mentioned.
  2. Even for pure Go programs, stacks for syscalls are generally allocated from stack_inuse (in cgo programs this is not tracked at all).

This different behaviour should be called out in the docs.

@gopherbot gopherbot added compiler/runtime Issues related to the Go compiler and/or runtime. Documentation labels Aug 11, 2022
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 11, 2022
@thanm
Copy link
Contributor

thanm commented Aug 11, 2022

@golang/runtime

@mknyszek mknyszek added this to the Go1.20 milestone Aug 17, 2022
@mknyszek mknyszek self-assigned this Aug 17, 2022
@gopherbot gopherbot modified the milestones: Go1.20, Go1.21 Feb 1, 2023
@gopherbot
Copy link

Change https://go.dev/cl/502156 mentions this issue: runtime,runtime/metrics: clarify OS stack metrics

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 19, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
There are some subtle details here about measuring OS stacks in cgo
programs. There's also an expectation about magnitude in the MemStats
docs that isn't in the runtime/metrics docs. Fix both.

Fixes golang#54396.

Change-Id: I6b60a62a4a304e6688e7ab4d511d66193fc25321
Reviewed-on: https://go-review.googlesource.com/c/go/+/502156
Run-TryBot: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Documentation NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants