This change resolves a TODO in the coroutine switch implementation (used
exclusively by iter.Pull at the moment) to enable tracing. This was
blocked on eliminating the atomic load in the tracer's "off" path
(completed in the previous CL in this series) and the addition of new
tracer events to minimize the overhead of tracing in this circumstance.
This change introduces 3 new event types to support coroutine switches:
GoCreateBlocked, GoSwitch, and GoSwitchDestroy.
GoCreateBlocked needs to be introduced because the goroutine created for
the coroutine starts out in a blocked state. There's no way to represent
this in the tracer right now, so we need a new event for it.
GoSwitch represents the actual coroutine switch, which conceptually
consists of a GoUnblock, a GoBlock, and a GoStart event in series
(unblocking the next goroutine to run, blocking the current goroutine,
and then starting the next goroutine to run).
GoSwitchDestroy is closely related to GoSwitch, implementing the same
semantics except that GoBlock is replaced with GoDestroy. This is used
when exiting the coroutine.
The implementation of all this is fairly straightforward, and the trace
parser simply translates GoSwitch* into the three constituent events.
Because GoSwitch and GoSwitchDestroy imply a GoUnblock and a GoStart,
they need to synchronize with other past and future GoStart events to
create a correct partial ordering in the trace. Therefore, these events
need a sequence number for the goroutine that will be unblocked and
started.
Also, while implementing this, I noticed that the coroutine
implementation is actually buggy with respect to LockOSThread. In fact,
it blatantly disregards its invariants without an explicit panic. While
such a case is likely to be rare (and inefficient!) we should decide how
iter.Pull behaves with respect to runtime.LockOSThread.
Lastly, this change also bumps the trace version from Go 1.22 to Go
1.23. We're adding events that are incompatible with a Go 1.22 parser,
but Go 1.22 traces are all valid Go 1.23 traces, so the newer parser
supports both (and the CL otherwise updates the Go 1.22 definitions of
events and such). We may want to reconsider the structure and naming of
some of these packages though; it could quickly get confusing.
For #61897.
Change-Id: I96897a46d5852c02691cde9f957dc6c13ef4d8e7
Reviewed-on: https://go-review.googlesource.com/c/go/+/565937
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Converting *void directly to mach_port_t causes newer clang to throw a
void-pointer-to-int-cast warning/error.
Change-Id: I709955d4678bed3f690a8337ce85fd8678d217bb
Reviewed-on: https://go-review.googlesource.com/c/go/+/573415
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Remove dynamic checks for atomic instructions for ARM64 targets that support LSE extension.
For #66131
Change-Id: I0ec1b183a3f4ea4c8a537430646e6bc4b4f64271
Reviewed-on: https://go-review.googlesource.com/c/go/+/569536
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Fannie Zhang <Fannie.Zhang@arm.com>
Reviewed-by: Shu-Chun Weng <scw@google.com>
The LockThreadExit tests in the runtime have been observed to fail after
reading /proc/self/task/<tid>/stat and blindly assuming its contents
followed a specific format. The parsing code is also wrong, because
splitting by spaces doesn't work when the comm name contains a space.
It also ignores errors without reporting them, which isn't great.
This change rewrites tidExists to be more robust by using
/proc/self/task/<tid>/status instead. It also modifies tidExists'
signature to report an error to its caller. Its caller then prints that
error.
Ignoring a non-not-exist error with opening this file is the likely but
unconfirmed cause of #65736 (ESRCH). This change also checks for that
error explicitly as an optimistic fix.
Fixes#65736.
Change-Id: Iea560b457d514426da2781b7eb7b8616a91ec23b
Reviewed-on: https://go-review.googlesource.com/c/go/+/567938
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
The RtlGetNtVersionNumbers function is not documented by Microsoft.
Use RtlGetVersion instead, which is documented and available on all
supported versions of Windows.
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64
Change-Id: Ibaf0e2c28e673951476c5d863a829fd166705aea
Reviewed-on: https://go-review.googlesource.com/c/go/+/571015
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
On Linux, both mprotect() and mmap() acquire the mmap_lock (in writer mode),
posing scalability challenges.
The mmap_lock (formerly called mmap_sem) is a reader/writer lock that controls
access to a process's address space; before making changes there (mapping in a
new range, for example), the kernel must acquire that lock.
Page-fault handling must also acquire mmap_lock (in reader mode) to ensure that
the address space doesn't change in surprising ways while a fault is being resolved.
A process can have a large address space and many threads running (and incurring
page faults) concurrently, turning mmap_lock into a significant bottleneck.
While both mmap() and mprotect() are protected by the mmap_lock, the shorter
duration of mprotect system call, due to their simpler nature, results in a reduced
locking time for the mmap_lock.
Change-Id: I7f929544904e31eab34d0d8a9e368abe4de64637
GitHub-Last-Rev: 6f27a216b4
GitHub-Pull-Request: golang/go#65038
Reviewed-on: https://go-review.googlesource.com/c/go/+/554935
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Some of the Windows syscall helpers don't need to be nosplit. Removing
this directive will allow to add instrumentation to these functions
without having to worry about the stack size.
Change-Id: I3885621f23733af48563803c704563474010b8d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/572415
Reviewed-by: Than McIntosh <thanm@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Minor cleanups to CL 561635's test for better debuggability
when it crashes. In a separate CL so that it's clear this CL is
not changing the code under test.
Change-Id: I12b72ae538f8454b5c382127eafd766c22c69b67
Reviewed-on: https://go-review.googlesource.com/c/go/+/571799
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This makes CL 561635's test pass without any changes to the
traceback textual format.
The test in this CL is copied identically from CL 561635.
Change-Id: I5130abdfefd9940f98f20c283cca6cd159e37617
Reviewed-on: https://go-review.googlesource.com/c/go/+/571798
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
This reverts commit 643d816c8b (CL 561635).
Reason for revert: This works for telemetry but broke various other
properties of the tracebacks as well as some programs that read
tracebacks. We should figure out a solution that works for all uses,
and in the interim we should not be making telemetry work at the
cost of breaking other, existing valid uses.
See #65761 for details.
Change-Id: I467993ae778887e5bd3cca4c0fb54e9d44802ee1
Reviewed-on: https://go-review.googlesource.com/c/go/+/571797
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
A proposal discussion in mid-2020 on #37196 decided to change
time.Timer and time.Ticker so that their Stop and Reset methods
guarantee that no old value (corresponding to the previous configuration
of the Timer or Ticker) will be received after the method returns.
The trivial way to do this is to make the Timer/Ticker channels
unbuffered, create a goroutine per Timer/Ticker feeding the channel,
and then coordinate with that goroutine during Stop/Reset.
Since Stop/Reset coordinate with the goroutine and the channel
is unbuffered, there is no possibility of a stale value being sent
after Stop/Reset returns.
Of course, we do not want an extra goroutine per Timer/Ticker,
but that's still a good semantic model: behave like the channels
are unbuffered and fed by a coordinating goroutine.
The actual implementation is more effort but behaves like the model.
Specifically, the timer channel has a 1-element buffer like it always has,
but len(t.C) and cap(t.C) are special-cased to return 0 anyway, so user
code cannot see what's in the buffer except with a receive.
Stop/Reset lock out any stale sends and then clear any pending send
from the buffer.
Some programs will change behavior. For example:
package main
import "time"
func main() {
t := time.NewTimer(2 * time.Second)
time.Sleep(3 * time.Second)
if t.Reset(2*time.Second) != false {
panic("expected timer to have fired")
}
<-t.C
<-t.C
}
This program (from #11513) sleeps 3s after setting a 2s timer,
resets the timer, and expects Reset to return false: the Reset is too
late and the send has already occurred. It then expects to receive
two values: the one from before the Reset, and the one from after
the Reset.
With an unbuffered timer channel, it should be clear that no value
can be sent during the time.Sleep, so the time.Reset returns true,
indicating that the Reset stopped the timer from going off.
Then there is only one value to receive from t.C: the one from after the Reset.
In 2015, I used the above example as an argument against this change.
Note that a correct version of the program would be:
func main() {
t := time.NewTimer(2 * time.Second)
time.Sleep(3 * time.Second)
if !t.Reset(2*time.Second) {
<-t.C
}
<-t.C
}
This works with either semantics, by heeding t.Reset's result.
The change should not affect correct programs.
However, one way that the change would be visible is when programs
use len(t.C) (instead of a non-blocking receive) to poll whether the timer
has triggered already. We might legitimately worry about breaking such
programs.
In 2020, discussing #37196, Bryan Mills and I surveyed programs using
len on timer channels. These are exceedingly rare to start with; nearly all
the uses are buggy; and all the buggy programs would be fixed by the new
semantics. The details are at [1].
To further reduce the impact of this change, this CL adds a temporary
GODEBUG setting, which we didn't know about yet in 2015 and 2020.
Specifically, asynctimerchan=1 disables the change and is the default
for main programs in modules that use a Go version before 1.23.
We hope to be able to retire this setting after the minimum 2-year window.
Setting asynctimerchan=1 also disables the garbage collection change
from CL 568341, although users shouldn't need to know that since
it is not a semantically visible change (unless we have bugs!).
As an undocumented bonus that we do not officially support,
asynctimerchan=2 disables the channel buffer change but keeps
the garbage collection change. This may help while we are
shaking out bugs in either of them.
Fixes#37196.
[1] https://github.com/golang/go/issues/37196#issuecomment-641698749
Change-Id: I8925d3fb2b86b2ae87fd2acd055011cbf7bd5916
Reviewed-on: https://go-review.googlesource.com/c/go/+/568341
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Classic operating system kernel mistake: if you start using
per-CPU data without disabling interrupts on the CPU,
and then an interrupt reschedules the process onto a different
CPU, now you're using the wrong CPU's per-CPU data.
The same thing happens in Go if you use per-M or per-P
data structures while not holding a lock nor using acquirem.
In the original timer.modify before CL 564977, I had been
very careful about this during the "unlock t; lock ts" dance,
only calling releasem after ts was locked. That made sure
we used the right ts. The refactoring of that code into its
own helper function in CL 564977 missed that nuance.
The code
ts := &getg().m.p.p.ptr().timers
ts.lock()
was now executing without holding any locks nor acquirem.
If the goroutine changed its M or P between deciding which
ts to use and actually locking that ts, the code would proceed
to add the timer t to some other P's timers. If the P was idle
by then, the scheduler could have already checked it for timers
and not notice the newly added timer when deciding when the
next timer should trigger.
The solution is to do what the old code correctly did, namely
acquirem before deciding which ts to use, rather than assume
getg().m.p won't change before ts.lock can complete.
This CL does that.
Before CL 564977,
stress ./time.test -test.run='ZeroTimer/impl=(func|cache)' -test.timeout=3m -test.count=20
ran without failure for over an hour on my laptop.
Starting in CL 564977, it consistently failed within a few minutes.
After this CL, it now runs without failure for over an hour again.
Fixes#66006.
Change-Id: Ib9e7ccaa0f22a326ce3fdef2b9a92f7f0bdafcbf
Reviewed-on: https://go-review.googlesource.com/c/go/+/571196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
From the beginning of Go, the time package has had a gotcha:
if you use a select on <-time.After(1*time.Minute), even if the select
finishes immediately because some other case is ready, the underlying
timer from time.After keeps running until the minute is over. This
pins the timer in the timer heap, which keeps it from being garbage
collected and in extreme cases also slows down timer operations.
The lack of garbage collection is the more important problem.
The docs for After warn against this scenario and suggest using
NewTimer with a call to Stop after the select instead, purely to work
around this garbage collection problem.
Oddly, the docs for NewTimer and NewTicker do not mention this
problem, but they have the same issue: they cannot be collected until
either they are Stopped or, in the case of Timer, the timer expires.
(Tickers repeat, so they never expire.) People have built up a shared
knowledge that timers and tickers need to defer t.Stop even though the
docs do not mention this (it is somewhat implied by the After docs).
This CL fixes the garbage collection problem, so that a timer that is
unreferenced can be GC'ed immediately, even if it is still running.
The approach is to only insert the timer into the heap when some
channel operation is blocked on it; the last channel operation to stop
using the timer takes it back out of the heap. When a timer's channel
is no longer referenced, there are no channel operations blocked on
it, so it's not in the heap, so it can be GC'ed immediately.
This CL adds an undocumented GODEBUG asynctimerchan=1
that will disable the change. The documentation happens in
the CL 568341.
Fixes#8898.
Fixes#61542.
Change-Id: Ieb303b6de1fb3527d3256135151a9e983f3c27e6
Reviewed-on: https://go-review.googlesource.com/c/go/+/512355
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Fixes#66215
Change-Id: Id7de15feabe08f66c048dc114c09494813c9febc
Reviewed-on: https://go-review.googlesource.com/c/go/+/570695
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Toggled by a compile-time const, so there should be no
runtime footprint in ordinary builds.
Change-Id: I7751847524f4fda3853388d3e5a18188bd737c27
Reviewed-on: https://go-review.googlesource.com/c/go/+/570336
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
The timers had evolved to the point where the state was stored as follows:
if timer in heap:
state has timerHeaped set
if heap timer is stale:
heap deadline in t.when
real deadline in t.nextWhen
state has timerNextWhen set
else:
real deadline in t.when
t.nextWhen unset
else:
real deadline in t.when
t.nextWhen unset
That made it hard to find the real deadline and just hard to think about everything.
The new state is:
real deadline in t.when (always)
if timer in heap:
state has timerHeaped set
heap deadline in t.whenHeap
if heap timer is stale:
state has timerModified set
Separately, the 'state' word itself was being used as a lock
and state bits because the code started with CAS loops,
which we abstracted into the lock/unlock methods step by step.
At this point, we can switch to a real lock, making sure to
publish the one boolean needed by timers fast paths
at each unlock.
All this simplifies various logic considerably.
Change-Id: I35766204f7a26d999206bd56cc0db60ad1b17cbe
Reviewed-on: https://go-review.googlesource.com/c/go/+/570335
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8753622336585847105/+/u/step/11/log/2
shows a staticlockranking crash with pollcache (defaulted to LEAF)
being held during a write barrier, which got unlucky and acquired
wbufSpans, triggering a lock ordering throw.
My change in https://go-review.googlesource.com/c/go/+/570335/13/src/runtime/netpoll.go
around line 700 caused batching of many write barriers on the first
call rather than having just a few write barriers on each call,
making the crash much more likely, but the ordering problem
appears to have always existed. We just never allocated enough
pollDescs to trigger it.
Change-Id: Icb5e8340a5027dd4f7535a5ef02b2868476539e5
Reviewed-on: https://go-review.googlesource.com/c/go/+/571195
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
A few recent godebugs are missing IncNonDefault uses.
Test for that, so that people remember to do it.
Filed bugs for the missing ones.
For #66215.
For #66216.
For #66217.
Change-Id: Ia3fd10fd108e1b003bb30a8bc2f83995c768fab6
Reviewed-on: https://go-review.googlesource.com/c/go/+/570275
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
If user code has two timers t1 and t2 and does *t1 = *t2
(or *t1 = Timer{}), it creeps me out that we would be
corrupting the runtime data structures inlined in the
Timer struct. Replace that field with a pointer to the
runtime data structure instead, so that the corruption
cannot happen, even in a badly behaved program.
In fact, remove the struct definition entirely and linkname
a constructor instead. Now the runtime can evolve the struct
however it likes without needing to keep package time in sync.
Also move the workaround logic for #21874 out of
runtime and into package time.
Change-Id: Ia30f7802ee7b3a11f5d8a78dd30fd9c8633dc787
Reviewed-on: https://go-review.googlesource.com/c/go/+/568339
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
An upcoming CL will give this call more to do.
For now, separate out the compiler change that
stops inlining the computation.
For #37196.
Change-Id: I965426d446964b9b4958e4613246002a7660e7eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/568375
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Ticker.Reset was added in CL 217362 in 2020.
It added the runtime helper modTimer, which is
analogous to startTimer and resetTimer but for tickers.
Unlike those, it does not contain a racerelease, which
means that code synchronizing by starting a ticker
will be diagnosed with a spurious race.
Add racerelease to modTimer and add tests of all
three racereleases (in startTimer, resetTimer, and modTimer).
Also do not call time.resetTimer from elsewhere in runtime,
since that function is only for package time. Use t.reset instead.
For #33184.
Change-Id: Ie40c1ad24911f21e81b1d3cc608cf086ff2bc83d
Reviewed-on: https://go-review.googlesource.com/c/go/+/568340
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
allp < timers has not been necessary since CL 258303.
sched < timers was implied by allp < timers, and that
was still necessary, but only when the world is stopped.
Rewrite the code to avoid that lock since the world is stopped.
Now timers and timer are independent of the scheduler,
so they could call into the scheduler (for example to ready
a goroutine) if we wanted them to.
Change-Id: I12a93013c98e51c9e2f2148175b02afce8384a59
Reviewed-on: https://go-review.googlesource.com/c/go/+/568337
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Before CL 564118, there were two ways to add a new timer:
addtimer or modtimer. Much code was duplicated between them
and it was always valid to call modtimer instead of addtimer
(but not vice versa), so that CL changed all addtimer call sites
to use modtimer and deleted addtimer.
One thing that was unique to addtimer, however, was that it
called cleantimers (now named ts.cleanHead) after locking the
timers, while modtimer did not. This was the only difference
in the duplicated code, and I missed it. Restore the call to
ts.cleanHead when adding a new timer.
Also fix double-unlock in cleanHead.
Change-Id: I26cc50d650f31f977c0c31195cd013244883dba9
Reviewed-on: https://go-review.googlesource.com/c/go/+/568338
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
The comment in updateTimerPMask is wrong. It says:
// Looks like there are no timers, however another P
// may be adding one at this very moment.
// Take the lock to synchronize.
This was my incorrect simplification of the original comment
from CL 264477 when I was renaming all the things it mentioned:
// Looks like there are no timers, however another P may transiently
// decrement numTimers when handling a timerModified timer in
// checkTimers. We must take timersLock to serialize with these changes.
updateTimerPMask is being called by pidleput, so the P in question
is not in use. And other P's cannot add to this P.
As the original comment more precisely noted, the problem was
that other P's might be calling timers.check, which updates ts.len
occasionally while ts is locked, and one of those updates might
"leak" an ephemeral len==0 even when the heap is not going to
be empty when the P is finally unlocked. The lock/unlock in
updateTimerPMask synchronizes to avoid that. But this defeats
most of the purpose of using ts.len in the first place.
Instead of requiring that synchronization, we can arrange that
ts.len only ever shows a "publishable" length, meaning the len(ts.heap)
we leave behind during ts.unlock.
Having done that, updateTimerPMask can be inlined into pidleput.
The big comment on updateTimerPMask explaining how timerpMask
works is better placed as the doc comment for timerpMask itself,
so move it there.
Change-Id: I5442c9bb7f1473b5fd37c43165429d087012e73f
Reviewed-on: https://go-review.googlesource.com/c/go/+/568336
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
No semantic changes here.
Cleaning up for next change.
Change-Id: I9706009739677ff9eb893bcc007d805f7877511e
Reviewed-on: https://go-review.googlesource.com/c/go/+/568335
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The timer zombie count was fundamentally racy and worked around
in CL 569995. We worked around that by ignoring underflow.
The fundamnental race was because t.ts was set before t was
inserted into ts. CL 564997 corrected that fundamental problem,
so now we can account for zombies completely accurately,
never seeing values less than zero. Do that.
Change-Id: Idfbccc6662af5935f29f2a06a35e8ea93929bed7
Reviewed-on: https://go-review.googlesource.com/c/go/+/569996
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
Auto-Submit: Russ Cox <rsc@golang.org>
timers.wakeTime, which is called concurrently by P's trying to decide
how long they should sleep, can return inaccurate values while
timers.adjust is running. (Before the refactoring, this was still true
but the code did not have good names and was spread across more
files, making the race harder to see.)
The runtime thread sleeping code is complex enough that I am not
confident that the inaccuracy can cause delayed timer wakeups,
but I am also not confident that it can't, nor that it won't in the future.
There are two parts to the fix:
1. A simple logic change in timers.adjust.
2. The introduction of t.maybeAdd to avoid having a t that is
marked as belonging to a specific timers ts but not present
in ts.heap. That was okay before when everything was racy
but needs to be eliminated to make timers.adjust fully consistent.
The cost of the change is an extra CAS-lock operation on a timer add
(close to free since the CAS-lock was just unlocked) and a change
in the static lock ranking to allow malloc while holding a timer lock.
Change-Id: I1249e6e24ae9ef74a69837f453e15b513f0d75c0
Reviewed-on: https://go-review.googlesource.com/c/go/+/564977
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
Writes to timers.len are protected by the timers.lock.
There is no need to use an Add instead of a Store,
and the code is clearer (and perhaps slightly faster)
using the Store.
Change-Id: Icc6caef1b7405adec55c9b55b999b71de7d97484
Reviewed-on: https://go-review.googlesource.com/c/go/+/564976
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
These names were copied over from the p field names,
but now that they are part of the timers type they can use
shorter names that make the relationship clearer.
timer0When -> minWhen
timerModifiedEarliest -> minNextWhen
This code change is only the renaming.
Change-Id: I1c0adc0b3a1289d35639619d5c945585b2d81a9f
Reviewed-on: https://go-review.googlesource.com/c/go/+/564975
Auto-Submit: Russ Cox <rsc@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Austin Clements <austin@google.com>
The value produced for the EvFrequency event on Windows is missing the
fact that the cputicks clock gets divided. This results in durations
that are consistently wrong by the same factor (about 256).
Fixes#65997.
Change-Id: I930cbfce3499d435c20699f41c11e3227d84f911
Reviewed-on: https://go-review.googlesource.com/c/go/+/567937
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
Right now, we're careful to clean up dead P state when we advance to
future trace generations. If we don't, then if that P comes back to
life, we might end up using its old stale trace state.
Unfortunately, we never handled this in the case when tracing stops,
only when advancing to new generations. As a result, stopping a trace,
starting it again, and then bringing a P back to life in the following
generation meant that the dead P could be using stale state.
Fixes#65318.
Change-Id: I9297d9e58a254f2be933b8007a6ef7c5ec3ef4f0
Reviewed-on: https://go-review.googlesource.com/c/go/+/567077
Reviewed-by: Michael Pratt <mpratt@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Tracing is currently broken when using iter.Pull from the rangefunc
experiment partly because the "tracing is off" fast path in traceAcquire
was deemed too expensive to check (an atomic load) during the coroutine
switch.
This change adds trace.enabled, a non-atomic indicator of whether
tracing is enabled. It doubles trace.gen, which is the source of truth
on whether tracing is enabled. The semantics around trace.enabled are
subtle.
When tracing is enabled, we need to be careful to make sure that if gen
!= 0, goroutines enter the tracer on traceAcquire. This is enforced by
making sure trace.enabled is published atomically with trace.gen. The
STW takes care of synchronization with most Ms, but there's still sysmon
and goroutines exiting syscalls. We need to synchronize with those
explicitly anyway, which luckily takes care of trace.enabled as well.
When tracing is disabled, it's always OK for trace.enabled to be stale,
since traceAcquire will always double-check gen before proceeding.
For #61897.
Change-Id: I47c2a530fb5339c15e419312fbb1e22d782cd453
Reviewed-on: https://go-review.googlesource.com/c/go/+/565935
Auto-Submit: Michael Knyszek <mknyszek@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
The current implementation sets t.ts before adding t to ts;
that can cause inconsistencies with temporarily negative
ts.zombies values. Handle them gracefully, since we only
care about detecting very positive values.
Pending CL 564977 removes the race that sets t.ts early,
and then CL 569996 builds on top of that to make the count precise.
This CL just gets examples like the new test working sooner.
Change-Id: Ibe1aecc2554f83436f761f48e4050bd962982e4f
Reviewed-on: https://go-review.googlesource.com/c/go/+/569995
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL updates os.Readlink so it no longer tries to normalize volumes
to drive letters, which was not always even possible.
This behavior is controlled by the `winreadlinkvolume` setting.
For Go 1.23, it defaults to `winreadlinkvolume=1`.
Previous versions default to `winreadlinkvolume=0`.
Fixes#63703.
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64
Change-Id: Icd6fabbc8f0b78e23a82eef8db89940e89e9222d
Reviewed-on: https://go-review.googlesource.com/c/go/+/567735
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
This CL changes the behavior of os.Lstat to stop setting the
os.ModeSymlink type mode bit for mount points on Windows. As a result,
filepath.EvalSymlinks no longer evaluates mount points, which was the
cause of many inconsistencies and bugs.
Additionally, os.Lstat starts setting the os.ModeIrregular type mode bit
for all reparse tags on Windows, except for those that are explicitly
supported by the os package, which, since this CL, doesn't include mount
points. This helps to identify files that need special handling outside
of the os package.
This behavior is controlled by the `winsymlink` GODEBUG setting.
For Go 1.23, it defaults to `winsymlink=1`.
Previous versions default to `winsymlink=0`.
Fixes#39786Fixes#40176Fixes#61893
Updates #63703
Updates #40180
Updates #63429
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest,gotip-windows-arm64
Change-Id: I2e7372ab8862f5062667d30db6958d972bce5407
Reviewed-on: https://go-review.googlesource.com/c/go/+/565136
Reviewed-by: Bryan Mills <bcmills@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Continuing conversion from C to Go, introduce type timers
encapsulating all timer heap state, with methods for operations.
This should at least be easier to think about, instead of having
these fields strewn through the P struct. It should also be easier
to test.
I am skeptical about the pair of atomic int64 deadlines:
I think there are missed wakeups lurking.
Having the code in an abstracted API should make it easier
to reason through and fix if needed.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: If5ea3e0b946ca14076f44c85cbb4feb9eddb4f95
Reviewed-on: https://go-review.googlesource.com/c/go/+/564132
Reviewed-by: Austin Clements <austin@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Russ Cox <rsc@golang.org>
The itab we're initializing again, just to figure out which method
is missing, might be stored in read-only memory.
This can only happen in certain weird generics situations, so it is
pretty rare, but it causes a runtime crash when it does happen.
Fixes#65962
Change-Id: Ia86e216fe33950a794ad8e475e76317f799e9136
Reviewed-on: https://go-review.googlesource.com/c/go/+/567615
Reviewed-by: Cherry Mui <cherryyz@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@google.com>
For #65290Fixes#65971
Change-Id: If15853f287e06b85bb1cb038b3785516d5812f84
Reviewed-on: https://go-review.googlesource.com/c/go/+/567556
Auto-Submit: Ian Lance Taylor <iant@google.com>
Commit-Queue: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Mauri de Souza Meneguzzo <mauri870@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
I ran go fmt to fix format on the entire repository.
Change-Id: I2f09166b6b8ba0ffb0ba27f6500efb0ea4cf21ff
Reviewed-on: https://go-review.googlesource.com/c/go/+/566835
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
A few tests rely on finalizers running, but are doing tiny allocations.
These tests will break if, for example, the testing package does is own
tiny allocations before calling the test function (see CL 478955). The
tiny allocator will group these allocations together and the ones done
for the tests themselves will live longer than desired. Use types which
have/are pointers for these tests so they won't be allocated by the tiny
allocator.
While here, pick up a small refactor suggested by Michael Knyszek to use
the BlockUntilEmptyFinalizerQueue helper to wait for the finalizers to
run in TestFinalizerRegisterABI.
Change-Id: I39f477d61f81dc76c87fae215339f8a38979cf94
Reviewed-on: https://go-review.googlesource.com/c/go/+/529555
Auto-Submit: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
An upcoming CL will give this call more to do.
For now, separate out the compiler change that
stops inlining the computation.
Change-Id: I4c5cbd84a0694b306191bff38cc6ea2d69458d7d
Reviewed-on: https://go-review.googlesource.com/c/go/+/564556
Auto-Submit: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Continuing conversion from C to Go, change timer API to use methods.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I4cb88a366993a77aa4fad739793a7db7213cc38c
Reviewed-on: https://go-review.googlesource.com/c/go/+/564131
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
The status enumeration is simple enough now that we can
view it as a bit set instead. Switch to a bit set, freeing up
the remaining bits for use in followup work to allow
garbage-collecting timers.
[This is one CL in a refactoring stack making very small changes
in each step, so that any subtle bugs that we miss can be more
easily pinpointed to a small change.]
Change-Id: I5f331fe3db1b5cb52f8571091f97f8ba029f3ac9
Reviewed-on: https://go-review.googlesource.com/c/go/+/564130
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Andy Pan
Ian Lance Taylor
Michael Anthony Knyszek
Roland Shoemaker
Andrey Bokhanko
qmuntal
Lance Yang
Tobias Klauser
Russ Cox
Mauri de Souza Meneguzzo
apocelipes
Pouriya
khr@golang.org
sivchari
Nick Ripley