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

Jump to bottom

runtime: modified timer results in extreme cpu load [1.18 backport] #53847

Closed
gopherbot opened this issue Jul 13, 2022 · 3 comments
Closed

runtime: modified timer results in extreme cpu load [1.18 backport] #53847

gopherbot opened this issue Jul 13, 2022 · 3 comments
Assignees
@prattmic
Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Milestone
Go1.18.5

Comments

@gopherbot
Copy link

gopherbot commented Jul 13, 2022

@prattmic requested issue #51654 to be considered for backport to the next 1.18 minor release.

@gopherbot Please backport to 1.17 and 1.18. This is a serious issue (busy loop in scheduler for up to 2 minutes) with no reasonable workaround.
@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Jul 13, 2022
@gopherbot gopherbot mentioned this issue Jul 13, 2022
Closed
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
@gopherbot gopherbot added this to the Go1.18.5 milestone Jul 13, 2022
@gopherbot
Copy link
Author

gopherbot commented Jul 13, 2022

Change https://go.dev/cl/417475 mentions this issue: [release-branch.go1.18] runtime: clear timerModifiedEarliest when last timer is deleted

bradfitz pushed a commit to tailscale/go that referenced this issue Jul 14, 2022
@prattmic @bradfitz
[release-branch.go1.18] runtime: clear timerModifiedEarliest when las…
5161ac3 
…t timer is deleted

timerModifiedEarliest contains the lowest possible expiration for a
modified earlier timer, which may be earlier than timer0When because we
haven't yet updated the heap. Note "may", as the modified earlier timer
that set timerModifiedEarliest may have since been modified later or
deleted.

We can clear timerModifiedEarliest when the last timer is deleted
because by definition there must not be any modified earlier timers.

Why does this matter? checkTimersNoP claims that there is work to do if
timerModifiedEarliest has passed, causing findRunnable to loop back
around to checkTimers. But the code to clean up timerModifiedEarliest in
checkTimers (i.e., the call to adjusttimers) is conditional behind a
check that len(pp.timers) > 0.

Without clearing timerModifiedEarliest, a spinning M that would
otherwise go to sleep will busy loop in findRunnable until some other
work is available.

Note that changing the condition on the call to adjusttimers would also
be a valid fix. I took this approach because it feels a bit cleaner to
clean up timerModifiedEarliest as soon as it is known to be irrelevant.

For golang#51654.
Fixes golang#53847.

Change-Id: I3f3787c67781cac7ce87939c5706cef8db927dd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/417434
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit c006b7a)
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 14, 2022
@prattmic @bradfitz
[release-branch.go1.18] runtime: clear timerModifiedEarliest when las…
149f7d8 
…t timer is deleted

timerModifiedEarliest contains the lowest possible expiration for a
modified earlier timer, which may be earlier than timer0When because we
haven't yet updated the heap. Note "may", as the modified earlier timer
that set timerModifiedEarliest may have since been modified later or
deleted.

We can clear timerModifiedEarliest when the last timer is deleted
because by definition there must not be any modified earlier timers.

Why does this matter? checkTimersNoP claims that there is work to do if
timerModifiedEarliest has passed, causing findRunnable to loop back
around to checkTimers. But the code to clean up timerModifiedEarliest in
checkTimers (i.e., the call to adjusttimers) is conditional behind a
check that len(pp.timers) > 0.

Without clearing timerModifiedEarliest, a spinning M that would
otherwise go to sleep will busy loop in findRunnable until some other
work is available.

Note that changing the condition on the call to adjusttimers would also
be a valid fix. I took this approach because it feels a bit cleaner to
clean up timerModifiedEarliest as soon as it is known to be irrelevant.

For golang#51654.
Fixes golang#53847.

Change-Id: I3f3787c67781cac7ce87939c5706cef8db927dd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/417434
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit c006b7a)
@toothrot toothrot added the CherryPickApproved Used during the release process for point releases label Jul 20, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Jul 20, 2022
@toothrot
Copy link
Contributor

toothrot commented Jul 20, 2022

Approved. This is a serious issue with no workaround.

@gopherbot
Copy link
Author

gopherbot commented Jul 25, 2022

Closed by merging 4782f42 to release-branch.go1.18.

gopherbot pushed a commit that referenced this issue Jul 25, 2022
@prattmic @cherrymui
[release-branch.go1.18] runtime: clear timerModifiedEarliest when las…
4782f42 
…t timer is deleted

timerModifiedEarliest contains the lowest possible expiration for a
modified earlier timer, which may be earlier than timer0When because we
haven't yet updated the heap. Note "may", as the modified earlier timer
that set timerModifiedEarliest may have since been modified later or
deleted.

We can clear timerModifiedEarliest when the last timer is deleted
because by definition there must not be any modified earlier timers.

Why does this matter? checkTimersNoP claims that there is work to do if
timerModifiedEarliest has passed, causing findRunnable to loop back
around to checkTimers. But the code to clean up timerModifiedEarliest in
checkTimers (i.e., the call to adjusttimers) is conditional behind a
check that len(pp.timers) > 0.

Without clearing timerModifiedEarliest, a spinning M that would
otherwise go to sleep will busy loop in findRunnable until some other
work is available.

Note that changing the condition on the call to adjusttimers would also
be a valid fix. I took this approach because it feels a bit cleaner to
clean up timerModifiedEarliest as soon as it is known to be irrelevant.

For #51654.
Fixes #53847.

Change-Id: I3f3787c67781cac7ce87939c5706cef8db927dd5
Reviewed-on: https://go-review.googlesource.com/c/go/+/417434
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
(cherry picked from commit c006b7a)
Reviewed-on: https://go-review.googlesource.com/c/go/+/417475
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

Labels
CherryPickApproved Used during the release process for point releases compiler/runtime Issues related to the Go compiler and/or runtime.
Projects
Go Compiler / Runtime
Status: Done
Milestone
Go1.18.5
Development

No branches or pull requests
3 participants
@toothrot @prattmic @gopherbot