RFR (M): 8029396: PPC64 (part 212): Several memory ordering fixes in C-code.

[Lindenmaier, Goetz]

Hi David, 

the GC stuff is only called from shared code.

The ordering in BiasedLocking is needed, e.g., in the context of force_revoke_and_rebias.
If an other thread wants to inflate the lock written to the mark word
in force_revoke_and_rebias, it must be assured that changing the displaced
header is visible to that other thread.

We added the memory barriers for the _thread_state field in 2006 and can 
not reconstruct the concrete cause.  But things as setting the last_java_frame
and then the state to in_native should be ordered.

Best regards,
  Goetz.

-----Original Message-----
From: David Holmes [mailto:david.holmes at oracle.com] 
Sent: Donnerstag, 19. Dezember 2013 11:52
To: Lindenmaier, Goetz
Cc: 'hotspot-dev at openjdk.java.net'; 'ppc-aix-port-dev at openjdk.java.net'; Vladimir Kozlov
Subject: Re: RFR (M): 8029396: PPC64 (part 212): Several memory ordering fixes in C-code.

Somewhat late but I was away for two weeks.

GC stuff:

Is the use of the new release parameter always set to true from shared 
code? If so I don't have a problem with it being used to optimize the 
stores under certain conditions. But if pcc64 will pass true where other 
archs won't then again I object to this because it should be an 
algorithmic correctness requirement that is always present.


General: I find a lot of the commentary excessively platform specific. 
Eg We don't expect any C++ compiler we currently use to emit memory 
barriers for C++ volatiles. If they start doing that we will have a 
bazillion unnecessary injected barriers in our code!

BiasedLocking:

It is not clear to me that the BiasedLocking change is needed. AFAICS 
there is only one path where revoke_bias is not called at a safepoint 
and the comments around that seem to indicate that it was considered 
safe to do so. It may be they assumed TSO when making that decision but 
I'd be interested to know how this change was arrived at.

Thread state:

The thread state changes have me most concerned as they are heavily used 
and so the performance impact here could be extensive. Many of them 
occur on paths that involve membars or membar-equivalent actions so they 
would seem redundant then. Again I would like to see some analysis 
showing these are in fact essential for correctness. There may well be 
some situations where they are, but to me this seems an even better 
candidate for adding the "release" parameter when needed!

David
-----

On 3/12/2013 2:51 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> This change contains a row of fixes to the memory ordering in runtime, GC etc.
> http://cr.openjdk.java.net/~goetz/webrevs/8029396-0-memo/
>
> These are:
> - Accessing arrays in CMS (compactibleFreeListSpace.cpp)
> - CMS: Do release when marking a card dirty. The release must only be done if GC is running. (several files)
> - Method counter initialization (method.hpp).
> - Order accessing f1/f2 in constant pool cache.
> - Release stores in OopMapCache constructor (instanceKLass.cpp).
> - BiasedLocking: Release setting object header to displaced mark.
> - Release state of nmethod sweeper (sweeper.cpp).
> - Do barriers when writing the thread state (thread.hpp).
>
> Please review and test this change.
>
> If requested, I can part this into smaller changes.  But for now
> I wanted to put them all into one change as they all address the
> problems with the PPC memory model.
>
> Best regards,
>    Goetz.
>