http://bugs.winehq.org/show_bug.cgi?id=29168
--- Comment #145 from Carsten Juttner carjay@gmx.net 2012-03-10 07:43:28 CST --- (In reply to comment #138)
order; from what I read, it should be done like
user_shared_data->SystemTime.High2Time = now.HighPart; user_shared_data->SystemTime.LowPart = now.LowPart; user_shared_data->SystemTime.High1Time = now.HighPart;
You are right, I remember I wanted to scrutinize this a bit more but obviously failed to do so and ended up copying and pasting the original wine source code from the initial initialisation.
So let's do this now. We have L, H1 and H2. Obviously on the "kernel" side this is coherent (since the now-structure is not changing).
Writing L, H1, H2 (as in the patch) concurrently with the expected H1, L, H2 read sequence (in a different thread) leads to 4 cases:
4 relevant points in time: - H1 old, L old, H2 old: ok - H1 old, L new, H2 old: nok, but will not(!) be detected - H1 new, L new, H2 old: nok - H1 new, L new, H2 new: ok
So indeed this code is obviously wrong and whoever wrote this probably thought that it would not matter because it was intended as a one time initial write.
Shows that copying and paste code without reviewing is a bad practice. :)
For the intended write order H2, L, H1 and read order H1, L, H2 this cannot happen (but see below): - H1 old, L old, H2 old: ok - H1 old, L old, H2 new: nok - H1 old, L new, H2 new: nok - H2 new, L new, H2 new: ok
Since this only becomes an issue at the moment the HighTime-Part actually changes this could in theory happen every (1<<32)/10e6 = 429 seconds which is roughly every 7 minutes.
So for a reader application this will appear like the system jumps in time by about 7 minutes.
Sadly, even if the order is corrected in the patch I think this will not be enough since due to the hackish nature several updates could happen concurrently.
Because, if several threads enter a critical section and thus update the page at the same time this may invalidate the strict order guarantee (since a second thread may overwrite H2 with an old value after another thread set it to a new one). So there is still a chance that H and L do not fit.
For a final implementation this also needs to take memory reference reordering and cache coherence issues into account. IIRC x86 is safe here as long as the compiler does not reorder the assembler instructions but for other platforms this may differ.
Some memory fences should help.