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/geth: use automaxprocs to apply cpu quota correctly #27506

Merged
merged 2 commits into from Jul 14, 2023

Conversation

jsvisa
Copy link
Contributor

@jsvisa jsvisa commented Jun 19, 2023

As discussed with @rjl493456442 offline, if Geth was running in a container with CPU quota(set with cgroup limiting), and runtime.NumCPU returns the host's CPU cores(maybe bigger than the docker's setting), which may result in more Golang runtime schedule and context switch.

So I'm using the uber/atomaxprocs package to return the correct cpu quota.

Signed-off-by: jsvisa <delweng@gmail.com>
Signed-off-by: jsvisa <delweng@gmail.com>
@fjl
Copy link
Contributor

fjl commented Jun 19, 2023

I've checked the dependency a bit, and it's kind of complicated for what it is. Would be nice to have a simpler solution.

@fjl
Copy link
Contributor

fjl commented Jun 19, 2023

We already have a package common/fdlimit that could be extended with a function to read the assigned CPU quota in Linux.

@jsvisa
Copy link
Contributor Author

jsvisa commented Jun 19, 2023

We already have a package common/fdlimit that could be extended with a function to read the assigned CPU quota in Linux.

thanks for the advice, I'll try to implement it in the package

@fjl
Copy link
Contributor

fjl commented Jun 19, 2023

Actually, after checking the automaxprocs implementation a bit more, I can say we probably don't want to reimplement this.

@jsvisa
Copy link
Contributor Author

jsvisa commented Jun 29, 2023

@fjl please take another look, is it ready to merge?

@fjl fjl changed the title cmd/geth: respect to container's cpu cores cmd/geth: use automaxprocs to apply cpu quota correctly Jul 14, 2023
@fjl fjl merged commit 47b9f1b into ethereum:master Jul 14, 2023
1 of 2 checks passed
@fjl fjl added this to the 1.12.1 milestone Jul 14, 2023
@jsvisa jsvisa deleted the eth-pebble-cpu branch August 15, 2023 18:51
MoonShiesty pushed a commit to MoonShiesty/go-ethereum that referenced this pull request Aug 30, 2023
It is usually best to set GOMAXPROCS to the number of available CPU cores. However, setting
it like that does not work well when the process is quota-limited to a certain number of CPUs.
The automaxprocs library configures GOMAXPROCS, taking such limits into account.
DarianShawn pushed a commit to dogechain-lab/dbsc that referenced this pull request Sep 3, 2023
### Description

upstream PR:
[go-ethereum#27506](ethereum/go-ethereum#27506)

As discussed with @rjl493456442 offline, if Geth was running in a
container with CPU quota(set with cgroup limiting), and runtime.NumCPU
returns the host's CPU cores(maybe bigger than the docker's setting),
which may result in more Golang runtime schedule and context switch.

So I'm using the
[uber/atomaxprocs](https://github.com/uber-go/automaxprocs) package to
return the correct cpu quota.
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
It is usually best to set GOMAXPROCS to the number of available CPU cores. However, setting
it like that does not work well when the process is quota-limited to a certain number of CPUs.
The automaxprocs library configures GOMAXPROCS, taking such limits into account.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants