raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.27k stars 844 forks source link

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

Closed daveythacher closed 6 months ago

daveythacher commented 7 months ago

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 commented 6 months ago

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

daveythacher commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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.

kilograham commented 6 months ago

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

daveythacher commented 6 months ago

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 commented 5 days ago

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