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

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

Open
minaevmike opened this Issue Sep 19, 2017 · 7 comments

Comments

Projects
None yet
10 participants
@minaevmike
Contributor

minaevmike commented Sep 19, 2017

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

This comment has been minimized.

Contributor

davecheney commented Sep 19, 2017

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

This comment has been minimized.

Member

dvyukov commented Sep 19, 2017

I think we need Swap there.

@minaevmike minaevmike changed the title from runtime: use atomic.Store instead of simle assignment to runtime: use atomic.Store instead of simple assignment Sep 19, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.10 milestone Sep 19, 2017

@randall77

This comment has been minimized.

Contributor

randall77 commented Sep 19, 2017

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

@choleraehyq

This comment has been minimized.

Contributor

choleraehyq commented Sep 21, 2017

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

@randall77

This comment has been minimized.

Contributor

randall77 commented Sep 21, 2017

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

This comment has been minimized.

gopherbot commented Sep 21, 2017

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

This comment has been minimized.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment