RFR: 8288904: Incorrect memory ordering in UL [v2]

[Robbin Ehn]

On Wed, 22 Jun 2022 20:39:37 GMT, Erik Österlund <eosterlund at openjdk.org> wrote:

> Talked to Johan and StefanK earlier today. In general this data structure is very poorly synchronized. Nodes are injected without release store even though there are concurrent readers, there is no fencing or atomics around basically any of the heads or next pointer accesses, despite reading and writing to them concurrently. Not sure how much of it we want to rewrite in this particular patch, but it does need major reworking I think. Maybe the most reasonable option is writing a new data structure entirely. Or maybe this is one of those times where having any form of readers writer lock would make this code way easier to reason about. We already mark where the reader vs writer sections are, but with unnecessarily relaxed and fragile synchronization for what it is. We have other places where we really want a readers writer lock and end up inventing the wheel again. Not sure if fixing this properly should be done in this patch or a follow up patch.

If I remember correctly I suggested this code should use the global counter instead.

On control flow dependencies, even compiler can change the control flow so this is an issue on x86 also. (in this case the variable is volatile, so it eliminates most issue).

According to "Who's afraid of a big bad optimizing compiler?" even this is buggy:

// Writer, stores are ordered
y = 1;
OrderAccess::storestore();
Atomic::store(&x, 1);

// Reader
int xr = Atomic::load(&x);
OrderAccess::loadstore();
if (xr == 1)
  y = 0;
 
// Compiler can re-write the reader as:
int xr = Atomic::load(&x);
OrderAccess::loadstore();
if (xr == 1)
   if (y != 0) // <--- can be reordered with load of x
     y = 0;

This particular compiler re-write seems to be very, very uncommon, but legal.

-------------

PR: https://git.openjdk.org/jdk/pull/9225