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

Make fields within mutex volatile, just in case it matters. #1580

Closed
wants to merge 1 commit into from

Conversation

daveythacher
Copy link
Contributor

Make fields within mutex structure volatile. This should prevent caching in register or stack. SDK currently manages barriers and has no cache to worry about. This is not likely required under most cases. but for completeness.

@kilograham
Copy link
Contributor

not needed specifically because they are protected by memory barriers, and making them volatile may preclude optimization which IS allowed on the core which holds the spin lock

@kilograham kilograham closed this Jan 8, 2024
@daveythacher
Copy link
Contributor Author

If the mutex is cached on two threads on the same core undefined behavior may occur. I will just sort this out on my side.

@kilograham kilograham reopened this Jan 10, 2024
@kilograham
Copy link
Contributor

kilograham commented Jan 10, 2024

Re-opening as i was unaware that GCC would cache memory values across a compiler memory barrier, which is surprising.

edit: actually I think maybe it isn't - may have misread the .DIS

@kilograham
Copy link
Contributor

Yup; things work as expected...

The compiler will not cache a variable across a compiler memory barrier, so variables are reloaded after entering a spin lock whether volatile or not, and equally a context switch for the correct core cannot occur while IRQs are disabled (as they are during the spin locks used here)

... so i'm not sure what issue you are worried about

@daveythacher
Copy link
Contributor Author

We want the owner value to be global. If the compiler optimizes there is no real way to know exactly where it is. My understanding is the compiler is free to destroy the SDK function if it does not cause an issue.

@peterharperuk peterharperuk requested review from kilograham and removed request for kilograham January 12, 2024 10:40
@kilograham
Copy link
Contributor

not sure what you mean by "destroy the SDK function"... if you mean inline it (which would only happen with LTO) it still doesn't matter, as the compiler memory barriers are in place (in the spin lock functions) to make sure things are reloaded where necessary

@kilograham kilograham closed this Jan 12, 2024
@daveythacher
Copy link
Contributor Author

My concern is for decomposing to register in two places. The stacks are not shared address space and the compiler does not know to commit into shared address space.

@daveythacher
Copy link
Contributor Author

Due to #1453, you may want to reconsider this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants