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

[David Holmes]

Hi Goetz,

On 20/12/2013 12:19 AM, Lindenmaier, Goetz wrote:
> Hi David,
>
> the GC stuff is only called from shared code.

Good to hear. I always wonder though whether the cost of passing the 
extra parameter through and checking it, outweighs the benefit of not 
issuing the action (the release in this case)? I'm not a compiler person 
but perhaps the extra parameter forces parameter passing via the stack 
rather than registers, or changes an inlining decision, or maybe the 
additional control flow check causes a problem ... Any hard data that 
not using release semantics all the time actually yields a benefit?

> 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.

I'll take your word for it. I don't have time to try and analyse the 
BiasedLocking code in depth and I don't think it is a performance issue 
for that code given the potentially redundant barrier occurs during a 
safepoint anyway.

> 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.

I am much more concerned about this. I can't accept a simple wrapping of 
all accesses with acquire/release semantics because there may be a few 
cases where it is needed - this code is too hot for that. We need to 
examine the individual cases, like last_java_frame, where you think 
there is an issue.

With regards to this part of the code do you force UseMembar true for 
PPC64? The memory serialization page mechanism is not reliable for 
non-TSO systems.

My Xmas break begins in about 5 hours but I will be checking in on email 
at times :)

Cheers,
David

> 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.
>>