use_file: Use Acquire/Release semantics instead of Relaxed.

[briansmith]

See the added comment for details. I don't know if this is really an issue but I couldn't convince myself that it isn't ever.

[ironhaven]

I am convinced that relaxed ordering is OK because the mutex should give us acquire/release semantics for the store and prevent double initialization.

Atomic ordering is about controlling how non-atomic variables are viewed by other threads, so having only a single AtomicUsize make this easier. Relaxed atomics are consistent with themselves, so if a thread sees an initialized fd from a relaxed load, it will never see an uninitialized value from future loads.

I sketched out the memory ordering of two racing threads, where one thread wins and initializes the AtomicUsize and the other thread will see the memory write after locking the mutex

First thread:    [FD.load] == UNINIT -> [Mutex.lock] -> [FD.load == UNINIT] -> [FD.store(INIT) (Happens before mutex unlock)] -> [Mutex.unlock (Release memory barrier)] ->Done
             
Second thread:   [FD.load] == UNINIT -> [Mutex.lock (Acquire memory barrier)] -> [FD.load  (Happens after mutex lock)] INIT -> Done
		 == INIT
                     |
                     V
		    DONE

I can see that with relaxed atomics a thread may try to take the lock when the FD is initialized, but it will not write a new value.

[briansmith]

I am convinced that relaxed ordering is OK because the mutex should give us acquire/release semantics for the store and prevent double initialization.

I filed rust-lang/rust#126239 last week exactly to get it clarified, using this library's code as the motivating example.

[briansmith]

To clarify, the issue isn't about the value in FD, but rather the issue with any internal state within libc that isn't stored atomically. AFAICT, in use_file.rs we're operating under the assumption that libc::open works atomically, which is basically does when all the file state is stored in the kernel. But that might not be true if there libc (or whatever) has additional in-process state, or when the "kernel" is in process.

[newpavlov]

that might not be true if there libc (or whatever) has additional in-process state

I think we can safely ignore any such unusual libc implementations, doubly so if there are no practical examples. We inevitably rely on "sensible" behavior of libc, in the case of open the assumption is that it's a thin wrapper around a syscall and that observing a valid FD value is sufficient for doing IO with it without any additional synchronization. Otherwise, we can not do anything with libc since functions in it can theoretically do anything.

when the "kernel" is in process

In my understanding, none of the targets covered by use_file support the "libOS" architecture.

[briansmith]

I think we can safely ignore any such unusual libc implementations, doubly so if there are no practical examples.

All of the sanitizers write unsynchronized state whenever libc::open, libc::read, etc. are called, as mentioned in the initial comment.

[newpavlov]

Hm, in this case I think we have to use Acquire/Release here. I thought we could somehow use memory fence to allow us to keep the relaxed load, but after re-reading docs, it looks like an Acquired load (or fence) is inevitable if we want to synchronize with potential relaxed writes. For example, Once internally uses Acquired loads.

[briansmith]

I rebased this on top of master, copy-edited the comment a bit, and also added a comment about the purpose of the Mutex.