-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add fences to mutex methods #33
Conversation
@thalesfragoso Note that |
How are core library mutexes doing it wrt to acquire/release? Mutexguards should have a similar problem, ain’t they? |
src/lib.rs
Outdated
@@ -61,6 +62,7 @@ impl<T> Mutex<T> { | |||
/// This does not require locking or a critical section since it takes `&mut self`, which | |||
/// guarantees unique ownership already. | |||
pub fn get_mut(&mut self) -> &mut T { | |||
fence(Ordering::SeqCst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this one needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user has a static mut
and knows that the program won't get interrupted at some specific point, and then, they decide to call get_mut
instead of lock
. In that case, if we don't have a fence
the program will get miscompiled. But right now I'm not 100% sure that this use case isn't UB by itself...
godbolt link
I think that in the case of the |
As discussed in the matrix chat, it seems that I also provided a solution for the msp430 case, but the one I found will only compile on nightly, but that seems to be the case in the CC @adamgreig |
@thalesfragoso I don't have a problem making |
Could we just document that That doesn't resolve the |
I also agree with @adamgreig, the fences should be added only when CriticalSection is created, not on every borrow. For example, you obtained a critical section and then do 3 borrows in a row: let cs = ...
let a = *FOO.borrow(cs).borrow();
let b = *BAR.borrow(cs).borrow();
let c = *FOO.borrow(cs).borrow(); You don't need a fence inserted in between each borrow here, because nothing else can change the values of FOO and BAR while you are holding a critical section token. Adding fences here would be a pessimisation, because compiler will no longer be able to see that |
That's already the case. |
I missed that, sorry. |
To summarise discussion in today's meeting on Matrix: we require a fence at the end of the critical section as well, because otherwise the compiler could re-order a store to after the CS finishes, which could race with another thread. Since the CS might be safely forgotten, there's no way to ensure this inside the CS type. Instead, we should add a safety precondition to the CS documentation to say that users (the architecture crates such as cortex-m, cortex-a, msp430, riscv, etc) must ensure a compiler fence is inserted immediately after entering and before leaving the CS. Currently this is mostly the case, for example cortex-m uses either an FFI call or inline asm with a memory clobber to enable and disable interrupts, which has the effect of ensuring a suitable fence. We should make these fences explicit (although it's a shame that So, the update to I don't think this resolves the |
For a little historical context: rust-embedded/cortex-m@a396a68 and rust-embedded/cortex-m@93c901d |
As it was discussed in the meeting, it seems that the best approach to this problem is to add one more condition to the I'm closing this in favor of this new solution, |
34: Document suitable conditions for the safety of CriticalSection and Mutex use r=adamgreig a=thalesfragoso For more information, see #33 and more specifically, #33 (comment). I'm a bit on the fence (heh) with respect to the documentation of `get_mut`, I would like some feedback on that. Co-authored-by: Thales Fragoso <thales.fragosoz@gmail.com> Co-authored-by: Thales <46510852+thalesfragoso@users.noreply.github.com>
Without the fences the Mutex doesn't behave as expected.
Given this example:
It's expected that the value of
FOO
will be evaluated at each loop pass, right now that doesn't happen,FOO
is only evaluated once and if the value is false then the function branchs to an infinite loop:godbolt link (Remove the comment to add the fence and see the difference)
I decided to go for a
fence
instead of acompiler_fence
to cover the cases where the processors have a cache or are out of order. We could changeSeqCst
for a pair ofAcquire
andRelease
, but then I think we would have to put one of them in theDrop
implementation of theCriticalSection
, but we can't really rely that drop will run (mem::forget
), so I went withSeqCst
, maybe I'm missing some other way...It seems that we would need to yank previous versions, unfortunately.