• Michael Anthony Knyszek's avatar
    runtime: fix (*gcSweepBuf).block guarantees · a5a6f610
    Michael Anthony Knyszek authored
    Currently gcSweepBuf guarantees that push operations may be performed
    concurrently with each other and that block operations may be performed
    concurrently with push operations as well.
    
    Unfortunately, this isn't quite true. The existing code allows push
    operations to happen concurrently with each other, but block operations
    may return blocks with nil entries. The way this can happen is if two
    concurrent pushers grab a slot to push to, and the first one (the one
    with the earlier slot in the buffer) doesn't quite write a span value
    when the block is called. The existing code in block only checks if the
    very last value in the block is nil, when really an arbitrary number of
    the last few values in the block may or may not be nil.
    
    Today, this case can't actually happen because when push operations
    happen concurrently during a GC (which is the only time block is
    called), they only ever happen during an allocation with the heap lock
    held, effectively serializing them. A block operation may happen
    concurrently with one of these pushes, but its callers will never see a
    nil mspan. Outside of a GC, this isn't a problem because although push
    operations from allocations can run concurrently with push operations
    from sweeping, block operations will never run.
    
    In essence, the real concurrency guarantees provided by gcSweepBuf are
    that block operations may happen concurrently with push operations, but
    that push operations may not be concurrent with each other if there are
    any block operations.
    
    To fix this, and to prepare for push operations happening without the
    heap lock held in a future CL, we update the documentation for block to
    correctly state that there may be nil entries in the returned slice.
    While we're here, make the mspan writes into the buffer atomic to avoid
    a block user racing on a nil check, and document that the user should
    load mspan values from the returned slice atomically. Finally, we make
    all callers of block adhere to the new rules.
    
    We choose to allow nil values rather than filter them out because the
    only caller of block is markrootSpans, and if it catches a nil entry,
    then there wasn't anything to mark in there anyway since the span is
    just being created.
    
    Updates #35112.
    
    Change-Id: I6450aab15f51690d7a000ba5b3d529cf2ca5da1e
    Reviewed-on: https://go-review.googlesource.com/c/go/+/203318
    Run-TryBot: Michael Knyszek <mknyszek@google.com>
    TryBot-Result: Gobot Gobot <gobot@golang.org>
    Reviewed-by: default avatarAustin Clements <austin@google.com>
    a5a6f610
mgcsweepbuf.go 5.98 KB