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: use atomic.Store instead of simple assignment #21931

Open
minaevmike opened this issue Sep 19, 2017 · 10 comments
Open

runtime: use atomic.Store instead of simple assignment #21931

minaevmike opened this issue Sep 19, 2017 · 10 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@minaevmike
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

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

master

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

any

May be it would be better to use atomic.Load here? https://github.com/golang/go/blob/master/src/runtime/mprof.go#L443
Anyway it lookg strange - we use atomic operation to store value and non atomic value to read it.
@davecheney davecheney added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2017
@davecheney
Copy link
Contributor

I agree this is is inconsistent. I've always understood it that StoreAtomic needs to be paired with LoadAtomic for proper visibility. If this code is doing something clever, then it probably needs a nice comment to explain why these operations can be intermixed.

/cc @dvyukov @aclements

@dvyukov
Copy link
Member

dvyukov commented Sep 19, 2017

I think we need Swap there.

@minaevmike minaevmike changed the title runtime: use atomic.Store instead of simle assignment runtime: use atomic.Store instead of simple assignment Sep 19, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 19, 2017
@randall77
Copy link
Contributor

We'd also need an atomic load of mutexprofilerate in sema.go:semacquire1

@choleraehyq
Copy link
Contributor

Hi, if nobody working on this I'll take a look and fix the unpaired atomic operations in package runtime.

@randall77
Copy link
Contributor

Sure, thanks.
Keep in mind that the runtime package is pretty special, and there may be cases where it isn't feasible / desirable to make this fix. We should probably take it on a variable-by-variable basis.

@gopherbot
Copy link

Change https://golang.org/cl/65210 mentions this issue: runtime: fix unpaired atomic operations

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017
@odeke-em
Copy link
Member

odeke-em commented Jul 7, 2018

Kindly paging you @choleraehyq, please rebase and fix conflicts in the CL 65210.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jul 10, 2018
@bradfitz bradfitz modified the milestones: Unplanned, Go1.12 Jul 19, 2018
@bradfitz bradfitz added the early-in-cycle A change that should be done early in the 3 month dev cycle. label Jul 19, 2018
@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@smasher164 smasher164 modified the milestones: Backlog, Go1.14 Oct 11, 2019
@andybons
Copy link
Member

We have only three weeks left before the freeze. @bradfitz do you still consider this risky enough that it should wait for the next release?

/cc @aclements

@bradfitz
Copy link
Contributor

This is @aclements's call.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.14, Backlog Dec 5, 2019
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@prattmic
Copy link
Member

In #53821, I am planning to convert atomic fields to runtime/internal/atomic types, which will naturally force all accesses to be atomic and resolve this.

@prattmic prattmic self-assigned this Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests